[pitivi] elements: Keyframe not removed on double-click

[Thibault Saunier <tsaunier src gnome org>]

commit c7e8c3092aead2e12e41ef4dab82c488019ffaf3
Author: Stefan Popa <stefanpopa2209 gmail com>
Date:   Thu Jun 8 14:44:25 2017 +0000

    elements: Keyframe not removed on double-click
    
    Double-clicking on a keyframe didn't remove the keyframe because a
    double-click also triggered a keyframe movement and the undo system failed
    with two pending top-level operations.
    
    To fix this, the removal of a keyframe is not considered a top-level operation
    anymore. A unit test was also added.
    
    Fixes T7762
    
    Reviewed-by: Thibault Saunier <tsaunier gnome org>
    Differential Revision: https://phabricator.freedesktop.org/D1753

 pitivi/timeline/elements.py     |   35 +++++++++++-------
 pitivi/undo/undo.py             |   10 +++++
 tests/test_timeline_elements.py |   76 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 14 deletions(-)
---
diff --git a/pitivi/timeline/elements.py b/pitivi/timeline/elements.py
index 7948f3f..ffaa9c7 100644
--- a/pitivi/timeline/elements.py
+++ b/pitivi/timeline/elements.py
@@ -112,15 +112,15 @@ class KeyframeCurve(FigureCanvas, Loggable):
         self.__line_ys = []
 
         # facecolor to None for transparency
-        self.__ax = figure.add_axes([0, 0, 1, 1], facecolor='None')
+        self._ax = figure.add_axes([0, 0, 1, 1], facecolor='None')
         # Clear the Axes object.
-        self.__ax.cla()
+        self._ax.cla()
 
         # FIXME: drawing a grid and ticks would be nice, but
         # matplotlib is too slow for now.
-        self.__ax.grid(False)
+        self._ax.grid(False)
 
