Fix ping lag in plot options (spacetelescope#2326)

* avoid updating stretch histogram if no expected updates

* the browser can throttle pings, which can result in 'is_active' toggling "constantly", which with an expensive call to update the stretch histogram can result in extremely laggy behavior.  This tracks whether there are any known changes since the last time is_active was toggled, and skips re-computing the histogram unless needed

* minor layout fixes

* number of bins and the stretch_hist_zoom_limits toggle were in the same row, resulting in undesired behavior for wide plugin width

* changelog entry

* fix zoom-limits callback not sending msg

* fix/optimize compass behavior with is_active

Author: kecnry

CHANGES.rst

@@ -72,6 +72,12 @@ Cubeviz
 Imviz
 ^^^^^
 
+- Fixes possible extreme lag when opening the Plot Options plugin. [#2326]
+
+- Fixes minor layout issues in the Plot Options plugin. [#2326]
+
+- Fixes compass updating in popout/inline mode. [#2326]
+
 Mosviz
 ^^^^^^
 

jdaviz/configs/default/plugins/plot_options/plot_options.py

@@ -249,6 +249,12 @@ class PlotOptions(PluginTemplateMixin):
     show_viewer_labels = Bool(True).tag(sync=True)
 
     def __init__(self, *args, **kwargs):
+        # track whether the stretch histogram needs an update (some entry has changed) if is_active
+        # becomes True, to address potential lag from a backlog of calls to
+        # _update_stretch_histogram if the browser throttles pings
+        # (https://github.com/spacetelescope/jdaviz/issues/2317)
+        self._stretch_histogram_needs_update = True
+
         super().__init__(*args, **kwargs)
         self.viewer = ViewerSelect(self, 'viewer_items', 'viewer_selected', 'multiselect')
         self.layer = LayerSelect(self, 'layer_items', 'layer_selected', 'viewer_selected', 'multiselect')  # noqa
@@ -494,26 +500,37 @@ def vue_set_value(self, data):
     @observe('is_active', 'layer_selected', 'viewer_selected',
              'stretch_hist_zoom_limits')
     def _update_stretch_histogram(self, msg={}):
-        # Import here to prevent circular import.
-        from jdaviz.configs.imviz.plugins.viewers import ImvizImageView
-        from jdaviz.configs.cubeviz.plugins.viewers import CubevizImageView
-
-        if not self.stretch_function_sync.get('in_subscribed_states'):  # pragma: no cover
-            # no (image) viewer with stretch function options
-            return
         if not hasattr(self, 'viewer'):  # pragma: no cover
             # plugin hasn't been fully initialized yet
             return
-        if (not self.is_active
-                or not self.viewer.selected
-                or not self.layer.selected):  # pragma: no cover
-            # no need to make updates, updates will be redrawn when plugin is opened
-            # NOTE: this won't update when the plugin is shown but not open in the tray
-            return
-        if not isinstance(msg, dict) and not self.stretch_hist_zoom_limits:  # pragma: no cover
-            # then this is from the limits callbacks and we don't want to waste resources
+
+        if not isinstance(msg, dict):  # pragma: no cover
+            # then this is from the limits callbacks
             # IMPORTANT: this assumes the only non-observe callback to this method comes
             # from state callbacks from zoom limits.
+            if not self.stretch_hist_zoom_limits:
+                # there isn't anything to update, let's not waste resources
+                return
+            # override msg as an empty dict so that the rest of the logic doesn't have to check
+            # its type
+            msg = {}
+
+        if msg.get('name', None) == 'is_active' and not self._stretch_histogram_needs_update:
+            # this could be re-triggering if the browser is throttling pings on the js-side
+            # and since this is expensive, could result in laggy behavior
+            return
+        elif msg.get('name', None) != 'is_active' and not self.is_active:
+            # next time is_active becomes True, we need to update the histogram plot
+            self._stretch_histogram_needs_update = True
+            return
+
+        if not self.stretch_function_sync.get('in_subscribed_states'):  # pragma: no cover
+            # no (image) viewer with stretch function options
+            return
+
+        if (not self.viewer.selected or not self.layer.selected):  # pragma: no cover
+            # nothing to plot
+            self.stretch_histogram.clear_all_marks()
             return
 
         if self.multiselect and (len(self.viewer.selected) > 1
@@ -523,6 +540,11 @@ def _update_stretch_histogram(self, msg={}):
             self.stretch_histogram.clear_all_marks()
             return
 
+        # Import here to prevent circular import (and not at the top of the method so the import
+        # check is avoided, whenever possible).
+        from jdaviz.configs.imviz.plugins.viewers import ImvizImageView
+        from jdaviz.configs.cubeviz.plugins.viewers import CubevizImageView
+
         if not isinstance(self.viewer.selected_obj, (ImvizImageView, CubevizImageView)):
             # don't update histogram if selected viewer is not an image viewer:
             return
@@ -552,6 +574,7 @@ def _update_stretch_histogram(self, msg={}):
 
         comp = data.get_component(data.main_components[0])
 
+        # TODO: further optimization could be done by caching sub_data
         if self.stretch_hist_zoom_limits:
             if hasattr(viewer, '_get_zoom_limits'):
                 # Viewer limits. This takes account of Imviz linking.
@@ -598,6 +621,8 @@ def _update_stretch_histogram(self, msg={}):
             # we'll force the traitlet to trigger a change
             hist_mark.send_state('sample')
 
+        self._stretch_histogram_needs_update = False
+
     @observe('stretch_vmin_value')
     def _stretch_vmin_changed(self, msg=None):
         self.stretch_histogram.marks['vmin'].x = [self.stretch_vmin_value, self.stretch_vmin_value]

jdaviz/configs/default/plugins/plot_options/plot_options.vue

@@ -348,28 +348,32 @@
       <glue-float-field label="Stretch VMax" :value.sync="stretch_vmax_value" />
     </glue-state-sync-wrapper>
 
-    <v-row v-if="stretch_function_sync.in_subscribed_states">
-      <!-- z-index to ensure on top of the jupyter widget with negative margin-top -->
-      <v-text-field
-          ref="stretch_hist_nbins"
-          type="number"
-          label="Number of Bins"
-          v-model.number="stretch_hist_nbins"
-          hint="The amount of bins used in the histogram."
-          persistent-hint
-          :rules="[() => stretch_hist_nbins !== '' || 'This field is required',
-                   () => stretch_hist_nbins > 0 || 'Number of Bins must be greater than zero']"
-      ></v-text-field>
-      <v-switch
-        v-model="stretch_hist_zoom_limits"
-        class="hide-input"
-        label="Limit histogram to current zoom limits"
-        style="z-index: 1"
-      ></v-switch>
-      <!-- NOTE: height defined here should match that in the custom CSS rules
-           below for the bqplot class -->
-      <jupyter-widget :widget="stretch_histogram_widget"/>
-    </v-row>
+    <div v-if="stretch_function_sync.in_subscribed_states">
+      <v-row>
+        <v-text-field
+            ref="stretch_hist_nbins"
+            type="number"
+            label="Number of Bins"
+            v-model.number="stretch_hist_nbins"
+            hint="The amount of bins used in the histogram."
+            persistent-hint
+            :rules="[() => stretch_hist_nbins !== '' || 'This field is required',
+                     () => stretch_hist_nbins > 0 || 'Number of Bins must be greater than zero']"
+        ></v-text-field>
+      </v-row>
+      <v-row>
+        <!-- z-index to ensure on top of the jupyter widget with negative margin-top -->
+        <v-switch
+          v-model="stretch_hist_zoom_limits"
+          class="hide-input"
+          label="Limit histogram to current zoom limits"
+          style="z-index: 1"
+        ></v-switch>
+        <!-- NOTE: height defined here should match that in the custom CSS rules
+             below for the bqplot class -->
+      </v-row>
+        <jupyter-widget :widget="stretch_histogram_widget"/>
+      </div>
 
     <!-- IMAGE:IMAGE -->
     <j-plugin-section-header v-if="image_visible_sync.in_subscribed_states">Image</j-plugin-section-header>

jdaviz/configs/imviz/plugins/compass/compass.py

@@ -65,9 +65,12 @@ def _compass_with_new_viewer(self, *args, **kwargs):
             # mixin object not yet initialized
             return
 
+        if not self.is_active:
+            return
+
         # There can be only one!
         for vid, viewer in self.app._viewer_store.items():
-            if vid == self.viewer.selected_id and self.plugin_opened:
+            if vid == self.viewer.selected_id:
                 viewer.compass = self
                 viewer.on_limits_change()  # Force redraw