Skip to content
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.

Sign up for GitHub

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

Context-aware plugin relevancy #2711

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Context-aware plugin relevancy #2711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9edd445
infra for plugins to be considered "irrelevant"
kecnry Feb 16, 2024
12de8a1
improve dev docs for irrelevant_msg
kecnry Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/dev/ui_style_guide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ try to adhere to the following principles:
To enable the "Keep active" check, pass `` :uses_active_status="uses_active_status" @plugin-ping="plugin_ping($event)" :keep_active.sync="keep_active" ``.
Any changes to style across all plugins should then take place in the
``j-tray-plugin`` stylesheet (``jdaviz/components/tray_plugin.vue``).
* ``disabled_msg`` should be set to replace the UI with a message explaining why the plugin is disabled.
``irrelevant_msg`` should be set to skip the plugin in the UI entirely where the user would not need an explanation (slice plugin not
relevant because cube data is not present, for example).
* Each item should be wrapped in a ``v-row``, but avoid any unnecessary additional wrapping-components
(``v-card-*``, ``v-container``, etc).
* Only use ``v-col`` components (within a ``<v-row class="row-no-outside-padding">``) if multiple
Expand Down Expand Up @@ -71,6 +74,11 @@ try to adhere to the following principles:
<j-tray-plugin
:description="docs_description || 'Plugin description.'"
:link="docs_link || 'https://jdaviz.readthedocs.io/en/'+vdocs+'/'+config+'/plugins.html#plugin-name'"
:irrelevant_msg="irrelevant_msg"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add explanation on when devs should consider something "irrelevant" or not. Do we still need "disabled" then? What are the differences when devs decide which one to use?

Copy link
Contributor

@bmorris3 bmorris3 Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distinction with disabled is meaningful. "irrelevant" extends upon the “disabled” plugin feature, which marks a plugin as disabled with a message in the UI, but does not “remove” it from the plugin tray or API. "irrelevant" additionally removes plugins from the UI and public API. The plugin won’t be visible in viz.plugins. But if you already assigned the plugin user API to a variable, that will still work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So once it is marked as irrelevant, can it become relevant again later in the same session? I think this was discussed before but I don't remember and none of those discussions are documented in the diff.

Is the only use for this to hide a WIP plugin?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only use for this to hide a WIP plugin?

It can have more general uses. We could also use it to put plugins away depending on the state of the app or viewer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a traitlet, so we can respond to anything we want to toggle its "relevancy", yes. Disabled leaves the plugin in the tray but just replaces the UI with a message (which is useful for cases where you want to tell the user WHY something disappeared). Imagine merging specviz and cubeviz, its obvious that the slice plugin is irrelevant without cube data, but hiding spectral analysis because of a unit conversion switch probably deserves showing a message. We can clarify a pattern for when to use each as they are used more often - and the under-the-hood implementation is very similar, so should be easy to migrate from one to the other when warranted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not really sure where to document this in the docs and I think it'll be a bit liquid for a while until we get a clear idea of how we want to use this... for now, there at least is a good papertrail in this discussion - and the PR description, so thanks for brining it up)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clarify the pattern now as you envisioned and clean it up later? We cannot guarantee new devs would be able to dig up this conversation here. A few sentences in the dev docs at least would be searchable in RTD.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

:disabled_msg="disabled_msg"
:uses_active_status="uses_active_status"
@plugin-ping="plugin_ping($event)"
:keep_active.sync="keep_active"
:popout_button="popout_button">

<v-row>
Expand Down
2 changes: 2 additions & 0 deletions jdaviz/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2523,9 +2523,11 @@ def compose_viewer_area(viewer_area_items):
app=self, plugin_name=tray_item_label, **optional_tray_kwargs
)

# NOTE: is_relevant is later updated by observing irrelevant_msg traitlet
self.state.tray_items.append({
'name': name,
'label': tray_item_label,
'is_relevant': len(tray_item_instance.irrelevant_msg) == 0,
'widget': "IPY_MODEL_" + tray_item_instance.model_id
})

