[pitivi] project_: Keep the assets undoable actions as the user expects

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

commit 675e06983bde50670a31604abaf4133b4ae263ba
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Tue Dec 20 01:05:28 2016 +0100

    project_: Keep the assets undoable actions as the user expects
    
    We were starting an undoable-action-log transaction when a new asset was
    added to the project. This was very ugly because by the time that
    transaction was closed, the user was able to make other operations on
    the timeline and those were included in the initial assets-added
    transaction.
    
    This got worse with the proxy functionality since that transaction can
    now take a very long time until complete, which means the undo/redo
    stack was easily messed up.
    
    Taking advantage of the fact that it does not really matter when the
    assets are actually added to the project, we now record the intention of
    adding them, or the intention of proxying them. Then when they are ready
    to be used, the user can use them and all should be fine.
    
    Fixes https://phabricator.freedesktop.org/T7531
    
    Reviewed-by: Thibault Saunier <tsaunier gnome org>
    Differential Revision: https://phabricator.freedesktop.org/D1575

 pitivi/project.py          |   30 ++++++++++---------
 pitivi/undo/project.py     |   70 +++++++++++++++++++++++++++++++++++++++----
 pitivi/undo/timeline.py    |    6 ++--
 pitivi/undo/undo.py        |   43 +++++++++++++++------------
 tests/test_undo.py         |    8 ++--
 tests/test_undo_project.py |   38 ++++++++++++++----------
 6 files changed, 132 insertions(+), 63 deletions(-)
---
diff --git a/pitivi/project.py b/pitivi/project.py
index 24b63c0..7123102 100644
--- a/pitivi/project.py
+++ b/pitivi/project.py
@@ -36,6 +36,8 @@ from pitivi.configure import get_ui_dir
 from pitivi.preset import AudioPresetManager
 from pitivi.preset import VideoPresetManager
 from pitivi.render import Encoders
+from pitivi.undo.project import AssetAddedIntention
+from pitivi.undo.project import AssetProxiedIntention
 from pitivi.utils.loggable import Loggable
 from pitivi.utils.misc import isWritable
 from pitivi.utils.misc import path_from_uri
@@ -986,7 +988,6 @@ class Project(Loggable, GES.Project):
 
     def __updateAssetLoadingProgress(self, estimated_time=0):
         if not self.loading_assets:
-            self.app.action_log.commit("Adding assets")
             self.emit("asset-loading-progress", 100, estimated_time)
             return
 
@@ -1181,16 +1182,17 @@ class Project(Loggable, GES.Project):
     def use_proxies_for_assets(self, assets):
         originals = []
         for asset in assets:
-            proxy_target = asset.get_proxy_target()
-            if not proxy_target:
+            if not asset.get_proxy_target():
                 # The asset is not a proxy.
                 originals.append(asset)
         if originals:
-            self.app.action_log.begin("Adding assets")
-            for asset in originals:
-                self._prepare_asset_processing(asset)
-                asset.force_proxying = True
-                self.app.proxy_manager.add_job(asset)
+            with self.app.action_log.started("Proxying assets"):
+                for asset in originals:
+                    action = AssetProxiedIntention(asset, self, self.app.proxy_manager)
+                    self.app.action_log.push(action)
+                    self._prepare_asset_processing(asset)
+                    asset.force_proxying = True
+                    self.app.proxy_manager.add_job(asset)
 
     def disable_proxies_for_assets(self, assets, delete_proxy_file=False):
         for asset in assets:
@@ -1211,9 +1213,6 @@ class Project(Loggable, GES.Project):
                 # The asset is an original which is not being proxied.
                 self.app.proxy_manager.cancel_job(asset)
 
-        if assets:
-            self.setModificationState(True)
-
     def hasDefaultName(self):
         return DEFAULT_NAME == self.name
 
