Merge pull request #56 from astrofrog/fix-remove-layer

Fix removal of layers

CHANGES.rst

@@ -3,6 +3,8 @@
 
 - Fixed issues with layer visibility. [#52]
 
+- Fixed bug that caused remonving layers to not work. [#56]
+
 - Fixed issue with viewer options not being set correctly when loading from a session. [#52]
 
 - Fixed compatibility with the latest developer version of glue. [#52]

glue_wwt/viewer/layer_artist.py

@@ -24,6 +24,7 @@ RESET_DATA_PROPERTIES = ('mode', 'frame', 'lon_att', 'lat_att', 'alt_att',
 class WWTLayer(LayerArtist):
 
     _layer_state_cls = WWTLayerState
+    _removed = False
 
     def __init__(self, wwt_client, viewer_state, layer_state=None, layer=None):
 
@@ -49,8 +50,20 @@ class WWTLayer(LayerArtist):
             self.wwt_layer = None
             self._coords = [], []
 
+    def remove(self):
+        self._removed = True
+        if self.wwt_layer is not None:
+            self.wwt_layer.remove()
+            self.wwt_layer = None
+            self._coords = [], []
+
     def _update_markers(self, force=False, **kwargs):
 
+        if self._removed:
+            return
+
+        changed = set() if force else self.pop_changed_properties()
+
         if self._viewer_state.lon_att is None or self._viewer_state.lat_att is None:
             if self.wwt_layer is not None:
                 self.wwt_layer.remove()
@@ -65,7 +78,7 @@ class WWTLayer(LayerArtist):
                 self.wwt_layer = None
             return
 
-        if force or 'mode' in kwargs or self.wwt_layer is None:
+        if force or 'mode' in changed or self.wwt_layer is None:
             if self.wwt_layer is not None:
                 self.wwt_layer.remove()
                 self.wwt_layer = None
@@ -77,7 +90,7 @@ class WWTLayer(LayerArtist):
         # for now we set unit to pc and scale values accordingly, so if the
         # unit changes we need to refresh the data just in case
 
-        if force or any(x in kwargs for x in RESET_DATA_PROPERTIES):
+        if force or any(x in changed for x in RESET_DATA_PROPERTIES):
 
             try:
                 lon = self.layer[self._viewer_state.lon_att]
@@ -182,7 +195,7 @@ class WWTLayer(LayerArtist):
             else:
                 return
 
-        if force or 'alt_unit' in kwargs:
+        if force or 'alt_unit' in changed:
             # FIXME: kpc isn't yet a valid unit in WWT/PyWWT:
             # https://github.com/WorldWideTelescope/wwt-web-client/pull/197
             # for now we set unit to pc and scale values accordingly
@@ -191,34 +204,34 @@ class WWTLayer(LayerArtist):
             else:
                 self.wwt_layer.alt_unit = self._viewer_state.alt_unit
 
-        if force or 'alt_type' in kwargs:
+        if force or 'alt_type' in changed:
             self.wwt_layer.alt_type = self._viewer_state.alt_type.lower()
 
-        if force or 'size' in kwargs or 'size_mode' in kwargs or 'size_scaling' in kwargs:
+        if force or 'size' in changed or 'size_mode' in changed or 'size_scaling' in changed:
             if self.state.size_mode == 'Linear':
                 self.wwt_layer.size_scale = self.state.size_scaling
             else:
                 self.wwt_layer.size_scale = self.state.size * 5 * self.state.size_scaling
 
-        if force or 'color' in kwargs:
+        if force or 'color' in changed:
             self.wwt_layer.color = self.state.color
 
-        if force or 'alpha' in kwargs:
+        if force or 'alpha' in changed:
             self.wwt_layer.opacity = self.state.alpha
 
-        if force or 'size_vmin' in kwargs:
+        if force or 'size_vmin' in changed:
             self.wwt_layer.size_vmin = self.state.size_vmin
 
-        if force or 'size_vmax' in kwargs:
+        if force or 'size_vmax' in changed:
             self.wwt_layer.size_vmax = self.state.size_vmax
 
-        if force or 'cmap_vmin' in kwargs:
+        if force or 'cmap_vmin' in changed:
             self.wwt_layer.cmap_vmin = self.state.cmap_vmin
 
-        if force or 'cmap_vmax' in kwargs:
+        if force or 'cmap_vmax' in changed:
             self.wwt_layer.cmap_vmax = self.state.cmap_vmax
 
-        if force or 'cmap' in kwargs:
+        if force or 'cmap' in changed:
             self.wwt_layer.cmap = self.state.cmap
 
         self.enable()

glue_wwt/viewer/tests/test_wwt_widget.py

@@ -111,6 +111,25 @@ class TestWWTDataViewer(object):
         self.viewer.state.alt_unit = 'kpc'
         self.viewer.state.alt_att = None
 
+    def test_remove_layer(self):
+
+        # Make sure that _update_markers doesn't get called after removing a
+        # layer. This is a regression test for
+        # https://github.com/glue-viz/glue-wwt/issues/54
+
+        self.register()
+        self.d.add_subset(self.d.id['x'] > 1)
+        self.viewer.add_data(self.d)
+        assert len(self.viewer.layers) == 2
+
+        subset_layer = self.viewer.layers[1]
+
+        subset_layer.wwt_client.layers.add_data_layer = MagicMock()
+
+        self.viewer.remove_subset(self.d.subsets[0])
+        assert len(self.viewer.layers) == 1
+        assert subset_layer.wwt_client.layers.add_data_layer.call_count == 0
+        assert subset_layer.wwt_layer is None
 
     # TODO: determine if the following test is the desired behavior
     # def test_subsets_not_live_added_if_data_not_present(self):