GNOME.org
 

[pitivi] timeline: Removed current_group

commit dd262c24aa75237d5c3496a486d05850b0f7703f
Author: swayne <swayamjeet swain>
Date:   Wed May 29 13:44:02 2019 +0530

    timeline: Removed current_group
    
    Instead of keeping the current_group and the Selection object in sync,
    we now use only the Selection object.
    
    When a clip was removed from its layer, the layer was taking care of
    removing it from the Selection object. This is not the case anymore,
    as it simplifies the drag&drop logic. Previously as clips were dragged
    from one layer to another, they were being removed from the Selection
    object. Now the Selection object is stable and the logic is simpler.
    
    Fixes #2288

 docs/Advanced_plugin.md         |   3 +-
 pitivi/timeline/elements.py     |   8 ---
 pitivi/timeline/layer.py        |   2 -
 pitivi/timeline/timeline.py     | 109 +++++++++++++++++-----------------------
 pitivi/titleeditor.py           |   2 -
 pitivi/utils/timeline.py        |  32 ++++++++++--
 tests/common.py                 |   1 +
 tests/test_timeline_timeline.py |  19 ++-----
 tests/test_undo_timeline.py     |   3 --
 9 files changed, 80 insertions(+), 99 deletions(-)
---
diff --git a/docs/Advanced_plugin.md b/docs/Advanced_plugin.md
index b1d0ea86..760f468e 100644
--- a/docs/Advanced_plugin.md
+++ b/docs/Advanced_plugin.md
@@ -49,7 +49,6 @@ The interesting part is the `__clicked_cb` callback method, which is connected t
         timeline = self.app.gui.editor.timeline_ui.timeline
         clips = sorted(timeline.selection, key=lambda x : x.get_start())
         if len(clips) >= 2:
-            timeline.current_group.ungroup(False)  # https://gitlab.gnome.org/GNOME/pitivi/issues/2288
             previous = clips[0]
             for clip in clips[1:]:
                 clip.set_start(previous.get_start() + previous.get_duration())
@@ -72,4 +71,4 @@ The plugin can be improved by:
 - Disabling the button automatically when `timeline.selection` contains less than two clips.
 - Making it work for the entire timeline in case there is no selection.
 
