mapr extension of webclient

[will-moore]

See https://github.com/ome/omero-web/pull/425

This uses the OME.getCustomUrl() function in that PR to allow extending a webclient template instead of duplicating and replacing right_plugin_general.js.html. All customisations that mapr needs are moved into an extension of the base_container.html, adding content to the top of the page in the script block.

To test:

• mapr should continue to create custom links from map-annotations as before
• mapr doesn't overwrite the right-hand panel, so recent features (such as fading-in loading spinner) should be working (that feature was never replicated in mapr, so before this PR is was overwritten by mapr and lost)

[will-moore]

To test:

• Browse to images with map annotations, e.g. https://merge-ci.openmicroscopy.org/web/mapr/gene/?value=CENPJ
• Links should still be looking like:

[pwalczysko]

The test described in https://github.com/ome/omero-mapr/pull/74#issuecomment-1344278519 is passing.

But when I click in the LHP onto the top node (, I get a 404 error

[will-moore]

@pwalczysko yes, sorry - I had noticed that 404 error earlier. Hopefully it should be fixed by the last commit above...

[sbesson]

This uses the hook added in that PR to extend webclient without overwriting right_plugin.general.js.html.

This overwriting strategy has caused much pain especially in terms of tracking and syncing upstream changes. Thus I am very supportive to any effort to getting rid of it.

I have two concerns about the current proposal:

• from the OMERO.web perspective, there is a danger of turning every single customisation as a new property as this opens the way to a potential explosion of configurations.
• from an end-user perspective, until now, I would set-up this extension by installing the package, registering the app in the config and setting a few configurations defining the namespaces that my OMERO.web should recognize. Why do I need to also set an extra property at installlation time to turn existing behavior on ?

Is it not possible to extend the base right_plugin.general.js.html after defining the relevant extension points in the upstream template?

[will-moore]

If I wanted to create a new page, e.g. at mapr/right_panel/ and this page wanted to extend the code in right_plugin.general.js.html I could create a new template mapr/right_panel.js which could start with: {% extends "webclient/data/includes/right_plugin.general.js.html" %} and if that template had e.g. {% block custom %} then the mapr/right_panel.js could extend that and add it's own code.

But this in not what mapr needs to do. Mapr does not create a new page. I wants to change the behaviour of the host page. Django doesn't provide a mechanism for this (that I am aware of) other than this settings approach.

This is the same approach used for right-panel and centre panel extensions, and top links etc. But we don't have a hook for doing what I'm proposing here (a simple point to inject code into the page, after other JavaScript, so we can overwrite various methods).

The webgateway base_site.html template used to support an extra_js scripts (https://github.com/ome/omero-web/blob/59ec465df1a4f55c7097696647e14dccc810a602/omeroweb/webgateway/templates/webgateway/base_site.html#L44) but the config to support this has been removed long ago - so these should be cleaned up now.

I would also prefer not to add configuration if possible, which is why I first used the alternative approach of providing a single template path that could be used to inject custom code without configuration. The limitation of this approach is that it can only be used by one app at a time, whereas the config option allows multiple apps to add their templates to the list to be included.

[pwalczysko]

Yes, the 404 Error reported in https://github.com/ome/omero-mapr/pull/74#issuecomment-1348910201 is solved now. LGTM - happy for this to be merged once the discussion about the code architecture is solved.

[will-moore]

To summarise the pros and cons of various approaches:

• Current approach: mapr replaces webclient right_plugin_general.html.js with it's own copy
  • Pros: No change needed in webclient, No config needed in mapr
  • Cons: Code duplication: changes in webclient need to be ported to mapr. Not reusable by any other app
• webclient provides a single extension point (see first commit at https://github.com/ome/omero-web/pull/425)
  • Pros: No config needed in mapr
  • Cons: Can only be used by a single app. This fixes the code duplication issue but doesn't provide a reusable extension point for other apps
• webclient includes a configurable list of templates (see https://github.com/ome/omero-web/pull/425/commits/c93b5374632e1e6970d5a5eee63c582994ce5110):
  • Pros: provides an extension point that can be used by multiple apps
  • Cons: Requires configuration in each app (e.g. mapr). If this is intended for use by other apps, then we have to document it and support it etc. Any script added to the page can access anything on the page. There is no API, so no guarantee that a script that overrides existing functions or manipulates the page will continue to work.

EDIT: My preference is for a new option added below!

[will-moore]

Ah - I remembered I'd thought of another option at https://github.com/ome/omero-web/pull/425#issuecomment-1362213175

• Each app can add a single template at /app_label/includes/webclient_head.html
  • Pros: allows multiple apps to extend webclient, with no config
  • Cons: only 1 template per app. If we want others to use it, we still have to document etc (new feature)

This option has the best Pros/Cons balance and is now implemented in this PR and at https://github.com/ome/omero-web/pull/425

[joshmoore]

If I understand correctly, @will-moore, this is starting to feel much more "extension-y" (as opposed to "hack-y" or "custom-y") but a configuration is still required, right? In which case, I guess from our conversation it's @sbesson who should be feeding back.

The only concern I have but could imaging leaving for a future iteration of refactoring is the hard overwriting of the global function. That feels like it isn't something that we can suggest as a general practice and therefore won't scale (i.e. "custom-y")

[will-moore]

There is no configuration required as in omero config set..... The AppConfig.label is existing part of most apps, specifying the convention that e.g. omero-mapr URL is /mapr/. Most apps also store their templates under the same dir as the label e.g. /mapr/ or /figure/. So, this kinda codifies that convention a bit more.

[will-moore]

Closing for now since https://github.com/ome/omero-web/pull/425 is excluded...

[will-moore]

This is now deployed on idr-testing. cc @sbesson @francesw @jburel @pwalczysko

[pwalczysko]

@will-moore explained that https://github.com/ome/omero-mapr/pull/74#issuecomment-1488563537 is a new addition to mapr which aims to have the Description "cleaned-up". With it, the Description does not need to contain fields such as Publication title any longer, as those will be on the top of RHP.

The RHP looks good to me on idr-testing.

Nothing against this cleaning of the Description, but I think it would be better to have this mapr change in a separate PR ?

[pwalczysko]

lgtm on idr-testing

[will-moore]

Excluding this temporarily to test that https://github.com/ome/omero-web/pull/425 works OK without this PR (not a breaking change).

EDIT - removed flag.