@@ -1272,9 +1271,12 @@ class Project(Loggable, GES.Project):
         Args:
             uris (List[str]): The URIs of the assets.
         """
-        self.app.action_log.begin("Adding assets")
-        for uri in uris:
-            self.create_asset(quote_uri(uri), GES.UriClip)
+        with self.app.action_log.started("Adding assets"):
+            for uri in uris:
+                if self.create_asset(quote_uri(uri), GES.UriClip):
+                    # The asset was not already part of the project.
+                    action = AssetAddedIntention(self, uri)
+                    self.app.action_log.push(action)
 
     def assetsForUris(self, uris):
         assets = []
diff --git a/pitivi/undo/project.py b/pitivi/undo/project.py
index 6296e04..8c45e19 100644
--- a/pitivi/undo/project.py
+++ b/pitivi/undo/project.py
@@ -21,22 +21,50 @@ from gi.repository import GObject
 from gi.repository import Gst
 
 from pitivi.undo.timeline import TimelineObserver
+from pitivi.undo.undo import Action
 from pitivi.undo.undo import MetaContainerObserver
 from pitivi.undo.undo import UndoableAction
 
 
-class AssetAddedAction(UndoableAction):
+class AssetAddedIntention(UndoableAction):
+    """The intention of adding an asset to a project.
 
-    def __init__(self, project, asset):
+    This should be created when the async operation starts.
+    See also AssetAddedAction.
+    """
+
+    def __init__(self, project, uri):
         UndoableAction.__init__(self)
         self.project = project
-        self.asset = asset
+        self.uri = uri
+        self.asset = None
+        self.project.connect("asset-added", self._asset_added_cb)
+
+    def _asset_added_cb(self, project, asset):
+        if asset.get_id() == self.uri:
+            self.asset = asset
+            self.project.disconnect_by_func(self._asset_added_cb)
 
     def undo(self):
-        self.project.remove_asset(self.asset)
+        # The asset might be missing if removed before it's added
+        if self.asset:
+            self.project.remove_asset(self.asset)
 
     def do(self):
-        self.project.add_asset(self.asset)
+        if self.asset:
+            self.project.add_asset(self.asset)
+
+
+class AssetAddedAction(Action):
+    """The adding of an asset to a project.
+
+    This should be created when the asset has been added.
+    See also AssetAddedIntention.
+    """
+
+    def __init__(self, asset):
+        Action.__init__(self)
+        self.asset = asset
 
     def asScenarioAction(self):
         st = Gst.Structure.new_empty("add-asset")
@@ -47,6 +75,7 @@ class AssetAddedAction(UndoableAction):
 
 
 class AssetRemovedAction(UndoableAction):
+    """The removal of an asset from a project."""
 
     def __init__(self, project, asset):
         UndoableAction.__init__(self)
@@ -67,6 +96,33 @@ class AssetRemovedAction(UndoableAction):
         return st
 
 
+class AssetProxiedIntention(UndoableAction):
+    """The intention of proxying an asset.
+
+    Attributes:
+        asset (GES.Asset): The original asset to be proxied.
+        proxy_manager (pitivi.utils.proxy.ProxyManager): The manager
+            controlling the proxy generation.
+    """
+
+    def __init__(self, asset, project, proxy_manager):
+        UndoableAction.__init__(self)
+        self.asset = asset
+        self.project = project
+        self.proxy_manager = proxy_manager
+
+    def do(self):
+        self.asset.force_proxying = True
+        self.proxy_manager.add_job(self.asset)
+
+    def undo(self):
+        self.proxy_manager.cancel_job(self.asset)
+        proxy = self.asset.props.proxy
+        self.asset.set_proxy(None)
+        if proxy:
+            self.project.remove_asset(proxy)
+
+
 class ProjectObserver(MetaContainerObserver):
     """Monitors a project instance and reports UndoableActions.
 
@@ -81,10 +137,10 @@ class ProjectObserver(MetaContainerObserver):
         self.timeline_observer = TimelineObserver(project.ges_timeline,
                                                   action_log)
 
-    def _assetAddedCb(self, project, asset):
+    def _assetAddedCb(self, unused_project, asset):
         if not isinstance(asset, GES.UriClipAsset):
             return
-        action = AssetAddedAction(project, asset)
+        action = AssetAddedAction(asset)
         self.action_log.push(action)
 
     def _assetRemovedCb(self, project, asset):
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index fc317cc..c6823ca 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -21,11 +21,11 @@ from gi.repository import GObject
 from gi.repository import Gst
 
 from pitivi.effects import PROPS_TO_IGNORE
