From 201a68670cfe30b1cc9475000a8c903fe211b506 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Fri, 16 Oct 2015 17:04:57 +0200 Subject: [PATCH 01/12] Make sure image viewer updates when changing component --- glue/clients/image_client.py | 6 +++++- glue/qt/widgets/image_widget.py | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/glue/clients/image_client.py b/glue/clients/image_client.py index 02266fa8c..9f6bd0eed 100644 --- a/glue/clients/image_client.py +++ b/glue/clients/image_client.py @@ -475,10 +475,14 @@ 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() + def _update_attribute(self): + self._update_data_plot() + self._redraw() + def add_layer(self, layer): if layer in self.artists: return self.artists[layer][0] diff --git a/glue/qt/widgets/image_widget.py b/glue/qt/widgets/image_widget.py index a242f096f..eda97ad8e 100644 --- a/glue/qt/widgets/image_widget.py +++ b/glue/qt/widgets/image_widget.py @@ -220,7 +220,8 @@ def _connect(self): # 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_attribute', nonpartial(self.client._update_attribute)) + 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) From cf09bac3a02d7c84a96ad0f0dbad4807c182536f Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 12:06:42 +0200 Subject: [PATCH 02/12] =?UTF-8?q?Always=20emit=20currentIndexChanged=20eve?= =?UTF-8?q?nt,=20even=20if=20index=20hasn=E2=80=99t=20changed,=20when=20up?= =?UTF-8?q?dating=20contents=20of=20combo=20box?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- glue/qt/qtutil.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/glue/qt/qtutil.py b/glue/qt/qtutil.py index fd4f5173a..6f55eeba3 100644 --- a/glue/qt/qtutil.py +++ b/glue/qt/qtutil.py @@ -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: + combo.currentIndexChanged.emit(index) + return combo From 88c7c11396473da1149de64a5e04833d4503df8b Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 12:26:11 +0200 Subject: [PATCH 03/12] Fix the scenario in which we add a dataset to an image viewer, then another dataset, and switch back to the first. --- glue/clients/image_client.py | 13 ++++++------- glue/qt/widget_properties.py | 2 +- glue/qt/widgets/image_widget.py | 17 ++++++++++++++--- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/glue/clients/image_client.py b/glue/clients/image_client.py index 9f6bd0eed..86e855efa 100644 --- a/glue/clients/image_client.py +++ b/glue/clients/image_client.py @@ -479,10 +479,6 @@ def _update_aspect(self): self._update_data_plot(relim=True) self._redraw() - def _update_attribute(self): - self._update_data_plot() - self._redraw() - def add_layer(self, layer): if layer in self.artists: return self.artists[layer][0] @@ -755,11 +751,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) diff --git a/glue/qt/widget_properties.py b/glue/qt/widget_properties.py index 0a60d9d6b..b1991e0b6 100644 --- a/glue/qt/widget_properties.py +++ b/glue/qt/widget_properties.py @@ -237,7 +237,7 @@ def connect_current_combo(client, prop, widget): """ def _push_combo(value): - idx = widget.findData(value) + idx = _find_combo_data(widget, value) if idx == -1: return widget.setCurrentIndex(idx) diff --git a/glue/qt/widgets/image_widget.py b/glue/qt/widgets/image_widget.py index eda97ad8e..dad9c48be 100644 --- a/glue/qt/widgets/image_widget.py +++ b/glue/qt/widgets/image_widget.py @@ -138,6 +138,7 @@ 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) + self.client.display_data = data self.set_attribute_combo(self.client.display_data) return r is not None @@ -218,9 +219,8 @@ 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_attribute', nonpartial(self.client._update_attribute)) + 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 @@ -228,6 +228,17 @@ def _connect(self): 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, *args): + 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, *args): + self.client._update_and_redraw() + self.update_window_title() + @defer_draw def _update_rgb_console(self, is_monochrome): if is_monochrome: From 557e77b69830364b6da4223aaf1efb3996f51da1 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 10:59:15 +0200 Subject: [PATCH 04/12] Use _find_combo_data instead of findData, because PySide crashes if the data is not a string --- glue/qt/widget_properties.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glue/qt/widget_properties.py b/glue/qt/widget_properties.py index b1991e0b6..d614d1dff 100644 --- a/glue/qt/widget_properties.py +++ b/glue/qt/widget_properties.py @@ -237,6 +237,8 @@ def connect_current_combo(client, prop, widget): """ def _push_combo(value): + # NOTE: we can't use findData here because if the data is not a + # string, PySide will crash idx = _find_combo_data(widget, value) if idx == -1: return From 901881b1707888a92b3ab56da61e2e5e671f7978 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 11:15:38 +0200 Subject: [PATCH 05/12] Fix test_connect_current_combo --- glue/qt/tests/test_widget_properties.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/glue/qt/tests/test_widget_properties.py b/glue/qt/tests/test_widget_properties.py index 9b8b4738e..dd132737c 100644 --- a/glue/qt/tests/test_widget_properties.py +++ b/glue/qt/tests/test_widget_properties.py @@ -235,8 +235,9 @@ 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' def test_connect_float_edit(): From 606c699486306f47ac8a8b8f74732f32a5de2647 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 16:19:47 +0200 Subject: [PATCH 06/12] Fix all test failures --- glue/clients/image_client.py | 9 +++++++-- glue/plugins/ginga_viewer/client.py | 3 +++ glue/qt/widget_properties.py | 10 +++++++--- glue/qt/widgets/image_widget.py | 14 ++++++++++++-- glue/qt/widgets/tests/test_image_widget.py | 2 +- 5 files changed, 30 insertions(+), 8 deletions(-) diff --git a/glue/clients/image_client.py b/glue/clients/image_client.py index 86e855efa..263e45cc1 100644 --- a/glue/clients/image_client.py +++ b/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() diff --git a/glue/plugins/ginga_viewer/client.py b/glue/plugins/ginga_viewer/client.py index 62c9006ff..589091909 100644 --- a/glue/plugins/ginga_viewer/client.py +++ b/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) diff --git a/glue/qt/widget_properties.py b/glue/qt/widget_properties.py index d614d1dff..3437c28a1 100644 --- a/glue/qt/widget_properties.py +++ b/glue/qt/widget_properties.py @@ -239,9 +239,13 @@ def connect_current_combo(client, prop, widget): def _push_combo(value): # NOTE: we can't use findData here because if the data is not a # string, PySide will crash - idx = _find_combo_data(widget, value) - if idx == -1: - return + try: + idx = _find_combo_data(widget, value) + except ValueError: + if value is None: + idx = -1 + else: + raise widget.setCurrentIndex(idx) def _pull_combo(idx): diff --git a/glue/qt/widgets/image_widget.py b/glue/qt/widgets/image_widget.py index dad9c48be..ed352d0a3 100644 --- a/glue/qt/widgets/image_widget.py +++ b/glue/qt/widgets/image_widget.py @@ -13,7 +13,7 @@ from ...clients.ds9norm import DS9Normalize from ...external.modest_image import imshow -from ...clients.layer_artist import Pointer +from ...clients.layer_artist import Pointer, ImageLayerArtist from ...core.callback_property import add_callback, delay_callback from .data_slice_widget import DataSlice @@ -138,7 +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) - self.client.display_data = 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 @@ -229,6 +230,11 @@ def _connect(self): connect_current_combo(self.client, 'display_aspect', self.ui.aspectCombo) def _display_data_changed(self, *args): + + 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) @@ -236,6 +242,10 @@ def _display_data_changed(self, *args): self.update_window_title() def _display_attribute_changed(self, *args): + + if self.client.display_attribute is None: + return + self.client._update_and_redraw() self.update_window_title() diff --git a/glue/qt/widgets/tests/test_image_widget.py b/glue/qt/widgets/tests/test_image_widget.py index 9b5fdbf14..6bb440a68 100644 --- a/glue/qt/widgets/tests/test_image_widget.py +++ b/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() From 98860903883acf84378db1995b673aed3b9a5194 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 16:24:27 +0200 Subject: [PATCH 07/12] Remove unused import --- glue/qt/widgets/image_widget.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glue/qt/widgets/image_widget.py b/glue/qt/widgets/image_widget.py index ed352d0a3..955abed10 100644 --- a/glue/qt/widgets/image_widget.py +++ b/glue/qt/widgets/image_widget.py @@ -13,7 +13,7 @@ from ...clients.ds9norm import DS9Normalize from ...external.modest_image import imshow -from ...clients.layer_artist import Pointer, ImageLayerArtist +from ...clients.layer_artist import Pointer from ...core.callback_property import add_callback, delay_callback from .data_slice_widget import DataSlice From bbb1ffcbcc925b1d5a05ff54b3ac08d22f71d301 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 16:42:35 +0200 Subject: [PATCH 08/12] Added tests for update_combobox --- glue/qt/qtutil.py | 4 ++-- glue/qt/tests/test_qtutil.py | 40 +++++++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/glue/qt/qtutil.py b/glue/qt/qtutil.py index 6f55eeba3..df63587e7 100644 --- a/glue/qt/qtutil.py +++ b/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 @@ -997,7 +997,7 @@ def update_combobox(combo, labeldata): # 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: + if idx == index or idx == -1: combo.currentIndexChanged.emit(index) return combo diff --git a/glue/qt/tests/test_qtutil.py b/glue/qt/tests/test_qtutil.py index 6dd8ccf83..8cf499366 100644 --- a/glue/qt/tests/test_qtutil.py +++ b/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 From 88fb1b1623e62108e8ac2fd04f711683ed5425aa Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 16:50:30 +0200 Subject: [PATCH 09/12] Deal with None values in combo boxes --- glue/qt/tests/test_widget_properties.py | 7 +++++++ glue/qt/widget_properties.py | 18 ++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/glue/qt/tests/test_widget_properties.py b/glue/qt/tests/test_widget_properties.py index dd132737c..b1742635c 100644 --- a/glue/qt/tests/test_widget_properties.py +++ b/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(): @@ -239,6 +243,9 @@ class Test(object): 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(): diff --git a/glue/qt/widget_properties.py b/glue/qt/widget_properties.py index 3437c28a1..1cdaafcc0 100644 --- a/glue/qt/widget_properties.py +++ b/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) From b5ad1465565d5c497762bf892daeddf70e71111f Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 17:01:48 +0200 Subject: [PATCH 10/12] Added regression test for bug with combo boxes not updating --- glue/qt/widgets/tests/test_image_widget.py | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/glue/qt/widgets/tests/test_image_widget.py b/glue/qt/widgets/tests/test_image_widget.py index 6bb440a68..86c8f14d7 100644 --- a/glue/qt/widgets/tests/test_image_widget.py +++ b/glue/qt/widgets/tests/test_image_widget.py @@ -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 From 945dd559cffd68a419c57361da4fc40427a0a0c4 Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 17:05:05 +0200 Subject: [PATCH 11/12] Added changelog entry --- CHANGES.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 289111411..f8064e356 100644 --- a/CHANGES.md +++ b/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] From b3e351c2d1cc7bf51a8e527e42b3de494ad809cf Mon Sep 17 00:00:00 2001 From: Thomas Robitaille Date: Thu, 22 Oct 2015 18:08:07 +0200 Subject: [PATCH 12/12] Remove unnecessary *args --- glue/qt/widgets/image_widget.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glue/qt/widgets/image_widget.py b/glue/qt/widgets/image_widget.py index 955abed10..54c5ef2ca 100644 --- a/glue/qt/widgets/image_widget.py +++ b/glue/qt/widgets/image_widget.py @@ -229,7 +229,7 @@ def _connect(self): 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, *args): + def _display_data_changed(self): if self.client.display_data is None: self.ui.attributeComboBox.clear() @@ -241,7 +241,7 @@ def _display_data_changed(self, *args): self.client._update_and_redraw() self.update_window_title() - def _display_attribute_changed(self, *args): + def _display_attribute_changed(self): if self.client.display_attribute is None: return