Expand Down
2 changes: 1 addition & 1 deletion jdaviz/app.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
></v-text-field>
<v-expansion-panels accordion multiple focusable flat tile v-model="state.tray_items_open">
<v-expansion-panel v-for="(trayItem, index) in state.tray_items" :key="index">
<div v-if="trayItemVisible(trayItem, state.tray_items_filter)">
<div v-if="trayItem.is_relevant && trayItemVisible(trayItem, state.tray_items_filter)">
<v-expansion-panel-header >
<j-tooltip :tipid="trayItem.name">
{{ trayItem.label == 'Orientation' ? 'Orientation (prev. Links Control)' : trayItem.label }}
Expand Down
4 changes: 2 additions & 2 deletions jdaviz/components/tray_plugin.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@

<script>
module.exports = {
props: ['disabled_msg', 'description', 'link', 'popout_button',
props: ['irrelevant_msg', 'disabled_msg', 'description', 'link', 'popout_button',
'uses_active_status', 'keep_active'],
methods: {
isDisabled() {
return this.getDisabledMsg().length > 0
},
getDisabledMsg() {
return this.disabled_msg || ''
return this.irrelevant_msg || this.disabled_msg || ''
},
sendPing(recursive) {
if (!this.$el.isConnected) {
Expand Down
1 change: 1 addition & 0 deletions jdaviz/configs/cubeviz/plugins/slice/slice.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<template>
<j-tray-plugin
:description="docs_description || 'Select slice of the cube to show in the image viewers. The slice can also be changed interactively in the spectrum viewer by activating the slice tool.'"
:irrelevant_msg="irrelevant_msg"
:link="docs_link || 'https://jdaviz.readthedocs.io/en/'+vdocs+'/'+config+'/plugins.html#slice'"
:popout_button="popout_button">

Expand Down
2 changes: 1 addition & 1 deletion jdaviz/core/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def plugins(self):
dict of plugin objects
"""
plugins = {item['label']: widget_serialization['from_json'](item['widget'], None).user_api
for item in self.app.state.tray_items}
for item in self.app.state.tray_items if item['is_relevant']}

# handle renamed plugins during deprecation
if 'Orientation' in plugins:
Expand Down
16 changes: 13 additions & 3 deletions jdaviz/core/template_mixin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,8 @@ class PluginTemplateMixin(TemplateMixin):
"""
This base class can be inherited by all sidebar/tray plugins to expose common functionality.
"""
disabled_msg = Unicode("").tag(sync=True)
disabled_msg = Unicode("").tag(sync=True) # noqa if non-empty, will show this message in place of plugin content
irrelevant_msg = Unicode("").tag(sync=True) # noqa if non-empty, will exclude from the tray, and show this message in place of any content in other instances
docs_link = Unicode("").tag(sync=True) # set to non-empty to override value in vue file
docs_description = Unicode("").tag(sync=True) # set to non-empty to override value in vue file
plugin_opened = Bool(False).tag(sync=True) # noqa any instance of the plugin is open (recently sent an "alive" ping)
Expand Down Expand Up @@ -384,6 +385,14 @@ def user_api(self):
# can even be dependent on config, etc.
return PluginUserApi(self, expose=[])

@observe('irrelevant_msg')
def _irrelevant_msg_changed(self, *args):
labels = [ti['label'] for ti in self.app.state.tray_items]
if self._registry_label not in labels:
return
index = labels.index(self._registry_label)
self.app.state.tray_items[index]['is_relevant'] = len(self.irrelevant_msg) == 0

def vue_plugin_ping(self, ping_timestamp):
if isinstance(ping_timestamp, dict):
# popout windows can sometimes ping but send an empty dictionary instead of the
Expand Down Expand Up @@ -464,9 +473,10 @@ def close_in_tray(self, close_sidebar=False):
if close_sidebar:
self.app.state.drawer = False

@observe('plugin_opened', 'keep_active')
@observe('plugin_opened', 'keep_active', 'irrelevant_msg')
def _update_is_active(self, *args):
self.is_active = self.keep_active or self.plugin_opened
self.is_active = ((len(self.irrelevant_msg) == 0)
and (self.keep_active or self.plugin_opened))

@contextmanager
def as_active(self):
Expand Down