+from pitivi.undo.undo import Action
 from pitivi.undo.undo import ExpandableUndoableAction
 from pitivi.undo.undo import FinalizingAction
 from pitivi.undo.undo import GObjectObserver
 from pitivi.undo.undo import MetaContainerObserver
-from pitivi.undo.undo import SimpleUndoableAction
 from pitivi.undo.undo import UndoableAction
 from pitivi.undo.undo import UndoableAutomaticObjectAction
 from pitivi.utils.loggable import Loggable
@@ -543,10 +543,10 @@ class KeyframeChangedAction(UndoableAction):
         self.control_source.set(time, value)
 
 
-class ControlSourceSetAction(SimpleUndoableAction):
+class ControlSourceSetAction(Action):
 
     def __init__(self, action_info):
-        SimpleUndoableAction.__init__(self)
+        Action.__init__(self)
         self.action_info = action_info
 
     def asScenarioAction(self):
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index c5dcef2..5483154 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -34,26 +34,30 @@ class UndoWrongStateError(UndoError):
     pass
 
 
-class UndoableAction(GObject.Object, Loggable):
+class Action(GObject.Object, Loggable):
+    """Something which might worth logging in a scenario."""
+
+    def __init__(self):
+        GObject.Object.__init__(self)
+        Loggable.__init__(self)
+
+    def asScenarioAction(self):
+        raise NotImplementedError()
+
+
+class UndoableAction(Action):
     """An action that can be undone.
 
     When your object's state changes, create an UndoableAction to allow
     reverting the change later on.
     """
 
-    def __init__(self):
-        GObject.Object.__init__(self)
-        Loggable.__init__(self)
-
     def do(self):
         raise NotImplementedError()
 
     def undo(self):
         raise NotImplementedError()
 
