Error if `region_nav_plot` is attached to a panel rather than the plot

[sir4ur0n]

See https://github.com/statgen/locuszoom/issues/270#issuecomment-1131954582

region_nav_plot widget should only be attached to the plot, not to an individual panel, since it changes the global state for all panels.

Logic can probably be copy-pasted from the set_state widget

[sir4ur0n]

I just realized I never stated it in the original message: I'll try to contribute this to LZ library if that's ok/helpful. I can't make any promise on the timeframe, but I'm keeping it in my todolist.

[abought]

Thanks. We're always very happy to accept contributions from others, and I'd be happy to help answer any questions related to contributions.

(there are one or two slightly odd quirks internally, due to backwards compatibility reasons. Hopefully the examples provide a good starting point)

[sir4ur0n]

So I started looking at how to implement it, and it looks like it's not a simple matter of copy-pasting the checks from SetState (esm/components/toolbar/widgets.js:1531) because while SetState never makes sense to be attached to a single panel (since it mutates the whole plot state), some toolbars make sense panel-wise, others don't.

So far no "elegant" solution comes to my mind, here are some possibilities I can think of:

1. In Toolbar constructor (esm/components/toolbar/index.js:16), introspect the widgets, and if any "global" widget is found (e.g. shift_region or zoom_region types) and parent is a Panel, throw an error. This is a bit error-prone (easy to forget some types as new types of widgets are added to LZ) and not modular (if developers add their own global widgets, this check would not detect them) but otherwise I suspect it's the "simplest" solution (local change, works for basic workflows)?
2. Add some field to the toolbar JSON, e.g. is_global: boolean. In Toolbar constructor, if is_global === true && type !== 'plot', then throw an error. This is also error-prone because one could easily forget to add the right is_global value to a toolbar JSON.
3. Split Toolbar into PanelToolbar and PlotToolbar entirely (or create PlotToolbar which would inherit from Toolbar) and check in the constructor the parent. This may be a bigger refactoring, but might be clearer documentation-wise?

Related questions:

• should "global (plot)" and "local (widget and/or plot)" toolbars be split in the LZ register namespace? E.g. split LocusZoom.Layouts._items.toolbar into LocusZoom.Layouts._items.plot_toolbar and LocusZoom.Layouts._items.widget_toolbar?
• Do some toolbars or widgets make sense both at the plot level and the panel level?

[abought]

Hmm. This is a fair question.

Premade toolbars are nothing more than a list of widget configuration options, provided for convenience when someone wants to get a bunch of related buttons by at once. By itself, nothing in LocusZoom.Layouts.get('toolbar'...) has code of its own.

Thus from a "modular design" standpoint, I think the right approach is to focus the fix on widgets, not toolbars. Much like set_state, we should have each widget ask if it's in the right place when it is first created. Configuration options tell us what to create, and it is the classes (named in widget_options.type) that contain the actual custom code. Users shouldn't ever see a totally broken plot, so the focus should be on giving helpful feedback to the developer (prevent broken code from ever being shipped).

> LocusZoom.Layouts.get('toolbar', 'region_nav_plot').widgets.map(item => item.type)
[ "title", "download", "download_png", "shift_region", "shift_region", "zoom_region", "zoom_region", "shift_region", "shift_region" ]

Looking at this list, most of these widget types only really make sense at plot level (because they operate on the entire plot- multiple copies would be redundant). So, to think aloud:

• this check should be added to the constructor for shift_region, download_svg (png will inherit for free!), and zoom_region.
• As we get stricter about checking these things, error messages should improve to include the name of the widget that has the problem. (eg, the check in set_state should say it is a problem with set_state)
• As you mention: if someone is making a custom layout, they will probably bypass any special options added to the toolbar JSON / configuration options. A targeted fix at the point of use is better than a slightly magical global option.
• The secondary usability issue is that, if we choose defaults well, most people never really think about the internals at all- they would get error messages about modular pieces, without knowing that a toolbar was made up of individual pieces at all. The tutorials were a start... someday a validator (jsonschema or otherwise) could show people the entire layout to give better feedback about context.

There's a real challenge in creating a system modular enough to do anything, but clear enough for our target audience (biologists) to get started with easy defaults. :) I very much want to see it improve and I'm quite glad to see new ideas!

[abought]

Lastly: There are definitely widgets that make sense at all levels. Title is the obvious example, but I actually could imagine using set_state to control the behavior of a particular data source only used by one panel. (eg, one set_state could control the LD population for all tracks; another set_state button could trigger a value only looked at by one specific panel- like to tell a data source "modify the request for data, based on a parameter in plot.state").

The user would never know it was a generic button under the hood, but a clever developer could give the impression of panel-specific behavior, and in that case, it makes sense to let the button be close to what gets changed. Thinking about this a bit, we actually might not want to restrict the most generic buttons (like set_state) too much.

By contrast, things that change the plot view region (like shift/zoom) typically affect all panels. It makes sense to restrict things that can only be used in one specific way, and only globally.

[sir4ur0n]

Regarding your last comment: is the conclusion that we should add current set_state's check to shift_region, download_svg and zoom_region, and remove the same check from set_state? I think I agree. While set_state mutates the global state, it may very well mutate a sub-part of the state used by a single panel. That would unnecessarily restrict valid uses of set_state.

By the way, should those be throwing errors? An alternative might be to:

• either log a warning
• or show an alert
• or show a warning above/below the plot
• or a combination of the above

And optionally to provide a way to disable this warning.

E.g.:

Warning: Global widget shift_region is attached to a panel rather than the encompassing plot. This is most likely not what you want, because this widget will affect the plot state, thus all panels. If you know what you are doing and want to disable this warning for all widgets, set warnings.plot_widget_in_panel: false in the plot state.

That seems like a decent compromise:

• we help the developer/biologist detect misconfigurations and understand why this is most likely a misconfiguration
• we do not block "power users" who know what they are doing

Drawbacks:

• adds a bit of complexity to the code (a few more ifs here and there)
• adds a new kind of configuration (enabling/disabling warnings), i.e. more documentation

[abought]

Hmm. Good questions.

1. Broadly, I think that list of widets makes sense for where to put the errors.
2. Adding a widget to an invalid location means that code should break (never run). This is better handled as a runtime exception (throw new Error) (which will automatically get logged in the dev console). We shouldn't add UI for the error, because end users should never see a website if the code did not run as written.
   • Errors should be shown in the right context when they are actionable; devs can fix the bug but end users cannot.
3. I'm all for better error messages! Trim it down a little for brevity. This widget may only be added to plot-level toolbars, because it would affect settings shared across all panels
4. ...but I'd avoid making basic errors configurable without a very clear use case.
   • Everyone likes to compromise by "doing both", but often the second option ("config.option = false") doesn't get used (and handling the options makes the code harder to read/ write). You wouldn't know it from the docs, but I've actually removed dozens of LocusZoom options without anyone noticing.
   • Another form of compromise is to make improvements over time. We try to listen to users, and add features quickly on request. When building shared tools: "It's easy to add features, but hard to take them away"