[pitivi] effects: Handle dependent property update

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

commit 8da9ca9cd16b16c210c850e0cd998c7bf0ce6555
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Fri Jan 25 17:54:39 2019 +0100

    effects: Handle dependent property update
    
    When EffectsPropertiesManager._on_widget_value_changed_cb sets the
    effect property and this updates a dependent property, its widget is
    updated and we should ignore that.
    
    Fixes #2274

 pitivi/effects.py       | 44 +++++++++++++++++++++++++-------------------
 pitivi/utils/widgets.py |  9 ++++-----
 tests/test_effects.py   | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 24 deletions(-)
---
diff --git a/pitivi/effects.py b/pitivi/effects.py
index db2114cb..ec5220da 100644
--- a/pitivi/effects.py
+++ b/pitivi/effects.py
@@ -685,26 +685,32 @@ class EffectsPropertiesManager(GObject.Object, Loggable):
             return
         self.debug('Additional properties successfully set for "%s"', effect_name)
 
-    def _connectAllWidgetCallbacks(self, effect_settings_widget, effect):
-        for prop, widget in effect_settings_widget.properties.items():
-            widget.connectValueChanged(self._onValueChangedCb, widget, prop, effect)
-
-    def _onSetDefaultCb(self, unused_widget, effect_widget):
-        effect_widget.setWidgetToDefault()
-
-    def _onValueChangedCb(self, unused_widget, effect_widget, prop, effect):
-        value = effect_widget.getWidgetValue()
-
-        # FIXME Workaround in order to make aspectratiocrop working
-        if isinstance(value, Gst.Fraction):
-            value = Gst.Fraction(int(value.num), int(value.denom))
+    def _connectAllWidgetCallbacks(self, effect_widget, effect):
+        for prop, widget in effect_widget.properties.items():
+            widget.connectValueChanged(self._on_widget_value_changed_cb, widget, prop, effect, effect_widget)
+
+    def _on_widget_value_changed_cb(self, unused_widget, prop_widget, prop, effect, effect_widget):
+        if effect_widget.updating_property:
+            # The widget is updated as a side-effect of setting one of its
+            # properties. Ignore.
+            return
 
-        from pitivi.undo.timeline import CommitTimelineFinalizingAction
-        pipeline = self.app.project_manager.current_project.pipeline
-        with self.app.action_log.started("Effect property change",
-                                         finalizing_action=CommitTimelineFinalizingAction(pipeline),
-                                         toplevel=True):
-            effect.set_child_property(prop.name, value)
+        effect_widget.updating_property = True
+        try:
+            value = prop_widget.getWidgetValue()
+
+            # FIXME Workaround in order to make aspectratiocrop working
+            if isinstance(value, Gst.Fraction):
+                value = Gst.Fraction(int(value.num), int(value.denom))
+
+            from pitivi.undo.timeline import CommitTimelineFinalizingAction
+            pipeline = self.app.project_manager.current_project.pipeline
+            with self.app.action_log.started("Effect property change",
+                                             finalizing_action=CommitTimelineFinalizingAction(pipeline),
+                                             toplevel=True):
+                effect.set_child_property(prop.name, value)
+        finally:
+            effect_widget.updating_property = False
 
     def create_property_widget(self, element_settings_widget, prop, prop_value):
         prop_widget = self.emit("create_property_widget", element_settings_widget, 
element_settings_widget.element,
diff --git a/pitivi/utils/widgets.py b/pitivi/utils/widgets.py
index 5d695ddc..af2104c0 100644
--- a/pitivi/utils/widgets.py
+++ b/pitivi/utils/widgets.py
@@ -38,11 +38,8 @@ from pitivi.utils.misc import is_valid_file
 from pitivi.utils.timeline import Zoomable
 from pitivi.utils.ui import beautify_length
 from pitivi.utils.ui import disable_scroll
-from pitivi.utils.ui import pack_color_32
-from pitivi.utils.ui import pack_color_64
 from pitivi.utils.ui import SPACING
 from pitivi.utils.ui import time_to_string
-from pitivi.utils.ui import unpack_color
 
 
 ZOOM_SLIDER_PADDING = SPACING * 4 / 5
@@ -435,8 +432,9 @@ class FractionWidget(TextWidget, DynamicWidget):
             return self._parseText(self.last_valid)
         return Gst.Fraction(1, 1)
 
-    def _parseText(self, text):
-        match = self.fraction_regex.match(text)
+    @classmethod
+    def _parseText(cls, text):
+        match = cls.fraction_regex.match(text)
         groups = match.groups()
         num = 1.0
         denom = 1.0
@@ -690,6 +688,7 @@ class GstElementSettingsWidget(Gtk.Box, Loggable):
         self.__widgets_by_reset_button = {}
         self._unhandled_properties = []
         self.uncontrolled_properties = {}
+        self.updating_property = False
 
     def deactivate_keyframe_toggle_buttons(self):
         """Makes sure the keyframe togglebuttons are deactivated."""
diff --git a/tests/test_effects.py b/tests/test_effects.py
index 35ebaa72..91c5e2eb 100644
--- a/tests/test_effects.py
+++ b/tests/test_effects.py
@@ -22,6 +22,7 @@
 import os
 
 from gi.repository import GES
+from gi.repository import Gst
 from gi.repository import Gtk
 
 from pitivi.effects import AUDIO_EFFECT
@@ -172,3 +173,39 @@ class EffectsPropertiesManagerTest(common.TestCase):
         _, prop_value = self.alpha_effect.get_child_property(self.prop_name)
         self.assertEqual(self.prop.default_value, prop_value)
         self.assertEqual(self.prop.default_value, wrapped_spin_button.getWidgetValue())
+
+    def test_dependent_properties(self):
+        """Checks dependent properties updating is handled correctly."""
+        mainloop = common.create_main_loop()
+        app = common.create_pitivi()
+        app.project_manager.new_blank_project()
+        manager = EffectsPropertiesManager(app)
+
+        called = False
+
+        def set_child_property(prop_name, value):
+            nonlocal called
+            called = True
+
+            self.assertEqual(prop_name, "aspect-ratio")
+            GES.Effect.set_child_property(effect, prop_name, value)
+
+            # When setting the aspect-ratio property, and the stars align,
+            # the effect also changes the left/right properties.
+            # Here we simulate the updating of the dependent properties.
+            GES.Effect.set_child_property(effect, "left", 100)
+            GES.Effect.set_child_property(effect, "right", 100)
+
+        effect = GES.Effect.new("aspectratiocrop")
+        effect.set_child_property = set_child_property
+
+        effect_widget = manager.getEffectConfigurationUI(effect)
+
+        widgets = {prop.name: widget
+                   for prop, widget in effect_widget.properties.items()}
+        # Simulate the user choosing an aspect-ratio.
+        widgets["aspect-ratio"].setWidgetValue(Gst.Fraction(4, 3))
+
+        mainloop.run(until_empty=True)
+
+        self.assertTrue(called)