-    def asScenarioAction(self):
-        raise NotImplementedError()
-
 
 class UndoableAutomaticObjectAction(UndoableAction):
     """An action on an automatically created object.
@@ -90,7 +94,7 @@ class UndoableAutomaticObjectAction(UndoableAction):
             cls.__updates[other] = new_auto_object
 
 
-class ExpandableUndoableAction(GObject.Object, Loggable):
+class ExpandableUndoableAction(UndoableAction):
     """An action which can include immediately following actions."""
 
     def expand(self, action):
@@ -106,15 +110,6 @@ class ExpandableUndoableAction(GObject.Object, Loggable):
         raise NotImplementedError()
 
 
-class SimpleUndoableAction(UndoableAction):
-
-    def do(self):
-        pass
-
-    def undo(self):
-        pass
-
-
 class FinalizingAction:
     """Base class for actions applied when an undo or redo is performed."""
 
@@ -217,9 +212,19 @@ class UndoableActionLog(GObject.Object, Loggable):
         self.emit("begin", stack)
 
     def push(self, action):
-        """Adds an action to the current operation."""
+        """Reports a change.
+
+        Args:
+            action (Action): The action representing the change.
+                If it's an UndoableAction, it's added to the current
+                operation, if any.
+        """
         self.emit("pre-push", action)
 
+        if not isinstance(action, UndoableAction):
+            # Nothing else to do with it.
+            return
+
         if self.running:
             self.debug("Ignore push because running: %s", action)
             return
diff --git a/tests/test_undo.py b/tests/test_undo.py
index 68a4ed9..c2d82d9 100644
--- a/tests/test_undo.py
+++ b/tests/test_undo.py
@@ -148,7 +148,7 @@ class TestUndoableActionLog(TestCase):
         self.assertTrue(self.log.is_in_transaction())
 
         self.assertEqual(self.log.undo_stacks, [])
-        self.log.push(mock.Mock())
+        self.log.push(mock.Mock(spec=UndoableAction))
         self.log.commit("meh")
         self.assertEqual(len(self.signals), 3)
         name, (stack, action) = self.signals[1]
@@ -184,7 +184,7 @@ class TestUndoableActionLog(TestCase):
         self.assertTrue(self.log.is_in_transaction())
 
         self.assertEqual(self.log.undo_stacks, [])
-        self.log.push(mock.Mock())
+        self.log.push(mock.Mock(spec=UndoableAction))
         self.log.commit("nested")
         self.assertEqual(len(self.signals), 4)
         name, (stack, action) = self.signals[2]
@@ -208,9 +208,9 @@ class TestUndoableActionLog(TestCase):
         action1 = mock.Mock()
         action2 = mock.Mock()
         with self.log.started("one", finalizing_action=action1):
-            self.log.push(mock.Mock())
+            self.log.push(mock.Mock(spec=UndoableAction))
             with self.log.started("two", finalizing_action=action2):
-                self.log.push(mock.Mock())
+                self.log.push(mock.Mock(spec=UndoableAction))
         action1.do.assert_called_once_with()
         # For now, we call the finalizing action only for the top stack.
         action2.do.assert_not_called()
diff --git a/tests/test_undo_project.py b/tests/test_undo_project.py
index a556e1b..9141dc7 100644
--- a/tests/test_undo_project.py
+++ b/tests/test_undo_project.py
@@ -48,20 +48,20 @@ class TestProjectUndo(TestCase):
         self.assertFalse(self.action_log.undo_stacks)
 
     def test_asset_added(self):
+        uris = [common.get_sample_uri("tears_of_steel.webm")]
         mainloop = common.create_main_loop()
 
-        def commit_cb(unused_action_log, stack):
-            self.assertEqual(stack.action_group_name, "Adding assets")
-            mainloop.quit()
-
-        self.action_log.connect("commit", commit_cb)
-
         def loaded_cb(unused_project, unused_timeline):
-            uris = [common.get_sample_uri("tears_of_steel.webm")]
             self.project.addUris(uris)
 
         self.project.connect_after("loaded", loaded_cb)
 
+        def progress_cb(unused_project, progress, unused_estimated_time):
+            if progress == 100:
+                mainloop.quit()
+
+        self.project.connect_after("asset-loading-progress", progress_cb)
+
         mainloop.run()
 
         self.assertEqual(len(self.project.list_assets(GES.Extractable)), 1)
@@ -75,17 +75,19 @@ class TestProjectUndo(TestCase):
         uris = [common.get_sample_uri("tears_of_steel.webm")]
         mainloop = common.create_main_loop()
 
-        def commit_cb(unused_action_log, stack):
-            self.assertEqual(stack.action_group_name, "Adding assets")
-            mainloop.quit()
-        self.action_log.connect("commit", commit_cb)
-
         def loaded_cb(unused_project, unused_timeline):
+            # The new project has been loaded, add some assets.
             self.project.addUris(uris)
         self.project.connect_after("loaded", loaded_cb)
 
+        def progress_cb(unused_project, progress, unused_estimated_time):
+            if progress == 100:
+                # The assets have been loaded.
+                mainloop.quit()
+        self.project.connect_after("asset-loading-progress", progress_cb)
+
         mainloop.run()
-        self.action_log.disconnect_by_func(commit_cb)
+        self.project.disconnect_by_func(progress_cb)
         self.assertEqual(len(self.project.list_assets(GES.Extractable)), 1)
 
         # Make sure the asset is not a proxy.
@@ -101,18 +103,22 @@ class TestProjectUndo(TestCase):
         self.app.proxy_manager.connect("error-preparing-asset", error_cb)
 
         def proxy_ready_cb(proxy_manager, asset, proxy):
-            uris.remove(asset.props.id)
-            if not uris:
-                mainloop.quit()
+            mainloop.quit()
         self.app.proxy_manager.connect("proxy-ready", proxy_ready_cb)
 
         self.project.use_proxies_for_assets(assets)
         mainloop.run()
 
         self.assertEqual(len(self.project.list_assets(GES.Extractable)), 2)
+
+        # Undo proxying.
         self.action_log.undo()
         self.assertEqual(len(self.project.list_assets(GES.Extractable)), 1)
+
+        # Redo proxying.
         self.action_log.redo()
+        # Wait for the proxy to be signalled as ready.
+        mainloop.run()
         self.assertEqual(len(self.project.list_assets(GES.Extractable)), 2)
 
     def test_project_settings(self):