[pitivi] timeline: Simplified the can-group/ungroup logic

[Alexandru Băluț <alexbalut src gnome org>]

commit f3db09ab2850108ec2004499efc93ea22c07806b
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Wed Jun 19 23:18:30 2019 +0200

    timeline: Simplified the can-group/ungroup logic

 pitivi/timeline/timeline.py     |  6 ++---
 pitivi/utils/timeline.py        | 51 +++++++++++++++++++++++------------------
 tests/test_timeline_timeline.py | 41 ++++++++++++++++-----------------
 tests/test_utils_timeline.py    | 44 +++++++++++++++++++++++++++++++----
 4 files changed, 91 insertions(+), 51 deletions(-)
---
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 11054e04..2106a6d3 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -840,7 +840,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
             toplevel = clip.get_toplevel_parent()
             if isinstance(toplevel, GES.Group) and toplevel != self.current_group:
                 grouped_clips.update([c for c in toplevel.get_children(True)
-                                if isinstance(c, GES.Clip)])
+                                      if isinstance(c, GES.Clip)])
 
         return clips.union(grouped_clips)
 
@@ -1538,7 +1538,7 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
         self.delete_action.set_enabled(selection_non_empty)
         self.delete_and_shift_action.set_enabled(selection_non_empty)
         self.group_action.set_enabled(selection.can_group)
-        self.ungroup_action.set_enabled(selection.can_ungroup and not selection.can_group)
+        self.ungroup_action.set_enabled(selection.can_ungroup)
         self.copy_action.set_enabled(selection_non_empty)
         can_paste = bool(self.__copied_group)
         self.paste_action.set_enabled(can_paste)
@@ -1767,9 +1767,9 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
                                                 
finalizing_action=CommitTimelineFinalizingAction(self._project.pipeline),
                                                 toplevel=True):
                     for clip in self.timeline.selection:
-                        layer = clip.get_layer()
                         if isinstance(clip, GES.TransitionClip):
                             continue
+                        layer = clip.get_layer()
                         layer.remove_clip(clip)
 
             self.timeline.selection.setSelection([], SELECT)
diff --git a/pitivi/utils/timeline.py b/pitivi/utils/timeline.py
index 33669884..e6174506 100644
--- a/pitivi/utils/timeline.py
+++ b/pitivi/utils/timeline.py
@@ -141,18 +141,14 @@ class Selection(GObject.Object, Loggable):
         self.emit("selection-changed")
 
     def set_can_group_ungroup(self):
-        containers = set()
-        for obj in self.selected:
-            toplevel = obj.get_toplevel_parent()
-            if not toplevel.props.serialize:
-                for child in toplevel.get_children(False):
-                    containers.add(child)
-            else:
-                containers.add(toplevel)
-            if len(containers) > 1:
+        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(containers) > 1
-        self.can_ungroup = len(self.getSelectedTrackElements()) > 1
+        self.can_group = len(toplevels) > 1
+        self.can_ungroup = can_ungroup and not self.can_group
 
     def __get_selection_changes(self, old_selection):
         for obj in old_selection - self.selected:
@@ -182,17 +178,6 @@ class Selection(GObject.Object, Loggable):
 
         return set(objects)
 
