-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hook for mapr or other app to extend webclient main page #425
Conversation
|
||
|
||
<!-- allow omero-mapr OR another app to include custom code --> | ||
{% include "webclient/data/includes/mapr_extension.html" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general 👍 for adding extension points rather than having apps fully overwriting the templates (which quickly turns into a maintenance hell)
I am a bit concerned about the introduction of an extension point explicitly named after a specific app though as introducing this type of circular dependency will eventually cause issues. Is it not possible to override the templates using extends
and block
as defined in https://docs.djangoproject.com/en/3.2/howto/overriding-templates/#extending-an-overridden-template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapr is not extending an existing template to create it's own page (which is what those docs refer to).
It is "hijacking" the template that is included in the "host" webclient page, so change the "host" app behaviour.
When Django looks up a template, e.g. to {% include "webclient/data/includes/right_plugin.general.js.html" %}
in the main webclient page, it goes through all the apps in it's installed apps list until it finds a match.
We deliberately install the omero-web apps after any user-installed apps (
omero-web/omeroweb/settings.py
Line 1542 in 93f89ed
INSTALLED_APPS += ( |
webclient/data/includes/right_plugin.general.js.html
so this is chosen instead of the webclient one.
I don't think that Django provides a mechanism for this kind of behaviour.
However, we could add a mechanism in webclient to allow other apps to inject their code with a configuration (similar to the addition of code for centre / right panels in webclient).
This would just mean that we need to add an additional setting for mapr when installing, but I guess this is easy enough.
|
||
<!-- | ||
This is a hook for use by omero-mapr OR another omero-web app to add custom | ||
code to the <head> of the webclient main page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would head_extension.html
be a reasonable name then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshmoore @sbesson Was the naming of mapr_extension.html
main problem with this PR in it's original form, or was it the fact that this extension point could only be used by 1 app at a time?
If it's only the naming that's the issue, I'm happy to re-name that extension point (e.g. to head_extension.html
) and we could avoid the config that's needed to allow multiple apps to extend (which @sbesson is concerned about the config at ome/omero-mapr#74 (comment)).
Do we need to chat about this to discuss the various options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the naming is the main problem as much as having a good name helps us to understand what the costs/benefits of this strategy actually are. Happy to jump on a call next week Monday or Tuesday if you're still around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, available till Tuesday EOB if you want to schedule a call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm away next week I'm afraid.
The name mapr_extension.html
was meant to warn that this is being used by mapr
, so you should only use it if you don't have mapr
installed (which is explained in the mapr_extension.html
file itself).
Used a config approach - Description above and at ome/omero-mapr#74 updated. |
Conflicting PR. Removed from build OMERO-python-superbuild-push#1286. See the console output for more details.
|
50858ec
to
5166612
Compare
One other option that could allow a single template to be included from each app without configuration: For each app in INSTALLED_APPS, look for e.g. |
That option is now implemented in the last 2 commits above. |
I updated the description above to match the current behaviour. cc @sbesson |
code to the <head> of the webclient main page. | ||
|
||
Simply add a file with the same path under the app/templates directory: | ||
/app/templates/webclient/data/includes/mapr_extension.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still correct with the latest version of this PR? This is what this file and the description suggest but from the util.py
changes, I would expect the file should be called webclient_head
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I forgot to delete that file. It's not needed any more.
omeroweb/webgateway/util.py
Outdated
"""For each installed app, try to load AppConfig and return label""" | ||
|
||
labels = [] | ||
for app in settings.INSTALLED_APPS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what is the contract for the order in which these apps are resolved? Alphabetical order? Order in the configuration?
And assuming several apps would try to overwrite the same variable, would the last app win?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apps are ordered as in the omero.web.apps
config, but order is not important as app labels are unique.
Excluding ahead of dependencies upgrade work EDIT: (Will): removing exclude flag - 21st March 2023 |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
I might not have fully wrapped my head around all changes, but I'm not yet convinced we need an additional way to include custom plugin files, versus just utilizing the Django template inheritance mechanism. For example, if // This can be overwritten by plugins to provide custom URL for right panel
// NB: The returned html should have an element <div id="object-id">oid</div>
// where the oid matches the oid from this function
{% block custom-right-panel-url %}
OME.getCustomRightPanelUrl = function getCustomRightPanelUrl(oid) {
return;
}
{% endblock %} A plugin (or multiple plugins) would have its own {% extends "right_plugin.general.js.html" %}
{% block custom-right-panel-url %}
{{ block.super }}
(function () {
var old = OME.getCustomRightPanelUrl;
OME.getCustomRightPanelUrl = function getCustomRightPanelUrl(oid) {
return old(oid) || some_new_value;
}
})();
{% endblock %} Similarly other template blocks in Again, there may be a limitation here that I'm not seeing, but right now a whole new way to include plugin functionality seems overkill to me. |
@knabar Thanks - yes you are right that I don't need all that complexity! "This technique works because the template loader does not consider the already loaded override template (at templates/admin/base_site.html) when resolving the extends tag." |
tested on merge-ci, I can see both the fading-in spinner and a correct Mapr behaviour, with the custom external and internal links working. lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirmed the expected behaviour of this PR on merge-ci without mapr change (excluded ome/omero-mapr#74 (comment)).
Mapr works as expected but with the existing behaviour of overwriting the right panel (no loading spinner etc).
As noted in #423 (comment)
changes to the webclient
templates/webclient/data/includes/mapr_extension.html
file are completely overwritten bymapr
, so any changes to that file must previously be duplicated in the corresponding file inomero-mapr
.Mapr needed to overwrite
update_metadata_general_tab()
to provide custom URL loading when custom items are selected.This PR provides a function
OME.getCustomRightPanelUrl()
which can be overwritten, instead of replacing the whole ofupdate_metadata_general_tab()
.This change is used by mapr in https://github.com/ome/omero-mapr/pull/74/files
To test: