[pitivi] timeline: Fix layers priorities updating when a layer is removed

[Thibault Saunier <tsaunier src gnome org>]

commit 7cf776c9d8265b058c58695cba288881b30bc16d
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Sun Feb 12 00:36:14 2017 +0100

    timeline: Fix layers priorities updating when a layer is removed
    
    Until now, when the timeline detected a layer removal, it was updating
    the priorities of the rest of the layers itself. This was not good
    because when the removal is done when undoing a layer-add operation, the
    rest of the layers have their priorities decreased twice: once by the
    timeline and once by the LayerMoved actions recorded when the original
    undoable action has been performed.
    
    Now the priorities of the remaining layers are decreased at the same
    place where the removal is performed: `LayerControls.__delete_layer_cb`.
    The Timeline is responsible only for updating the position of the layer
    widgets, done through `__update_layers`.
    
    Fixes https://phabricator.freedesktop.org/T7700
    
    Reviewed-by: Thibault Saunier <tsaunier gnome org>
    Differential Revision: https://phabricator.freedesktop.org/D1660

 pitivi/timeline/layer.py        |   10 +++++++---
 pitivi/timeline/timeline.py     |   19 ++++++++-----------
 pitivi/undo/timeline.py         |    8 ++++++++
 tests/test_timeline_timeline.py |   22 ++++++++++++----------
 tests/test_undo_timeline.py     |   12 ++++++++++--
 5 files changed, 45 insertions(+), 26 deletions(-)
---
diff --git a/pitivi/timeline/layer.py b/pitivi/timeline/layer.py
index aafe6db..a489c9b 100644
--- a/pitivi/timeline/layer.py
+++ b/pitivi/timeline/layer.py
@@ -150,7 +150,7 @@ class LayerControls(Gtk.EventBox, Loggable):
         last = priority == layers_count - 1
         self.__move_layer_down_action.props.enabled = not last
         self.__move_layer_bottom_action.props.enabled = not last
-        self.__delete_layer_action.props.enabled = layers_count > 1
+        self.delete_layer_action.props.enabled = layers_count > 1
 
     def __updateName(self):
         self.name_entry.set_text(self.ges_layer.ui.getName())
@@ -183,8 +183,8 @@ class LayerControls(Gtk.EventBox, Loggable):
         action_group.add_action(action)
         menu_model.append(_("Move layer to bottom"), "layer.%s" % action.get_name().replace(" ", "."))
 
-        self.__delete_layer_action = Gio.SimpleAction.new("delete-layer", None)
-        action = self.__delete_layer_action
+        self.delete_layer_action = Gio.SimpleAction.new("delete-layer", None)
+        action = self.delete_layer_action
         action.connect("activate", self.__delete_layer_cb)
         action_group.add_action(action)
         menu_model.append(_("Delete layer"), "layer.%s" % action.get_name())
@@ -196,6 +196,10 @@ class LayerControls(Gtk.EventBox, Loggable):
         with self.app.action_log.started("delete layer",
                                          CommitTimelineFinalizingAction(pipeline)):
             self.ges_timeline.remove_layer(self.ges_layer)
+            removed_priority = self.ges_layer.props.priority
+            for ges_layer in self.ges_timeline.get_layers():
+                if ges_layer.props.priority > removed_priority:
+                    ges_layer.props.priority -= 1
 
     def __move_layer_cb(self, unused_simple_action, unused_parameter, step):
         index = self.ges_layer.get_priority()
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 18c9a92..1011f17 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -443,11 +443,11 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
             self.ges_timeline.disconnect_by_func(self._durationChangedCb)
             self.ges_timeline.disconnect_by_func(self._layer_added_cb)
-            self.ges_timeline.disconnect_by_func(self._layerRemovedCb)
+            self.ges_timeline.disconnect_by_func(self._layer_removed_cb)
             self.ges_timeline.disconnect_by_func(self._snapCb)
             self.ges_timeline.disconnect_by_func(self._snapEndedCb)
             for ges_layer in self.ges_timeline.get_layers():
-                self._removeLayer(ges_layer)
+                self._remove_layer(ges_layer)
 
             self.ges_timeline.ui = None
             self.ges_timeline = None
@@ -467,7 +467,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
         self.ges_timeline.connect("notify::duration", self._durationChangedCb)
         self.ges_timeline.connect("layer-added", self._layer_added_cb)
-        self.ges_timeline.connect("layer-removed", self._layerRemovedCb)
+        self.ges_timeline.connect("layer-removed", self._layer_removed_cb)
         self.ges_timeline.connect("snapping-started", self._snapCb)
         self.ges_timeline.connect("snapping-ended", self._snapEndedCb)
 
@@ -998,10 +998,10 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
             self.debug("Layers still being shuffled, not updating widgets: %s", priorities)
             return
         self.debug("Updating layers widgets positions")
-        for i, ges_layer in enumerate(self.ges_timeline.get_layers()):
+        for ges_layer in self.ges_timeline.get_layers():
             self.__update_layer(ges_layer)
 
-    def _removeLayer(self, ges_layer):
+    def _remove_layer(self, ges_layer):
         self.info("Removing layer: %s", ges_layer.props.priority)
         self.layout.layers_vbox.remove(ges_layer.ui)
         self._layers_controls_vbox.remove(ges_layer.control_ui)
@@ -1016,12 +1016,9 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         ges_layer.ui = None
         ges_layer.control_ui = None
 
-    def _layerRemovedCb(self, ges_timeline, ges_layer):
-        self._removeLayer(ges_layer)
-        removed_priority = ges_layer.props.priority
-        for priority, ges_layer in enumerate(ges_timeline.get_layers()):
-            if priority >= removed_priority:
-                ges_layer.props.priority -= 1
+    def _layer_removed_cb(self, unused_ges_timeline, ges_layer):
+        self._remove_layer(ges_layer)
+        self.__update_layers()
 
     def separator_priority(self, separator):
         position = self.layout.layers_vbox.child_get_property(separator, "position")
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index 52d1742..2e986b3 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -478,6 +478,9 @@ class LayerRemoved(UndoableAction):
         self.ges_timeline = ges_timeline
         self.ges_layer = ges_layer
 
+    def __repr__(self):
+        return "<LayerRemoved %s>" % self.ges_layer
+
     def do(self):
         self.ges_timeline.remove_layer(self.ges_layer)
         self.ges_timeline.get_asset().pipeline.commit_timeline()
@@ -499,6 +502,11 @@ class LayerMoved(UndoableAction):
         self.old_priority = old_priority
         self.priority = priority
 
+    def __repr__(self):
+        return "<LayerMoved %s: %s -> %s>" % (self.ges_layer,
+                                              self.old_priority,
+                                              self.priority)
+
     def do(self):
         self.ges_layer.props.priority = self.priority
 
diff --git a/tests/test_timeline_timeline.py b/tests/test_timeline_timeline.py
index 37d0f7b..886359f 100644
--- a/tests/test_timeline_timeline.py
+++ b/tests/test_timeline_timeline.py
@@ -202,27 +202,29 @@ class TestLayers(BaseTestTimeline):
         self.assertListEqual(positions, expected_positions)
 
     def test_remove_layer(self):
-        self.check_remove_layer([0, 0, 0, 0])
-        self.check_remove_layer([0, 0, 1, 0])
-        self.check_remove_layer([0, 1, 0, 0])
-        self.check_remove_layer([0, 2, 1, 0])
-        self.check_remove_layer([1, 0, 1, 0])
-        self.check_remove_layer([2, 2, 0, 0])
-        self.check_remove_layer([3, 2, 1, 0])
+        self.check_remove_layer([0, 0, 0])
+        self.check_remove_layer([0, 0, 1])
+        self.check_remove_layer([0, 1, 0])
+        self.check_remove_layer([0, 2, 1])
+        self.check_remove_layer([1, 0, 1])
+        self.check_remove_layer([2, 2, 0])
+        self.check_remove_layer([3, 2, 1])
 
     def check_remove_layer(self, removal_order):
         timeline = create_timeline_container().timeline
 
         # Add layers to remove them later.
         ges_layers = []
-        for priority in range(len(removal_order)):
+        # Pitivi doesn't support removing the last remaining layer,
+        # that's why we create an extra layer.
+        for priority in range(len(removal_order) + 1):
             ges_layer = timeline.createLayer(priority)
             ges_layers.append(ges_layer)
 
-        # Remove the layers in the specified order.
+        # Remove layers one by one in the specified order.
         for priority in removal_order:
             ges_layer = ges_layers[priority]
-            self.assertTrue(timeline.ges_timeline.remove_layer(ges_layer))
+            ges_layer.control_ui.delete_layer_action.activate(None)
             ges_layers.remove(ges_layer)
             self.check_priorities_and_positions(timeline, ges_layers, list(range(len(ges_layers))))
 
diff --git a/tests/test_undo_timeline.py b/tests/test_undo_timeline.py
index 60c5645..7224ff3 100644
--- a/tests/test_undo_timeline.py
+++ b/tests/test_undo_timeline.py
@@ -112,12 +112,14 @@ class TestTimelineObserver(BaseTestUndoTimeline):
         self.check_removal(self.timeline.get_layers())
 
     def check_removal(self, ges_layers):
+        if len(ges_layers) == 1:
+            # We don't support removing the last remaining layer.
+            return
         for ges_layer in ges_layers:
             remaining_layers = list(ges_layers)
             remaining_layers.remove(ges_layer)
 
-            with self.action_log.started("layer removed"):
-                self.timeline.remove_layer(ges_layer)
+            ges_layer.control_ui.delete_layer_action.activate(None)
             self.check_layers(remaining_layers)
 
             self.action_log.undo()
@@ -847,6 +849,7 @@ class TestDragDropUndo(BaseTestUndoTimeline):
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 2)
         self.assertEqual(layers[0], self.layer)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [])
         self.assertEqual(layers[1].get_clips(), [clip])
 
@@ -854,12 +857,14 @@ class TestDragDropUndo(BaseTestUndoTimeline):
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 1)
         self.assertEqual(layers[0], self.layer)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [clip])
 
         self.action_log.redo()
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 2)
         self.assertEqual(layers[0], self.layer)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [])
         self.assertEqual(layers[1].get_clips(), [clip])
 
@@ -897,18 +902,21 @@ class TestDragDropUndo(BaseTestUndoTimeline):
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 2)
         self.assertEqual(layers[1], self.layer)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [clip])
         self.assertEqual(layers[1].get_clips(), [])
 
         self.action_log.undo()
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 1)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [clip])
 
         self.action_log.redo()
         layers = self.timeline.get_layers()
         self.assertEqual(len(layers), 2)
         self.assertEqual(layers[0], self.layer)
+        self.check_layers(layers)
         self.assertEqual(layers[0].get_clips(), [clip])
         self.assertEqual(layers[1].get_clips(), [])