-    def getSelectedTrackElementsAtPosition(self, position, element_type=GObject.Object,
-                                           track_type=GES.TrackType.UNKNOWN):
-        selected = []
-        for clip in self.selected:
-            if clip.props.start <= position and position <= clip.props.start + clip.props.duration:
-                elements = clip.find_track_elements(None, track_type, element_type)
-                if elements:
-                    selected.extend(elements)
-
-        return selected
-
     def getSingleClip(self, clip_type=GES.SourceClip):
         """Returns the single-selected clip, if any.
 
@@ -205,6 +190,28 @@ class Selection(GObject.Object, Loggable):
                 return clip
         return None
 
+    @property
+    def toplevels(self):
+        """Returns the toplevel elements of the selection."""
+        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.update(self.__serializable_toplevels(toplevel))
+
+        return toplevels
+
+    def __serializable_toplevels(self, toplevel):
+        """Generates the specified element if serializable, or its children."""
+        if toplevel.props.serialize:
+            yield toplevel
+        else:
+            for element in toplevel.get_children(False):
+                for child in self.__serializable_toplevels(element):
+                    yield child
+
     def __len__(self):
         return len(self.selected)
 
diff --git a/tests/test_timeline_timeline.py b/tests/test_timeline_timeline.py
index 71926386..8580188c 100644
--- a/tests/test_timeline_timeline.py
+++ b/tests/test_timeline_timeline.py
@@ -27,7 +27,6 @@ from pitivi.utils.timeline import UNSELECT
 from pitivi.utils.ui import LAYER_HEIGHT
 from pitivi.utils.ui import SEPARATOR_HEIGHT
 from tests import common
-from tests.common import create_timeline_container
 
 THIN = LAYER_HEIGHT / 2
 THICK = LAYER_HEIGHT
@@ -87,7 +86,7 @@ class TestLayers(BaseTestTimeline):
                              [0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2])
 
     def checkGetLayerAt(self, heights, preferred, past_middle_when_adjacent, expectations):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
 
         # Allocate layers
@@ -146,7 +145,7 @@ class TestLayers(BaseTestTimeline):
         assertLayerAt(ges_layers[expectations[15]], h[0] + s + h[1] + s + h[2] - 1)
 
     def testSetSeparatorsPrelight(self):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         timeline.__on_separators = [mock.Mock()]
         timeline._setSeparatorsPrelight(False)
@@ -154,7 +153,7 @@ class TestLayers(BaseTestTimeline):
                          "The separators must be forgotten only in dragEnd()")
 
     def test_media_types(self):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
 
         ges_layer_1 = timeline.ges_timeline.append_layer()
@@ -194,7 +193,7 @@ class TestLayers(BaseTestTimeline):
         self.check_create_layer([0, 1, 2, 3], [0, 1, 2, 3])
 
     def check_create_layer(self, start_priorities, expected_priorities):
-        timeline = create_timeline_container().timeline
+        timeline = common.create_timeline_container().timeline
         ges_layers = []
         for priority in start_priorities:
             ges_layer = timeline.create_layer(priority)
@@ -249,7 +248,7 @@ class TestLayers(BaseTestTimeline):
         self.check_remove_layer([3, 2, 1])
 
     def check_remove_layer(self, removal_order):
-        timeline = create_timeline_container().timeline
+        timeline = common.create_timeline_container().timeline
 
         # Add layers to remove them later.
         ges_layers = []
@@ -276,7 +275,7 @@ class TestLayers(BaseTestTimeline):
         self.check_move_layer(4, 3, [0, 1, 2, 4, 3])
 
     def check_move_layer(self, from_priority, to_priority, expected_priorities):
-        timeline = create_timeline_container().timeline
+        timeline = common.create_timeline_container().timeline
 
         # Add layers to move them later.
         ges_layers = []
@@ -291,11 +290,11 @@ class TestLayers(BaseTestTimeline):
 class TestGrouping(BaseTestTimeline):
 
     def __check_can_group_ungroup(self, timeline_container, can_group, can_ungroup):
-        self.assertEqual(can_group, timeline_container.group_action.props.enabled)
-        self.assertEqual(can_ungroup, timeline_container.ungroup_action.props.enabled)
+        self.assertEqual(timeline_container.group_action.props.enabled, can_group)
+        self.assertEqual(timeline_container.ungroup_action.props.enabled, can_ungroup)
 
     def test_can_group_ungroup(self):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         self.__check_can_group_ungroup(timeline_container, False, False)
         ges_clip, = self.addClipsSimple(timeline, 1)
@@ -361,14 +360,14 @@ class TestGrouping(BaseTestTimeline):
         self.assertEqual(len(group.get_children(False)), len(clips))
 
     def testGroup(self):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clips = self.addClipsSimple(timeline, 2)
         self.group_clips(timeline_container, clips)
 
     def testGroupSelection(self):
         num_clips = 2
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clips = self.addClipsSimple(timeline, num_clips)
         self.group_clips(timeline_container, clips)
@@ -386,7 +385,7 @@ class TestGrouping(BaseTestTimeline):
 
     def testGroupUngroup(self):
         num_clips = 2
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clips = self.addClipsSimple(timeline, num_clips)
         self.group_clips(timeline_container, clips)
@@ -404,7 +403,7 @@ class TestGrouping(BaseTestTimeline):
     def testGroupSplittedClipAndSelectGroup(self):
         position = 5
 
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clips = self.addClipsSimple(timeline, 1)
         self.toggle_clip_selection(clips[0], expect_selected=True)
@@ -439,7 +438,7 @@ class TestGrouping(BaseTestTimeline):
         self.toggle_clip_selection(clips[1], expect_selected=True)
 
     def testUngroupClip(self):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         ges_clip, = self.addClipsSimple(timeline, 1)
 
@@ -472,7 +471,7 @@ class TestGrouping(BaseTestTimeline):
 
     def test_dragging_group_on_separator(self):
         # Create two clips on different layers and group them.
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clip1, = self.addClipsSimple(timeline, 1)
         layer1 = clip1.get_layer()
@@ -513,7 +512,7 @@ class TestGrouping(BaseTestTimeline):
 class TestCopyPaste(BaseTestTimeline):
 
     def copyClips(self, num_clips):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
 
         clips = self.addClipsSimple(timeline, num_clips)
@@ -576,7 +575,7 @@ class TestEditing(BaseTestTimeline):
 
     def test_trimming_on_layer_separator(self):
         # Create a clip
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         clip, = self.addClipsSimple(timeline, 1)
         layer = clip.get_layer()
@@ -624,7 +623,7 @@ class TestShiftSelection(BaseTestTimeline):
             self.assertEqual(clip.selected._selected, False)
 
     def __check_simple(self, left_click_also_seeks):
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         timeline.app.settings.leftClickAlsoSeeks = left_click_also_seeks
         ges_layer = timeline.ges_timeline.append_layer()
@@ -658,7 +657,7 @@ class TestShiftSelection(BaseTestTimeline):
 
     def __check_shift_selection_single_layer(self, left_click_also_seeks):
         """Checks group clips selection across a single layer."""
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         timeline.app.settings.leftClickAlsoSeeks = left_click_also_seeks
         ges_layer = timeline.ges_timeline.append_layer()
@@ -719,7 +718,7 @@ class TestShiftSelection(BaseTestTimeline):
 
     def __check_shift_selection_multiple_layers(self, left_click_also_seeks):
         """Checks group clips selection across multiple layers."""
-        timeline_container = create_timeline_container()
+        timeline_container = common.create_timeline_container()
         timeline = timeline_container.timeline
         timeline.app.settings.leftClickAlsoSeeks = left_click_also_seeks
         ges_layer1 = timeline.ges_timeline.append_layer()
diff --git a/tests/test_utils_timeline.py b/tests/test_utils_timeline.py
index 9d3a9288..40db10cc 100644
--- a/tests/test_utils_timeline.py
+++ b/tests/test_utils_timeline.py
@@ -27,6 +27,7 @@ from pitivi.utils.timeline import Selected
 from pitivi.utils.timeline import Selection
 from pitivi.utils.timeline import UNSELECT
 from tests import common
+from tests.test_timeline_timeline import BaseTestTimeline
 
 
 class TestSelected(common.TestCase):
@@ -42,7 +43,7 @@ class TestSelected(common.TestCase):
         self.assertFalse(selected)
 
 
-class TestSelection(common.TestCase):
+class TestSelection(BaseTestTimeline):
 
     def testBoolEvaluation(self):
         clip1 = mock.MagicMock()
@@ -81,14 +82,17 @@ class TestSelection(common.TestCase):
         self.assertIsNone(selection.getSingleClip(GES.TitleClip))
 
     def test_can_group_ungroup(self):
-        clip1 = common.create_test_clip(GES.UriClip)
-        clip2 = common.create_test_clip(GES.UriClip)
+        timeline_container = common.create_timeline_container()
+        timeline = timeline_container.timeline
+        clip1, clip2 = self.addClipsSimple(timeline, 2)
+
         selection = Selection()
-        self.assertFalse(selection)
+        self.assertFalse(selection.can_group)
+        self.assertFalse(selection.can_ungroup)
 
         selection.setSelection([clip1], SELECT)
-        self.assertFalse(selection.can_ungroup)
         self.assertFalse(selection.can_group)
+        self.assertTrue(selection.can_ungroup)
 
         selection.setSelection([clip2], SELECT_ADD)
         self.assertTrue(selection.can_group)
@@ -98,6 +102,36 @@ class TestSelection(common.TestCase):
         self.assertFalse(selection.can_group)
         self.assertFalse(selection.can_ungroup)
 
+    def test_toplevels(self):
+        timeline_container = common.create_timeline_container()
+        timeline = timeline_container.timeline
+        clip1, clip2, clip3, clip4 = self.addClipsSimple(timeline, 4)
+
+        selection = Selection()
+
+        selection.setSelection([clip1, clip2, clip3, clip4], SELECT)
+        self.assertSetEqual(selection.toplevels, {clip1, clip2, clip3, clip4})
+
+        group1 = GES.Container.group([clip1, clip2])
+        group1.props.serialize = True
+        self.assertSetEqual(selection.toplevels, {group1, clip3, clip4})
+
+        group2 = GES.Container.group([group1, clip3])
+        group2.props.serialize = True
+        self.assertSetEqual(selection.toplevels, {group2, clip4})
+
+        group1.props.serialize = True
+        group1.props.serialize = False
+        self.assertSetEqual(selection.toplevels, {group2, clip4})
+
+        group1.props.serialize = False
+        group2.props.serialize = False
+        self.assertSetEqual(selection.toplevels, {clip1, clip2, clip3, clip4})
+
+        group1.props.serialize = True
+        group2.props.serialize = False
+        self.assertSetEqual(selection.toplevels, {group1, clip3, clip4})
+
 
 class TestEditingContext(common.TestCase):
     """Tests for the EditingContext class."""