Merge pull request glue-viz#1278 from astrofrog/refactor-histogram

Refactor built-in histogram viewer

CHANGES.md

@@ -8,6 +8,11 @@ v0.11.0 (unreleased)
   references, which in turn caused some callback functions to not be
   cleaned up. [#1281]
 
+* Rewrote the histogram viewer to use the new state infrastructure. This
+  significantly simplifies the actual histogram viewer code both in terms
+  of number of lines and in terms of the number of connections/callbacks
+  that need to be set up manually. [#1278]
+
 v0.10.3 (unreleased)
 -------------------
 

doc/python_guide/data_viewer_options.rst

@@ -65,9 +65,9 @@ Here are the settings associated with each data viewer:
     ~ImageWidget.rgb_mode
     ~ImageWidget.slice
 
-.. currentmodule:: glue.viewers.histogram.qt.viewer_widget
+.. currentmodule:: glue.viewers.histogram.qt.data_viewer
 
-:class:`Histogram Viewer <HistogramWidget>`
+:class:`Histogram Viewer <HistogramViewer>`
 ---------------------------------------------
 
 .. autosummary::

glue/app/qt/tests/test_application.py

@@ -22,7 +22,7 @@
 from glue.utils.qt import process_dialog
 from glue.viewers.image.qt import ImageWidget
 from glue.viewers.scatter.qt import ScatterWidget
-from glue.viewers.histogram.qt import HistogramWidget
+from glue.viewers.histogram.qt import HistogramViewer
 
 
 from ..application import GlueApplication
@@ -315,9 +315,9 @@ def test_multi_tab(self):
         dc = DataCollection([d])
 
         app = GlueApplication(dc)
-        w1 = app.new_data_viewer(HistogramWidget, data=d)
+        w1 = app.new_data_viewer(HistogramViewer, data=d)
         app.new_tab()
-        w2 = app.new_data_viewer(HistogramWidget, data=d)
+        w2 = app.new_data_viewer(HistogramViewer, data=d)
         assert app.viewers == ((w1,), (w2,))
 
         self.check_clone(app)
@@ -327,7 +327,7 @@ def test_histogram(self):
         dc = DataCollection([d])
 
         app = GlueApplication(dc)
-        w = app.new_data_viewer(HistogramWidget, data=d)
+        w = app.new_data_viewer(HistogramViewer, data=d)
         self.check_clone(app)
 
         dc.new_subset_group()

glue/app/qt/tests/test_preferences.py

@@ -12,7 +12,7 @@
 from glue.app.qt import GlueApplication
 from glue.viewers.scatter.qt import ScatterWidget
 from glue.viewers.image.qt import ImageWidget
-from glue.viewers.histogram.qt import HistogramWidget
+from glue.viewers.histogram.qt import HistogramViewer
 from glue.plugins.dendro_viewer.qt.viewer_widget import DendroWidget
 
 rgb = ColorConverter().to_rgb
@@ -313,7 +313,7 @@ def test_foreground_background_settings():
     image1 = app.new_data_viewer(ImageWidget)
     image1.add_data(d_2d)
 
-    histogram1 = app.new_data_viewer(HistogramWidget)
+    histogram1 = app.new_data_viewer(HistogramViewer)
     histogram1.add_data(d_1d)
 
     dendrogram1 = app.new_data_viewer(DendroWidget)
@@ -360,7 +360,7 @@ def test_foreground_background_settings():
         image2 = app.new_data_viewer(ImageWidget)
         image2.add_data(d_2d)
 
-        histogram2 = app.new_data_viewer(HistogramWidget)
+        histogram2 = app.new_data_viewer(HistogramViewer)
         histogram2.add_data(d_1d)
 
         dendrogram2 = app.new_data_viewer(DendroWidget)

glue/core/data.py

@@ -698,19 +698,34 @@ def broadcast(self, attribute):
 
     @contract(old=ComponentID, new=ComponentID)
     def update_id(self, old, new):
-        """Reassign a component to a different :class:`glue.core.component_id.ComponentID`
+        """
+        Reassign a component to a different :class:`glue.core.component_id.ComponentID`
 
-        :param old: The old :class:`glue.core.component_id.ComponentID`.
-        :param new: The new :class:`glue.core.component_id.ComponentID`.
+        Parameters
+        ----------
+        old : :class:`glue.core.component_id.ComponentID`
+            The old component ID
+        new : :class:`glue.core.component_id.ComponentID`
+            The new component ID
         """
 
         if new is old:
             return
 
+        if new.parent is None:
+            new.parent = self
+
         changed = False
         if old in self._components:
-            self._components[new] = self._components[old]
+
+            # We want to keep the original order, so we can't just do:
+            #   self._components[new] = self._components[old]
+            # which will put the new component ID at the end, but instead
+            # we need to do:
+            self._components = OrderedDict((new, value) if key is old else (key, value)
+                                           for key, value in self._components.items())
             changed = True
+
         try:
             index = self._pixel_component_ids.index(old)
             self._pixel_component_ids[index] = new
@@ -730,7 +745,6 @@ def update_id(self, old, new):
 
             # remove old component and broadcast the change
             # see #508 for discussion of this
-            self._components.pop(old)
             msg = ComponentReplacedMessage(self, old, new)
             self.hub.broadcast(msg)
 

glue/core/qt/data_combo_helper.py

@@ -7,7 +7,8 @@
 from glue.core.message import (ComponentsChangedMessage,
                                DataCollectionAddMessage,
                                DataCollectionDeleteMessage,
-                               DataUpdateMessage)
+                               DataUpdateMessage,
+                               ComponentReplacedMessage)
 from glue.utils import nonpartial
 from glue.utils.qt import update_combobox
 from glue.utils.qt.widget_properties import CurrentComboDataProperty
@@ -106,8 +107,9 @@ def append_data(self, data, refresh=True):
                 self.refresh()
 
     def remove_data(self, data):
-        self._data.remove(data)
-        self.refresh()
+        if data in self._data:
+            self._data.remove(data)
+            self.refresh()
 
     def set_multiple_data(self, datasets):
         """
@@ -180,6 +182,9 @@ def refresh(self):
                     break
 
     def register_to_hub(self, hub):
+        hub.subscribe(self, ComponentReplacedMessage,
+                      handler=nonpartial(self.refresh),
+                      filter=lambda msg: msg.data in self._data)
         hub.subscribe(self, ComponentsChangedMessage,
                       handler=nonpartial(self.refresh),
                       filter=lambda msg: msg.data in self._data)

glue/core/qt/layer_artist_model.py

@@ -11,6 +11,8 @@
 
 from __future__ import absolute_import, division, print_function
 
+from weakref import WeakKeyDictionary
+
 from qtpy.QtCore import Qt
 from qtpy import QtCore, QtWidgets, QtGui
 from glue.core.layer_artist import LayerArtistBase, LayerArtistContainer
@@ -301,7 +303,7 @@ def __init__(self, parent=None, layer_style_widget_cls=None):
 
         self.setLayout(self.layout)
 
-        self.layout_style_widgets = {}
+        self.layout_style_widgets = WeakKeyDictionary()
 
         self.empty = QtWidgets.QWidget()
         self.layer_options_layout.addWidget(self.empty)

glue/core/qt/tests/test_data_combo_helper.py

@@ -1,17 +1,16 @@
 from __future__ import absolute_import, division, print_function
 
+from mock import MagicMock
+
 from glue.core import Data, DataCollection
+from glue.core.component_id import ComponentID
 from qtpy import QtWidgets
+from glue.utils.qt import combo_as_string
 
 from ..data_combo_helper import (ComponentIDComboHelper, ManualDataComboHelper,
                                  DataCollectionComboHelper)
 
 
-def _items_as_string(combo):
-    items = [combo.itemText(i) for i in range(combo.count())]
-    return ":".join(items)
-
-
 def test_component_id_combo_helper():
 
     combo = QtWidgets.QComboBox()
@@ -20,48 +19,49 @@ def test_component_id_combo_helper():
 
     helper = ComponentIDComboHelper(combo, dc)
 
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""
 
     data1 = Data(x=[1, 2, 3], y=[2, 3, 4], label='data1')
 
     dc.append(data1)
     helper.append_data(data1)
 
-    assert _items_as_string(combo) == "x:y"
+    assert combo_as_string(combo) == "x:y"
 
     data2 = Data(a=[1, 2, 3], b=['a', 'b', 'c'], label='data2')
 
     dc.append(data2)
     helper.append_data(data2)
 
-    assert _items_as_string(combo) == "data1:x:y:data2:a:b"
+    assert combo_as_string(combo) == "data1:x:y:data2:a:b"
 
     helper.categorical = False
 
-    assert _items_as_string(combo) == "data1:x:y:data2:a"
+    assert combo_as_string(combo) == "data1:x:y:data2:a"
 
     helper.numeric = False
 
-    assert _items_as_string(combo) == "data1:data2"
+    assert combo_as_string(combo) == "data1:data2"
 
     helper.categorical = True
     helper.numeric = True
 
     helper.visible = False
-    assert _items_as_string(combo) == "data1:x:Pixel Axis 0 [x]:World 0:y:data2:a:Pixel Axis 0 [x]:World 0:b"
+    assert combo_as_string(combo) == "data1:x:Pixel Axis 0 [x]:World 0:y:data2:a:Pixel Axis 0 [x]:World 0:b"
     helper.visible = True
 
     dc.remove(data2)
 
-    assert _items_as_string(combo) == "x:y"
+    assert combo_as_string(combo) == "x:y"
 
     # TODO: check that renaming a component updates the combo
     # data1.id['x'].label = 'z'
-    # assert _items_as_string(combo) == "z:y"
+    # assert combo_as_string(combo) == "z:y"
 
     helper.remove_data(data1)
 
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""
+
 
 def test_component_id_combo_helper_init():
 
@@ -77,19 +77,58 @@ def test_component_id_combo_helper_init():
 
     helper = ComponentIDComboHelper(combo, dc)
     helper.append_data(data)
-    assert _items_as_string(combo) == "a:b"
+    assert combo_as_string(combo) == "a:b"
 
     helper = ComponentIDComboHelper(combo, dc, numeric=False)
     helper.append_data(data)
-    assert _items_as_string(combo) == "b"
+    assert combo_as_string(combo) == "b"
 
     helper = ComponentIDComboHelper(combo, dc, categorical=False)
     helper.append_data(data)
-    assert _items_as_string(combo) == "a"
+    assert combo_as_string(combo) == "a"
 
     helper = ComponentIDComboHelper(combo, dc, numeric=False, categorical=False)
     helper.append_data(data)
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""
+
+
+def test_component_id_combo_helper_replaced():
+
+    # Make sure that when components are replaced, the equivalent combo index
+    # remains selected and an event is broadcast so that any attached callback
+    # properties can be sure to pull the latest text/userData.
+
+    callback = MagicMock()
+
+    combo = QtWidgets.QComboBox()
+    combo.currentIndexChanged.connect(callback)
+
+    dc = DataCollection([])
+
+    helper = ComponentIDComboHelper(combo, dc)
+
+    assert combo_as_string(combo) == ""
+
+    data1 = Data(x=[1, 2, 3], y=[2, 3, 4], label='data1')
+
+    callback.reset_mock()
+
+    dc.append(data1)
+    helper.append_data(data1)
+
+    callback.assert_called_once_with(0)
+    callback.reset_mock()
+
+    assert combo_as_string(combo) == "x:y"
+
+    new_id = ComponentID(label='new')
+
+    data1.update_id(data1.id['x'], new_id)
+
+    callback.assert_called_once_with(0)
+    callback.reset_mock()
+
+    assert combo_as_string(combo) == "new:y"
 
 
 def test_manual_data_combo_helper():
@@ -104,18 +143,18 @@ def test_manual_data_combo_helper():
 
     dc.append(data1)
 
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""
 
     helper.append_data(data1)
 
-    assert _items_as_string(combo) == "data1"
+    assert combo_as_string(combo) == "data1"
 
     data1.label = 'mydata1'
-    assert _items_as_string(combo) == "mydata1"
+    assert combo_as_string(combo) == "mydata1"
 
     dc.remove(data1)
 
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""
 
 
 def test_data_collection_combo_helper():
@@ -130,11 +169,11 @@ def test_data_collection_combo_helper():
 
     dc.append(data1)
 
-    assert _items_as_string(combo) == "data1"
+    assert combo_as_string(combo) == "data1"
 
     data1.label = 'mydata1'
-    assert _items_as_string(combo) == "mydata1"
+    assert combo_as_string(combo) == "mydata1"
 
     dc.remove(data1)
 
-    assert _items_as_string(combo) == ""
+    assert combo_as_string(combo) == ""