-Check out the Pitivi code and the [GES API](http://lazka.github.io/pgi-docs/#GES-1.0) to see what can be 
done, and tell us what you are hacking!
\ No newline at end of file
+Check out the Pitivi code and the [GES API](http://lazka.github.io/pgi-docs/#GES-1.0) to see what can be 
done, and tell us what you are hacking!
diff --git a/pitivi/timeline/elements.py b/pitivi/timeline/elements.py
index 6781a322..53705972 100644
--- a/pitivi/timeline/elements.py
+++ b/pitivi/timeline/elements.py
@@ -1124,8 +1124,6 @@ class Clip(Gtk.EventBox, Zoomable, Loggable):
 
         if target.name() == EFFECT_TARGET_ENTRY.target:
             self.info("Adding effect %s", self.timeline.dropData)
-            self.timeline.resetSelectionGroup()
-            self.timeline.current_group.add(self.ges_clip)
             self.timeline.selection.setSelection([self.ges_clip], SELECT)
             self.app.gui.editor.switchContextTab(self.ges_clip)
 
@@ -1256,17 +1254,11 @@ class Clip(Gtk.EventBox, Zoomable, Loggable):
         if self.timeline.get_parent()._controlMask:
             if not self.get_state_flags() & Gtk.StateFlags.SELECTED:
                 mode = SELECT_ADD
-                self.timeline.current_group.add(
-                    self.ges_clip.get_toplevel_parent())
             else:
-                self.timeline.current_group.remove(
-                    self.ges_clip.get_toplevel_parent())
                 mode = UNSELECT
             clicked_layer, click_pos = self.timeline.get_clicked_layer_and_pos(event)
             self.timeline.set_selection_meta_info(clicked_layer, click_pos, mode)
         else:
-            self.timeline.resetSelectionGroup()
-            self.timeline.current_group.add(self.ges_clip.get_toplevel_parent())
             self.app.gui.editor.switchContextTab(self.ges_clip)
 
         parent = self.ges_clip.get_toplevel_parent()
diff --git a/pitivi/timeline/layer.py b/pitivi/timeline/layer.py
index fe3e816d..2499227b 100644
--- a/pitivi/timeline/layer.py
+++ b/pitivi/timeline/layer.py
@@ -377,8 +377,6 @@ class Layer(Gtk.Layout, Zoomable, Loggable):
         ges_clip.disconnect_by_func(self._childAddedToClipCb)
         ges_clip.disconnect_by_func(self._childRemovedFromClipCb)
 
-        self.timeline.selection.unselect([ges_clip])
-
     def updatePosition(self):
         for ges_clip in self.ges_layer.get_clips():
             if hasattr(ges_clip, "ui"):
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 2106a6d3..d9f20a15 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -367,16 +367,16 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
         # Clip selection.
         self.selection = Selection()
-        self.current_group = None
         # The last layer where the user clicked.
         self.last_clicked_layer = None
         # Position where the user last clicked.
         self.last_click_pos = 0
-        self.resetSelectionGroup()
 
         # Clip editing.
-        # Which clip is being edited.
+        # Which clip widget is being edited.
         self.draggingElement = None
+        # The GES.Group in case there are one or more clips being dragged.
+        self.dragging_group = None
         # Which handle of the draggingElement has been clicked, if any.
         # If set, it means we are in a trim operation.
         self.__clickedHandle = None
@@ -445,14 +445,6 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
         return media_types
 
-    def resetSelectionGroup(self):
-        self.debug("Reset selection group")
-        if self.current_group:
-            self.current_group.ungroup(recursive=False)
-
-        self.current_group = GES.Group()
-        self.current_group.props.serialize = False
-
     def setProject(self, project):
         """Connects to the GES.Timeline holding the project."""
         # Avoid starting/closing preview generation like crazy while tearing down project
@@ -709,6 +701,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         self.__next_seek_position = next_seek_position
 
     def _button_press_event_cb(self, unused_widget, event):
+        """Handles a mouse button press event."""
         self.debug("PRESSED %s", event)
         self.app.gui.editor.focusTimeline()
 
@@ -721,9 +714,10 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
                 self.__clickedHandle = event_widget
             self.debug("Dragging element is %s", self.draggingElement)
 
-            if self.draggingElement is not None:
+            if self.draggingElement:
                 self.__drag_start_x = event.x
                 self._on_layer = self.draggingElement.layer.ges_layer
+                self.dragging_group = self.selection.group()
             else:
                 layer_controls = self._getParentOfType(event_widget, LayerControls)
                 if layer_controls:
@@ -746,8 +740,6 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
             self._scroll_start_x = event.x
             self._scroll_start_y = event.y
 
-        return False
-
     def _button_release_event_cb(self, unused_widget, event):
         allow_seek = not self.__got_dragged
 
@@ -781,13 +773,10 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
                     clicked_layer, click_pos = self.get_clicked_layer_and_pos(event)
                     self.set_selection_meta_info(clicked_layer, click_pos, SELECT)
                 else:
-                    self.resetSelectionGroup()
                     last_click_pos = self.last_click_pos
                     cur_clicked_layer, cur_click_pos = self.get_clicked_layer_and_pos(event)
                     clips = self.get_clips_in_between(
                         last_clicked_layer, cur_clicked_layer, last_click_pos, cur_click_pos)
-                    for clip in clips:
-                        self.current_group.add(clip.get_toplevel_parent())
                     self.selection.setSelection(clips, SELECT)
             elif not self.get_parent()._controlMask:
                 clicked_layer, click_pos = self.get_clicked_layer_and_pos(event)
@@ -838,7 +827,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         # Also include those clips which are grouped with currently selected clips.
         for clip in clips:
             toplevel = clip.get_toplevel_parent()
-            if isinstance(toplevel, GES.Group) and toplevel != self.current_group:
+            if isinstance(toplevel, GES.Group):
                 grouped_clips.update([c for c in toplevel.get_children(True)
                                       if isinstance(c, GES.Clip)])
 
@@ -895,11 +884,8 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         self.vadj.set_value(self.vadj.get_value() + y_diff)
 
     def _selectUnderMarquee(self):
-        self.resetSelectionGroup()
         if self.layout.marquee.props.width_request > 0:
             clips = self.layout.marquee.find_clips()
-            for clip in clips:
-                self.current_group.add(clip.get_toplevel_parent())
         else:
             clips = []
         self.selection.setSelection(clips, SELECT)
@@ -919,12 +905,13 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         """
         placement = 0
         self.draggingElement = None
-        self.resetSelectionGroup()
-        self.selection.setSelection([], SELECT)
+
         assets = self._project.assetsForUris(self.dropData)
         if not assets:
             self._project.addUris(self.dropData)
             return False
+
+        ges_clips = []
         for asset in assets:
             if asset.is_image():
                 clip_duration = self.app.settings.imageClipLength * \
@@ -945,17 +932,21 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
                                            clip_duration,
                                            asset.get_supported_formats())
             placement += clip_duration
-            self.current_group.add(ges_clip.get_toplevel_parent())
-            self.selection.setSelection([], SELECT_ADD)
             ges_clip.first_placement = True
             self._project.pipeline.commit_timeline()
 
-            if not self.draggingElement:
-                self.draggingElement = ges_clip.ui
-                self._on_layer = ges_layer
+            ges_clips.append(ges_clip)
 
+        if ges_clips:
+            ges_clip = ges_clips[0]
+            self.draggingElement = ges_clip.ui
+            self._on_layer = ges_layer
             self.dropping_clips = True
 
+            self.selection.setSelection(ges_clips, SELECT)
+
+            self.dragging_group = self.selection.group()
+
         return True
 
     def _drag_motion_cb(self, widget, context, x, y, timestamp):
@@ -985,19 +976,19 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         target = self.drag_dest_find_target(context, None)
         if self.draggingElement:
             self.__last_clips_on_leave = [(clip.get_layer(), clip)
-                                          for clip in self.current_group.get_children(False)]
+                                          for clip in self.dragging_group.get_children(False)]
             self.dropDataReady = False
             if self.dropping_clips:
-                clips = self.current_group.get_children(False)
-                self.resetSelectionGroup()
                 self.selection.setSelection([], SELECT)
-                for clip in clips:
+                for clip in self.dragging_group.get_children(False):
                     clip.get_layer().remove_clip(clip)
                 self._project.pipeline.commit_timeline()
 
             self.draggingElement = None
             self.__got_dragged = False
             self.dropping_clips = False
+            self.dragging_group.ungroup(recursive=False)
+            self.dragging_group = None
         elif target == URI_TARGET_ENTRY.target:
             self.cleanDropData()
 
@@ -1325,7 +1316,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         res = self._get_layer_at(y, prefer_ges_layer=self._on_layer)
         self._on_layer, self.__on_separators = res
         if (mode != GES.EditMode.EDIT_NORMAL or
-                self.current_group.props.height > 1):
+                self.dragging_group.props.height > 1):
             # When dragging clips from more than one layer, do not allow
             # them to be dragged between layers to create a new layer.
             self.__on_separators = []
@@ -1374,6 +1365,9 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
             self.editing_context.finish()
 
         self.draggingElement = None
+        if self.dragging_group is not None:
+            self.dragging_group.ungroup(recursive=False)
+            self.dragging_group = None
         self.__clickedHandle = None
         self.__got_dragged = False
         self.editing_context = None
@@ -1761,7 +1755,6 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
 
     def _deleteSelected(self, unused_action, unused_parameter):
         if self.ges_timeline:
-            self.timeline.resetSelectionGroup()
             with Previewer.manager.paused():
                 with self.app.action_log.started("delete clip",
                                                 
finalizing_action=CommitTimelineFinalizingAction(self._project.pipeline),
@@ -1824,13 +1817,11 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
         with self.app.action_log.started("ungroup",
                                          
finalizing_action=CommitTimelineFinalizingAction(self._project.pipeline),
                                          toplevel=True):
-            for obj in self.timeline.selection:
-                toplevel = obj.get_toplevel_parent()
-                if toplevel == self.timeline.current_group:
-                    for child in toplevel.get_children(False):
-                        child.ungroup(recursive=False)
+            toplevels = self.timeline.selection.toplevels
+            for toplevel in toplevels:
+                if isinstance(toplevel, GES.Group) or len(toplevels) == 1:
+                    toplevel.ungroup(recursive=False)
 
-        self.timeline.resetSelectionGroup()
         self.timeline.selection.setSelection([], SELECT)
 
     def _group_selected_cb(self, unused_action, unused_parameter):
@@ -1841,24 +1832,9 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
         with self.app.action_log.started("group",
                                          
finalizing_action=CommitTimelineFinalizingAction(self._project.pipeline),
                                          toplevel=True):
-            containers = set()
-            new_group = None
-            for obj in self.timeline.selection:
-                toplevel = obj.get_toplevel_parent()
-                if toplevel == self.timeline.current_group:
-                    for child in toplevel.get_children(False):
-                        containers.add(child)
-                    toplevel.ungroup(False)
-                else:
-                    containers.add(toplevel)
-
-            if containers:
-                new_group = GES.Container.group(list(containers))
-
-            self.timeline.resetSelectionGroup()
-
-            if new_group:
-                self.timeline.current_group.add(new_group)
+            toplevels = self.timeline.selection.toplevels
+            if toplevels:
+                GES.Container.group(list(toplevels))
 
             # timeline.selection doesn't change during grouping,
             # we need to manually update group actions.
@@ -1866,9 +1842,13 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
             self.updateActions()
 
     def __copyClipsCb(self, unused_action, unused_parameter):
-        if self.timeline.current_group:
-            self.__copied_group = self.timeline.current_group.copy(True)
-            self.updateActions()
+        group = self.timeline.selection.group()
+        try:
+            self.__copied_group = group.copy(deep=True)
+            self.__copied_group.props.serialize = False
+        finally:
+            group.ungroup(recursive=False)
+        self.updateActions()
 
     def __pasteClipsCb(self, unused_action, unused_parameter):
         if not self.__copied_group:
@@ -1880,8 +1860,11 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
                     toplevel=True):
             position = self._project.pipeline.getPosition()
             copied_group_shallow_copy = self.__copied_group.paste(position)
-            self.__copied_group = copied_group_shallow_copy.copy(True)
-            copied_group_shallow_copy.ungroup(recursive=False)
+            try:
+                self.__copied_group = copied_group_shallow_copy.copy(True)
+                self.__copied_group.props.serialize = False
+            finally:
+                copied_group_shallow_copy.ungroup(recursive=False)
 
     def __add_layer_cb(self, unused_action, unused_parameter):
         with self.app.action_log.started("add layer",
diff --git a/pitivi/titleeditor.py b/pitivi/titleeditor.py
index d9919133..4696e96f 100644
--- a/pitivi/titleeditor.py
+++ b/pitivi/titleeditor.py
@@ -262,8 +262,6 @@ class TitleEditor(Loggable):
             assert res
         # Select it so the Title editor becomes active.
         self._selection.setSelection([title_clip], SELECT)
-        self.app.gui.editor.timeline_ui.timeline.resetSelectionGroup()
-        self.app.gui.editor.timeline_ui.timeline.current_group.add(title_clip)
 
     def _propertyChangedCb(self, source, unused_gstelement, pspec):
         if self._setting_props:
diff --git a/pitivi/utils/timeline.py b/pitivi/utils/timeline.py
index e6174506..ac66b679 100644
--- a/pitivi/utils/timeline.py
+++ b/pitivi/utils/timeline.py
@@ -143,11 +143,14 @@ class Selection(GObject.Object, Loggable):
     def set_can_group_ungroup(self):
         can_ungroup = False
         toplevels = self.toplevels
-        for toplevel in toplevels:
-            if isinstance(toplevel, GES.Group) or len(toplevel.get_children(False)) > 1:
-                can_ungroup = True
-                break
         self.can_group = len(toplevels) > 1
+        # If we allow grouping, we disallow ungrouping.
+        if not self.can_group:
+            for toplevel in toplevels:
+                if (isinstance(toplevel, GES.Group) or
+                        (isinstance(toplevel, GES.Container) and len(toplevel.get_children(False)) > 1)):
+                    can_ungroup = True
+                    break
         self.can_ungroup = can_ungroup and not self.can_group
 
     def __get_selection_changes(self, old_selection):
@@ -212,6 +215,27 @@ class Selection(GObject.Object, Loggable):
                 for child in self.__serializable_toplevels(element):
                     yield child
 
+    def group(self):
+        """Groups the serializable toplevel elements into a new GES.Group.
+
+        Returns:
+            GES.Group: The serializable elements which have no parent or
+            have only non-serializable ancestors.
+        """
+        toplevels = set()
+        for obj in self.selected:
+            if not obj.timeline:
+                # The element has been removed from the timeline. Ignore it.
+                continue
+            toplevel = obj.get_toplevel_parent()
+            toplevels.add(toplevel)
+
+        group = GES.Group()
+        group.props.serialize = False
+        for toplevel in toplevels:
+            group.add(toplevel)
+        return group
+
     def __len__(self):
         return len(self.selected)
 
diff --git a/tests/common.py b/tests/common.py
index 7f170413..322ac53b 100644
--- a/tests/common.py
+++ b/tests/common.py
@@ -256,6 +256,7 @@ class TestCase(unittest.TestCase, Loggable):
                 with mock.patch.object(ges_clip.ui.timeline, "_get_layer_at") as _get_layer_at:
                     _get_layer_at.return_value = ges_clip.props.layer, None
                     ges_clip.ui._button_release_event_cb(None, event)
+                    ges_clip.ui.timeline._button_release_event_cb(None, event)
 
         self.assertEqual(bool(ges_clip.ui.get_state_flags() & Gtk.StateFlags.SELECTED),
                          expect_selected)
diff --git a/tests/test_timeline_timeline.py b/tests/test_timeline_timeline.py
index 8580188c..aed19b40 100644
--- a/tests/test_timeline_timeline.py
+++ b/tests/test_timeline_timeline.py
@@ -333,25 +333,18 @@ class TestGrouping(BaseTestTimeline):
 
         # Select the 2 clips
         for clip in clips:
+            self.assertIsNone(clip.get_parent())
             self.toggle_clip_selection(clip, expect_selected=True)
 
-        before_grouping_timeline_group = timeline.current_group
-
-        for clip in clips:
-            self.assertEqual(clip.get_parent(), timeline.current_group)
-
         timeline_container.group_action.emit("activate", None)
 
-        self.assertNotEqual(timeline.current_group, before_grouping_timeline_group)
         for clip in clips:
-            # Check that we created a new group and that this group is not
-            # the timeline current_group
+            # Check that we created a new group
             self.assertTrue(isinstance(clip.get_parent(), GES.Group))
-            self.assertNotEqual(clip.get_parent(), timeline.current_group)
             # The newly created group has been selected
-            self.assertEqual(clip.get_toplevel_parent(), timeline.current_group)
+            for selected_clip in timeline.selection:
+                self.assertEqual(clip.get_toplevel_parent(), selected_clip.get_toplevel_parent())
 
-        for clip in clips:
             self.assertEqual(clips[0].get_parent(), clip.get_parent())
             self.assertTrue(bool(clip.ui.get_state_flags() & Gtk.StateFlags.SELECTED))
             self.assertTrue(clip.selected.selected)
@@ -683,7 +676,6 @@ class TestShiftSelection(BaseTestTimeline):
             timeline._button_release_event_cb(None, event)
             self.__check_selected([ges_clip1, ges_clip2], [ges_clip3, ges_clip4])
             self.__reset_clips_selection(timeline)
-            timeline.resetSelectionGroup()
 
             # Simiulate shift+click before first and after fourth clip.
             timeline.get_clicked_layer_and_pos.return_value = (ges_layer, 1 * Gst.SECOND)
@@ -692,7 +684,6 @@ class TestShiftSelection(BaseTestTimeline):
             timeline._button_release_event_cb(None, event)
             self.__check_selected([ges_clip1, ges_clip2, ges_clip3, ges_clip4], [])
             self.__reset_clips_selection(timeline)
-            timeline.resetSelectionGroup()
 
             # Simiulate shift+click on first, after fourth and before third clip.
             timeline.get_clicked_layer_and_pos.return_value = (ges_layer, 6 * Gst.SECOND)
@@ -703,7 +694,6 @@ class TestShiftSelection(BaseTestTimeline):
             timeline._button_release_event_cb(None, event)
             self.__check_selected([ges_clip1, ges_clip2], [ges_clip3, ges_clip4])
             self.__reset_clips_selection(timeline)
-            timeline.resetSelectionGroup()
 
             # Simulate shift+click twice on the same clip.
             timeline.get_clicked_layer_and_pos.return_value = (ges_layer, 6 * Gst.SECOND)
@@ -759,7 +749,6 @@ class TestShiftSelection(BaseTestTimeline):
             self.__check_selected([ges_clip11, ges_clip12, ges_clip22, ges_clip23],
                 [ges_clip13, ges_clip21, ges_clip31, ges_clip32, ges_clip33])
             self.__reset_clips_selection(timeline)
-            timeline.resetSelectionGroup()
 
             timeline.get_clicked_layer_and_pos.return_value = (ges_layer1, 3 * Gst.SECOND)
             timeline._button_release_event_cb(None, event)
diff --git a/tests/test_undo_timeline.py b/tests/test_undo_timeline.py
index ef0a45f5..a695bcda 100644
--- a/tests/test_undo_timeline.py
+++ b/tests/test_undo_timeline.py
@@ -199,9 +199,6 @@ class TestTimelineObserver(BaseTestUndoTimeline):
         self.assertEqual(len(clips[1].get_children(False)), 1)
 
         timeline.selection.select(clips)
-        timeline.resetSelectionGroup()
-        for clip in clips:
-            timeline.current_group.add(clip)
         self.timeline_container.group_action.activate(None)
         clips = list(self.getTimelineClips())
         self.assertEqual(len(clips), 1, clips)
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]