Merge pull request #82 from mwcraig/refactor-marking

Implement the remaining API for the ginga viewer

astrowidgets/core.py

@@ -72,8 +72,6 @@ def __init__(self, logger=None, image_width=500, image_height=500,
                 warnings.warn('install opencv or set use_opencv=False')
 
         self._viewer = EnhancedCanvasView(logger=logger)
-        self._is_marking = False
-        self._click_center = False
 
         self._pixel_offset = pixel_coords_offset
 
@@ -112,6 +110,24 @@ def __init__(self, logger=None, image_width=500, image_height=500,
         self.canvas.enable_draw(True)
         self.canvas.enable_edit(True)
 
+        # Make sure all of the internal state trackers have a value
+        # and start in a state which is definitely allowed: all are
+        # False.
+        self._is_marking = False
+        self._click_center = False
+        self._click_drag = False
+        self._scroll_pan = False
+
+        # Set a couple of things to match the ginga defaults
+        self.scroll_pan = True
+        self.click_drag = False
+
+        bind_map = self._viewer.get_bindmap()
+        # Set up right-click and drag adjusts the contrast
+        bind_map.map_event(None, (), 'ms_right', 'contrast')
+        # Shift-right-click restores the default contrast
+        bind_map.map_event(None, ('shift',), 'ms_right', 'contrast_restore')
+
         # Marker
         self.marker = {'type': 'circle', 'color': 'cyan', 'radius': 20}
         # Maintain marker tags as a set because we do not want
@@ -215,7 +231,7 @@ def _mouse_click_cb(self, viewer, event, data_x, data_y):
 
             # NOTE: By always using CompoundObject, marker handling logic
             # is simplified.
-            obj = self.marker(x=data_x, y=data_y)
+            obj = self._marker(x=data_x, y=data_y)
             objs.append(obj)
             viewer.canvas.add(self.dc.CompoundObject(*objs),
                               tag=marker_name)
@@ -360,7 +376,10 @@ def zoom_level(self):
 
     @zoom_level.setter
     def zoom_level(self, val):
-        self._viewer.scale_to(val, val)
+        if val == 'fit':
+            self._viewer.zoom_fit()
+        else:
+            self._viewer.scale_to(val, val)
 
     def zoom(self, val):
         """
@@ -384,28 +403,42 @@ def is_marking(self):
         """
         return self._is_marking
 
-    @is_marking.setter
-    def is_marking(self, val):
-        if not isinstance(val, bool):
-            raise ValueError('Must be True or False')
-        elif self.click_center and val:
-            raise ValueError('Cannot set to True while in click-center mode')
-        self._is_marking = val
+    def start_marking(self, marker_name=None,
+                      marker=None):
+        """
+        Start marking, with option to name this set of markers or
+        to specify the marker style.
+        """
+        self._cached_state = dict(click_center=self.click_center,
+                                  click_drag=self.click_drag,
+                                  scroll_pan=self.scroll_pan)
+        self.click_center = False
+        self.click_drag = False
+        # Set scroll_pan to ensure there is a mouse way to pan
+        self.scroll_pan = True
+        self._is_marking = True
+        if marker is not None:
+            self.marker = marker
 
-    def stop_marking(self, clear_markers=True):
+    def stop_marking(self, clear_markers=False):
         """
         Stop marking mode, with option to clear markers, if desired.
 
         Parameters
         ----------
-        clear_markers : bool
+        clear_markers : bool, optional
             If ``clear_markers`` is `False`, existing markers are
             retained until :meth:`reset_markers` is called.
             Otherwise, they are erased.
         """
-        self.is_marking = False
-        if clear_markers:
-            self.reset_markers()
+        if self.is_marking:
+            self._is_marking = False
+            self.click_center = self._cached_state['click_center']
+            self.click_drag = self._cached_state['click_drag']
+            self.scroll_pan = self._cached_state['scroll_pan']
+            self._cached_state = {}
+            if clear_markers:
+                self.reset_markers()
 
     @property
     def marker(self):
@@ -421,24 +454,33 @@ def marker(self):
             {'type': 'plus', 'color': 'red', 'radius': 20}
 
         """
-        return self._marker
+        # Change the marker from a very ginga-specific type (a partial
+        # of a ginga drawing canvas type) to a generic dict, which is
+        # what we expect the user to provide.
+        #
+        # That makes things like self.marker = self.marker work.
+        return self._marker_dict
 
     @marker.setter
     def marker(self, val):
-        marker_type = val.pop('type')
+        # Make a new copy to avoid modifying the dict that the user passed in.
+        _marker = val.copy()
+        marker_type = _marker.pop('type')
         if marker_type == 'circle':
-            self._marker = functools.partial(self.dc.Circle, **val)
+            self._marker = functools.partial(self.dc.Circle, **_marker)
         elif marker_type == 'plus':
-            val['type'] = 'point'
-            val['style'] = 'plus'
-            self._marker = functools.partial(self.dc.Point, **val)
+            _marker['type'] = 'point'
+            _marker['style'] = 'plus'
+            self._marker = functools.partial(self.dc.Point, **_marker)
         elif marker_type == 'cross':
-            val['type'] = 'point'
-            val['style'] = 'cross'
-            self._marker = functools.partial(self.dc.Point, **val)
+            _marker['type'] = 'point'
+            _marker['style'] = 'cross'
+            self._marker = functools.partial(self.dc.Point, **_marker)
         else:  # TODO: Implement more shapes
             raise NotImplementedError(
                 'Marker type "{}" not supported'.format(marker_type))
+        # Only set this once we have successfully created a marker
+        self._marker_dict = val
 
     def get_markers(self, x_colname='x', y_colname='y',
                     skycoord_colname='coord',
@@ -645,7 +687,7 @@ def add_markers(self, table, x_colname='x', y_colname='y',
             self._viewer.canvas.delete_object_by_tag(marker_name)
 
         # TODO: Test to see if we can mix WCS and data on the same canvas
-        objs += [self.marker(x=x, y=y, coord=coord_type)
+        objs += [self._marker(x=x, y=y, coord=coord_type)
                  for x, y in zip(coord_x, coord_y)]
         self._viewer.canvas.add(self.dc.CompoundObject(*objs),
                                 tag=marker_name)
@@ -811,6 +853,10 @@ def click_center(self, val):
             raise ValueError('Must be True or False')
         elif self.is_marking and val:
             raise ValueError('Cannot set to True while in marking mode')
+
+        if val:
+            self.click_drag = False
+
         self._click_center = val
 
     # TODO: Awaiting https://github.com/ejeschke/ginga/issues/674
@@ -824,7 +870,24 @@ def click_drag(self):
         Note that this should be automatically made `False` when selection mode
         is activated.
         """
-        raise NotImplementedError
+        return self._click_drag
+
+    @click_drag.setter
+    def click_drag(self, value):
+        if not isinstance(value, bool):
+            raise ValueError('click_drag must be either True or False')
+        if self.is_marking:
+            raise ValueError('Interactive marking is in progress. Call '
+                             'stop_marking() to end marking before setting '
+                             'click_drag')
+        self._click_drag = value
+        bindmap = self._viewer.get_bindmap()
+        if value:
+            # Only turn off click_center if click_drag is being set to True
+            self.click_center = False
+            bindmap.map_event(None, (), 'ms_left', 'pan')
+        else:
+            bindmap.map_event(None, (), 'ms_left', 'cursor')
 
     @property
     def scroll_pan(self):
@@ -833,7 +896,19 @@ def scroll_pan(self):
         If True, scrolling moves around in the image.  If False, scrolling
         (up/down) *zooms* the image in and out.
         """
-        raise NotImplementedError
+        return self._scroll_pan
+
+    @scroll_pan.setter
+    def scroll_pan(self, value):
+        if not isinstance(value, bool):
+            raise ValueError('scroll_pan must be either True or False')
+
+        bindmap = self._viewer.get_bindmap()
+        self._scroll_pan = value
+        if value:
+            bindmap.map_event(None, (), 'pa_pan', 'pan')
+        else:
+            bindmap.map_event(None, (), 'pa_pan', 'zoom')
 
     def save(self, filename):
         """

astrowidgets/tests/test_api.py

@@ -84,6 +84,43 @@ def test_stop_marking():
 def test_is_marking():
     image = ImageWidget()
     assert image.is_marking in [True, False]
+    with pytest.raises(AttributeError):
+        image.is_marking = True
+
+
+def test_start_marking():
+    image = ImageWidget()
+
+    # Setting these to check that start_marking affects them.
+    image.click_center = True
+    assert image.click_center
+    image.scroll_pan = False
+    assert not image.scroll_pan
+
+    marker_style = {'color': 'yellow', 'radius': 10, 'type': 'cross'}
+    image.start_marking(marker_name='something',
+                        marker=marker_style)
+    assert image.is_marking
+    assert image.marker == marker_style
+    assert not image.click_center
+    assert not image.click_drag
+
+    # scroll_pan better activate when marking otherwise there is
+    # no way to pan while interactively marking
+    assert image.scroll_pan
+
+    # Make sure that when we stop_marking we get our old
+    # controls back.
+    image.stop_marking()
+    assert image.click_center
+    assert not image.scroll_pan
+
+    # Make sure that click_drag is restored as expected
+    image.click_drag = True
+    image.start_marking()
+    assert not image.click_drag
+    image.stop_marking()
+    assert image.click_drag
 
 
 def test_add_markers():
@@ -181,19 +218,58 @@ def test_cursor():
 
 def test_click_drag():
     image = ImageWidget()
-    with pytest.raises(NotImplementedError):
-        image.click_drag()
+    # Set this to ensure that click_drag turns it off
+    image._click_center = True
+
+    # Make sure that setting click_drag to False does not turn off
+    # click_center.
+
+    image.click_drag = False
+    assert image.click_center
+
+    image.click_drag = True
+
+    assert not image.click_center
+
+    # If is_marking is true then trying to click_drag
+    # should fail.
+    image._is_marking = True
+    with pytest.raises(ValueError) as e:
+        image.click_drag = True
+    assert 'Interactive marking' in str(e)
 
 
 def test_click_center():
     image = ImageWidget()
     assert (image.click_center is True) or (image.click_center is False)
 
+    # Set click_drag True and check that click_center affects it appropriately
+    image.click_drag = True
+
+    image.click_center = False
+    assert image.click_drag
+
+    image.click_center = True
+    assert not image.click_drag
+
+    image.start_marking()
+    # If marking is in progress then setting click center should fail
+    with pytest.raises(ValueError) as e:
+        image.click_center = True
+
+    assert 'Cannot set' in str(e)
+
+    # setting to False is fine though so no error is expected here
+    image.click_center = False
+
 
 def test_scroll_pan():
     image = ImageWidget()
-    with pytest.raises(NotImplementedError):
-        image.scroll_pan()
+
+    # Make sure scroll_pan is actually settable
+    for val in [True, False]:
+        image.scroll_pan = val
+        assert image.scroll_pan is val
 
 
 def test_save():

astrowidgets/tests/test_image_widget.py

@@ -154,7 +154,7 @@ def test_can_add_markers_with_names():
     # name has been added.
     data_x = 50
     data_y = 50
-    image.is_marking = True
+    image._is_marking = True
     image._mouse_click_cb(image._viewer, None, data_x, data_y)
     assert image._interactive_marker_set_name in image._marktags
 
@@ -188,7 +188,7 @@ def test_get_marker_with_names():
     image.add_markers(input_markers)
 
     # Add pseudo-interactive points
-    image.is_marking = True
+    image._is_marking = True
     for data_x, data_y in input_markers:
         image._mouse_click_cb(image._viewer, None, data_x, data_y)