-        self.__ax.tick_params(axis='both',
+        self._ax.tick_params(axis='both',
                               which='both',
                               bottom='off',
                               top='off',
@@ -132,14 +132,14 @@ class KeyframeCurve(FigureCanvas, Loggable):
 
         # The PathCollection object holding the keyframes dots.
         sizes = [50]
-        self.__keyframes = self.__ax.scatter([], [], marker='D', s=sizes,
+        self.__keyframes = self._ax.scatter([], [], marker='D', s=sizes,
                                              c=KEYFRAME_NODE_COLOR, zorder=2)
 
         # matplotlib weirdness, simply here to avoid a warning ..
         self.__keyframes.set_picker(True)
 
         # The Line2D object holding the lines between keyframes.
-        self.__line = self.__ax.plot([], [],
+        self.__line = self._ax.plot([], [],
                                      alpha=KEYFRAME_LINE_ALPHA,
                                      c=KEYFRAME_LINE_COLOR,
                                      linewidth=KEYFRAME_LINE_HEIGHT, zorder=1)[0]
@@ -161,9 +161,9 @@ class KeyframeCurve(FigureCanvas, Loggable):
         self.connect("event", self._eventCb)
         self.connect("notify::height-request", self.__heightRequestCb)
 
-        self.mpl_connect('button_press_event', self.__mplButtonPressEventCb)
-        self.mpl_connect('button_release_event', self.__mplButtonReleaseEventCb)
-        self.mpl_connect('motion_notify_event', self.__mplMotionEventCb)
+        self.mpl_connect('button_press_event', self._mpl_button_press_event_cb)
+        self.mpl_connect('button_release_event', self._mpl_button_release_event_cb)
+        self.mpl_connect('motion_notify_event', self._mpl_motion_event_cb)
 
     def release(self):
         disconnectAllByFunc(self, self.__heightRequestCb)
@@ -179,7 +179,7 @@ class KeyframeCurve(FigureCanvas, Loggable):
 
         ylim_min = -(KEYFRAME_LINE_HEIGHT / height)
         ylim_max = (self.__ylim_max * height) / (height - KEYFRAME_LINE_HEIGHT)
-        self.__ax.set_ylim(ylim_min, ylim_max)
+        self._ax.set_ylim(ylim_min, ylim_max)
 
     def __heightRequestCb(self, unused_self, unused_pspec):
         self.__computeYlim()
@@ -196,7 +196,7 @@ class KeyframeCurve(FigureCanvas, Loggable):
             self.__line_xs.append(value.timestamp)
             self.__line_ys.append(value.value)
 
-        self.__ax.set_xlim(self.__line_xs[0], self.__line_xs[-1])
+        self._ax.set_xlim(self.__line_xs[0], self.__line_xs[-1])
         self.__computeYlim()
 
         arr = numpy.array((self.__line_xs, self.__line_ys))
@@ -248,7 +248,7 @@ class KeyframeCurve(FigureCanvas, Loggable):
             self.__timeline.get_window().set_cursor(cursor)
         return False
 
-    def __mplButtonPressEventCb(self, event):
+    def _mpl_button_press_event_cb(self, event):
         if event.button != 1:
             return
 
@@ -264,6 +264,13 @@ class KeyframeCurve(FigureCanvas, Loggable):
                 if index == 0 or index == len(offsets) - 1:
                     # It's an edge keyframe. These should not be removed.
                     return
+
+                # Rollback the last operation if it is "Move keyframe".
+                # This is needed because a double-click also triggers a
+                # BUTTON_PRESS event which starts a "Move keyframe" operation
+                self.__timeline.app.action_log.try_rollback("Move keyframe")
+                self.__offset = None
+
                 # A keyframe has been double-clicked, remove it.
                 self.debug("Removing keyframe at timestamp %lf", offset)
                 with self.__timeline.app.action_log.started("Remove keyframe",
@@ -292,7 +299,7 @@ class KeyframeCurve(FigureCanvas, Loggable):
             self.__ydata_drag_start = max(self.__ylim_min, min(event.ydata, self.__ylim_max))
             self.handling_motion = True
 
-    def __mplMotionEventCb(self, event):
+    def _mpl_motion_event_cb(self, event):
         if event.ydata is not None and event.xdata is not None:
             # The mouse event is in the figure boundaries.
             if self.__offset is not None:
@@ -333,7 +340,7 @@ class KeyframeCurve(FigureCanvas, Loggable):
 
         self.__timeline.get_window().set_cursor(cursor)
 
-    def __mplButtonReleaseEventCb(self, event):
+    def _mpl_button_release_event_cb(self, event):
         if event.button != 1:
             return
 
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index 6b9b066..721f5da 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -254,6 +254,16 @@ class UndoableActionLog(GObject.Object, Loggable):
         self.emit("rollback", stack)
         stack.undo()
 
+    def try_rollback(self, action_group_name):
+        """Do rollback if the last started operation is @action_group_name."""
+        try:
+            last_operation = self._get_last_stack().action_group_name
+        except UndoWrongStateError:
+            return
+
+        if last_operation == action_group_name:
+            self.rollback()
+
     def commit(self, action_group_name):
         """Commits the last started operation."""
         if self.running:
diff --git a/tests/test_timeline_elements.py b/tests/test_timeline_elements.py
index 371cc68..3b954f7 100644
--- a/tests/test_timeline_elements.py
+++ b/tests/test_timeline_elements.py
@@ -22,9 +22,12 @@ from unittest import mock
 from unittest import TestCase
 
 from gi.overrides import GObject
+from gi.repository import Gdk
 from gi.repository import GES
+from matplotlib.backend_bases import MouseEvent
 
 from pitivi.timeline.elements import GES_TYPE_UI_TYPE
+from pitivi.undo.undo import UndoableActionLog
 from tests.common import create_test_clip
 from tests.common import create_timeline_container
 from tests.test_timeline_timeline import BaseTestTimeline
@@ -36,6 +39,7 @@ class TestKeyframeCurve(BaseTestTimeline):
     def test_keyframe_toggle(self):
         """Checks keyframes toggling at the playhead position."""
         timeline_container = create_timeline_container()
+        timeline_container.app.action_log = UndoableActionLog()
         timeline = timeline_container.timeline
         ges_layer = timeline.ges_timeline.append_layer()
         ges_clip1 = self.add_clip(ges_layer, 0)
@@ -52,6 +56,11 @@ class TestKeyframeCurve(BaseTestTimeline):
         self.check_keyframe_toggle(ges_clip3, timeline_container)
         self.check_keyframe_toggle(ges_clip4, timeline_container)
 
+        self.check_keyframe_ui_toggle(ges_clip1, timeline_container)
+        self.check_keyframe_ui_toggle(ges_clip2, timeline_container)
+        self.check_keyframe_ui_toggle(ges_clip3, timeline_container)
+        self.check_keyframe_ui_toggle(ges_clip4, timeline_container)
+
     def check_keyframe_toggle(self, ges_clip, timeline_container):
         """Checks keyframes toggling on the specified clip."""
         timeline = timeline_container.timeline
@@ -109,6 +118,73 @@ class TestKeyframeCurve(BaseTestTimeline):
             values = [item.timestamp for item in control_source.get_all()]
             self.assertEqual(values, [inpoint, inpoint + duration])
 
+    def check_keyframe_ui_toggle(self, ges_clip, timeline_container):
+        """Checks keyframes toggling by click events."""
+        timeline = timeline_container.timeline
+
+        inpoint = ges_clip.props.in_point
+        duration = ges_clip.props.duration
+        offsets = (1, int(duration / 2), int(duration) - 1)
+        timeline.selection.select([ges_clip])
+
+        ges_video_source = ges_clip.find_track_element(None, GES.VideoSource)
+        binding = ges_video_source.get_control_binding("alpha")
+        control_source = binding.props.control_source
+        keyframe_curve = ges_video_source.ui.keyframe_curve
+
+        values = [item.timestamp for item in control_source.get_all()]
+        self.assertEqual(values, [inpoint, inpoint + duration])
+
+        # Add keyframes.
+        for offset in offsets:
+            xdata, ydata = inpoint + offset, 1
+            x, y = keyframe_curve._ax.transData.transform((xdata, ydata))
+
+            event = MouseEvent(
+                name = "button_press_event",
+                canvas = keyframe_curve,
+                x = x,
+                y = y,
+                button = 1
+            )
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
+            keyframe_curve._mpl_button_press_event_cb(event)
+            event.name = "button_release_event"
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_RELEASE)
+            keyframe_curve._mpl_button_release_event_cb(event)
+
+            values = [item.timestamp for item in control_source.get_all()]
+            self.assertIn(inpoint + offset, values)
+
+        # Remove keyframes.
+        for offset in offsets:
+            xdata, ydata = inpoint + offset, 1
+            x, y = keyframe_curve._ax.transData.transform((xdata, ydata))
+
+            event = MouseEvent(
+                name="button_press_event",
+                canvas=keyframe_curve,
+                x=x,
+                y=y,
+                button=1
+            )
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
+            keyframe_curve._mpl_button_press_event_cb(event)
+            event.name = "button_release_event"
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_RELEASE)
+            keyframe_curve._mpl_button_release_event_cb(event)
+            event.name = "button_press_event"
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
+            keyframe_curve._mpl_button_press_event_cb(event)
+            event.guiEvent = Gdk.Event.new(Gdk.EventType._2BUTTON_PRESS)
+            keyframe_curve._mpl_button_press_event_cb(event)
+            event.name = "button_release_event"
+            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_RELEASE)
+            keyframe_curve._mpl_button_release_event_cb(event)
+
+            values = [item.timestamp for item in control_source.get_all()]
+            self.assertNotIn(inpoint + offset, values)
+
     def test_no_clip_selected(self):
         """Checks nothing happens when no clip is selected."""
         timeline_container = create_timeline_container()