Merge pull request #755 from astrofrog/fix-image-update-attribute

Bug when selecting attribute in RGB image

CHANGES.md

@@ -7,6 +7,11 @@ v0.6 (unreleased)
 * Prevent axes from moving around when data viewers are being resized, and
   instead make the absolute margins between axes and figure edge fixed. [#745]
 
+* Fixed a bug that caused image plots to not be updated immediately when
+  changing component, and fixed a bug that caused data and attribute combo
+  boxes to not update correctly when showing multiple datasets in an
+  ImageWidget. [#755]
+
 * Added tests to ensure that we remain backward-compatible with old session
   files for the FITS and HDF5 factories. [#736, #748]
 

glue/clients/image_client.py

@@ -412,10 +412,15 @@ def delete_layer(self, layer):
                 self.delete_layer(subset)
 
         if layer is self.display_data:
-            if len(self.artists) > 0:
-                self.display_data = self.artists.layers[0].data
+            for layer in self.artists:
+                if isinstance(layer, ImageLayerArtist):
+                    self.display_data = layer.data
+                    break
             else:
+                for artist in self.artists:
+                    self.delete_layer(artist.layer)
                 self.display_data = None
+                self.display_attribute = None
 
         self._redraw()
 
@@ -475,7 +480,7 @@ def rgb_mode(self, enable=None):
         self._redraw()
         return result
 
-    def update_aspect(self):
+    def _update_aspect(self):
         self._update_data_plot(relim=True)
         self._redraw()
 
@@ -751,11 +756,14 @@ def check_update(self, *args):
         vw = _view_window(self._axes)
         if vw != self._view_window:
             logging.getLogger(__name__).debug("updating")
-            self._update_data_plot()
-            self._update_subset_plots()
-            self._redraw()
+            self._update_and_redraw()
             self._view_window = vw
 
+    def _update_and_redraw(self):
+        self._update_data_plot()
+        self._update_subset_plots()
+        self._redraw()
+
     @requires_data
     def _update_axis_labels(self):
         labels = _axis_labels(self.display_data, self.slice)

glue/plugins/ginga_viewer/client.py

@@ -50,6 +50,9 @@ def _new_scatter_layer(self, layer):
     def _update_axis_labels(self):
         pass
 
+    def _update_and_redraw(self):
+        pass
+
     def set_cmap(self, cmap):
         self._canvas.set_cmap(cmap)
 

glue/qt/qtutil.py

@@ -980,7 +980,7 @@ def update_combobox(combo, labeldata):
     """
     combo.blockSignals(True)
     idx = combo.currentIndex()
-    if idx > 0:
+    if idx >= 0:
         current = combo.itemData(idx)
     else:
         current = None
@@ -994,6 +994,12 @@ def update_combobox(combo, labeldata):
     combo.blockSignals(False)
     combo.setCurrentIndex(index)
 
+    # We need to force emit this, otherwise if the index happens to be the
+    # same as before, even if the data is different, callbacks won't be
+    # called.
+    if idx == index or idx == -1:
+        combo.currentIndexChanged.emit(index)
+
     return combo
 
 

glue/qt/tests/test_qtutil.py

@@ -8,7 +8,7 @@
 from ...external.qt.QtCore import Qt
 from mock import MagicMock, patch
 from ..qtutil import GlueDataDialog
-from ..qtutil import pretty_number, GlueComboBox, PythonListModel
+from ..qtutil import pretty_number, GlueComboBox, PythonListModel, update_combobox
 
 from glue.config import data_factory
 from glue.core import Subset
@@ -452,3 +452,41 @@ def test_insert(self):
     def test_iter(self):
         m = PythonListModel([1, 2, 3])
         assert list(m) == [1, 2, 3]
+
+
+
+def test_update_combobox():
+    combo = GlueComboBox()
+    update_combobox(combo, [('a', 1), ('b', 2)])
+    update_combobox(combo, [('c', 3)])
+
+
+def test_update_combobox_indexchanged():
+    # Regression test for bug that caused currentIndexChanged to not be
+    # emitted if the new index happened to be the same as the old one but the
+    # label data was different.
+
+    class MyComboBox(GlueComboBox):
+
+        def __init__(self, *args, **kwargs):
+            self.change_count = 0
+            super(MyComboBox, self).__init__(*args, **kwargs)
+            self.currentIndexChanged.connect(self.changed)
+
+        def changed(self):
+            self.change_count += 1
+
+    combo = MyComboBox()
+    update_combobox(combo, [('a', 1), ('b', 2)])
+    update_combobox(combo, [('c', 3)])
+
+    assert combo.change_count == 2
+    assert combo.currentIndex() == 0
+
+    combo = MyComboBox()
+    update_combobox(combo, [('a', 1), ('b', 2)])
+    update_combobox(combo, [('a', 1), ('b', 3)])
+    update_combobox(combo, [('a', 3), ('b', 1)])
+
+    assert combo.change_count == 3
+    assert combo.currentIndex() == 1

glue/qt/tests/test_widget_properties.py

@@ -76,6 +76,10 @@ def __init__(self):
         tc.co = 'c'
     assert exc.value.args[0] == "Cannot find text 'c' in combo box"
 
+    tc.co = None
+    assert tc.co == None
+    assert tc._combo.currentIndex() == -1
+
 
 def test_text():
 
@@ -235,8 +239,12 @@ class Test(object):
     assert combo.currentIndex() == 0
 
     # TODO: should the following not return an error?
-    t.a = 'c'
-    assert combo.currentIndex() == 0
+    with pytest.raises(ValueError) as exc:
+        t.a = 'c'
+    assert exc.value.args[0] == 'c not found in combo box'
+
+    t.a = None
+    assert combo.currentIndex() == -1
 
 
 def test_connect_float_edit():

glue/qt/widget_properties.py

@@ -76,7 +76,10 @@ def getter(self, widget):
         """
         Return the itemData stored in the currently-selected item
         """
-        return widget.itemData(widget.currentIndex())
+        if widget.currentIndex() == -1:
+            return None
+        else:
+            return widget.itemData(widget.currentIndex())
 
     def setter(self, widget, value):
         """
@@ -88,7 +91,10 @@ def setter(self, widget, value):
         try:
             idx = _find_combo_data(widget, value)
         except ValueError:
-            raise ValueError("Cannot find data '{0}' in combo box".format(value))
+            if value is None:
+                idx = -1
+            else:            
+                raise ValueError("Cannot find data '{0}' in combo box".format(value))
         widget.setCurrentIndex(idx)
 
 CurrentComboProperty = CurrentComboDataProperty
@@ -103,7 +109,10 @@ def getter(self, widget):
         """
         Return the itemData stored in the currently-selected item
         """
-        return widget.itemText(widget.currentIndex())
+        if widget.currentIndex() == -1:
+            return None
+        else:
+            return widget.itemText(widget.currentIndex())
 
     def setter(self, widget, value):
         """
@@ -112,7 +121,8 @@ def setter(self, widget, value):
         """
         idx = widget.findText(value)
         if idx == -1:
-            raise ValueError("Cannot find text '{0}' in combo box".format(value))
+            if value is not None:
+                raise ValueError("Cannot find text '{0}' in combo box".format(value))
         widget.setCurrentIndex(idx)
 
 
@@ -237,9 +247,15 @@ def connect_current_combo(client, prop, widget):
     """
 
     def _push_combo(value):
-        idx = widget.findData(value)
-        if idx == -1:
-            return
+        # NOTE: we can't use findData here because if the data is not a 
+        # string, PySide will crash
+        try:
+            idx = _find_combo_data(widget, value)
+        except ValueError:
+            if value is None:
+                idx = -1
+            else:
+                raise
         widget.setCurrentIndex(idx)
 
     def _pull_combo(idx):

glue/qt/widgets/image_widget.py

@@ -138,6 +138,8 @@ def add_data(self, data):
             r = self.client.add_layer(data)
             if r is not None and self.client.display_data is not None:
                 self.add_data_to_combo(data)
+                if self.client.can_image_data(data):
+                    self.client.display_data = data
                 self.set_attribute_combo(self.client.display_data)
 
         return r is not None
@@ -218,15 +220,35 @@ def _connect(self):
         add_callback(self.client, 'display_data', self.ui.slice.set_data)
 
         # sync window title to data/attribute
-        add_callback(self.client, 'display_data', nonpartial(self.update_window_title))
-        add_callback(self.client, 'display_attribute', nonpartial(self.update_window_title))
-        add_callback(self.client, 'display_aspect', nonpartial(self.client.update_aspect))
+        add_callback(self.client, 'display_data', nonpartial(self._display_data_changed))
+        add_callback(self.client, 'display_attribute', nonpartial(self._display_attribute_changed))
+        add_callback(self.client, 'display_aspect', nonpartial(self.client._update_aspect))
 
         # sync data/attribute combos with client properties
         connect_current_combo(self.client, 'display_data', self.ui.displayDataCombo)
         connect_current_combo(self.client, 'display_attribute', self.ui.attributeComboBox)
         connect_current_combo(self.client, 'display_aspect', self.ui.aspectCombo)
 
+    def _display_data_changed(self):
+
+        if self.client.display_data is None:
+            self.ui.attributeComboBox.clear()
+            return
+
+        with self.client.artists.ignore_empty():
+            self.set_attribute_combo(self.client.display_data)
+        self.client.add_layer(self.client.display_data)
+        self.client._update_and_redraw()
+        self.update_window_title()
+
+    def _display_attribute_changed(self):
+
+        if self.client.display_attribute is None:
+            return
+
+        self.client._update_and_redraw()
+        self.update_window_title()
+
     @defer_draw
     def _update_rgb_console(self, is_monochrome):
         if is_monochrome:

glue/qt/widgets/tests/test_image_widget.py

@@ -188,7 +188,7 @@ def test_resize(self):
         # What appears to happen when the test fails is that the QTimer gets
         # started but basically never ends up triggering the timeout.
 
-        large = core.Data(label='im', x=np.random.random((1024, 1024)))
+        large = core.Data(label='largeim', x=np.random.random((1024, 1024)))
         self.collect.append(large)
 
         app = get_qapp()
@@ -295,3 +295,58 @@ def test_rgb_layer(self):
         assert w.ratt.label == 'x'
         assert w.gatt.label == 'y'
         assert w.batt.label == 'x'
+
+
+def test_combo_box_updates():
+
+    # Regression test for a bug that caused combo boxes to not be updated
+    # correctly when switching between different datasets.
+
+    session = simple_session()
+    hub = session.hub
+    dc = session.data_collection
+
+    data1 = core.Data(label='im1',
+                        x=[[1, 2], [3, 4]],
+                        y=[[2, 3], [4, 5]])
+
+    data2 = core.Data(label='im2',
+                        a=[[1, 2], [3, 4]],
+                        b=[[2, 3], [4, 5]])
+
+    dc.append(data1)
+    dc.append(data2)
+
+    widget = ImageWidget(session)
+    widget.register_to_hub(hub)
+
+    widget.add_data(data1)
+
+    assert widget.client.display_data is data1
+
+    assert widget.data.label == 'im1'
+    assert widget.attribute.label == 'x'
+
+    widget.add_data(data2)
+
+    assert widget.client.display_data is data2
+
+    assert widget.data.label == 'im2'
+    assert widget.attribute.label == 'a'
+
+    widget.attribute = data2.find_component_id('b')
+
+    with pytest.raises(ValueError) as exc:
+        widget.attribute =  data1.find_component_id('x')
+    assert exc.value.args[0] == "Cannot find data 'x' in combo box"
+
+    widget.data = data1
+    assert widget.attribute.label == 'x'
+
+    widget.attribute = data1.find_component_id('y')
+
+    with pytest.raises(ValueError) as exc:
+        widget.attribute =  data2.find_component_id('a')
+    assert exc.value.args[0] == "Cannot find data 'a' in combo box"
+
+    assert widget.client.display_data is data1