From 2c1e39f00e9f565600cf57129f121e6e4ada4a3c Mon Sep 17 00:00:00 2001
From: Olivier Goffart <ogoffart@sixtyfps.io>
Date: Tue, 17 Aug 2021 11:57:35 +0200
Subject: [PATCH] Fix priority when merging two_way_binding

We should not increase the priority when merging two way binding, only when
inlining.

This fixes the iot-dashboard's devices widget which were sometimes not
transparent as they should have been.
This was not deterministic because the order in which the two way binding are
merged is not deterministic because of hash table, and sometimes one binding
ended up having a higher priority as it should have had.
---
 sixtyfps_compiler/expression_tree.rs     | 29 ++++++++++++++----------
 sixtyfps_compiler/passes/inlining.rs     |  4 +++-
 tests/cases/bindings/two_way_priority.60 | 18 +++++++++++++++
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/sixtyfps_compiler/expression_tree.rs b/sixtyfps_compiler/expression_tree.rs
index a2e2f4d41..0f5d712b3 100644
--- a/sixtyfps_compiler/expression_tree.rs
+++ b/sixtyfps_compiler/expression_tree.rs
@@ -1120,37 +1120,42 @@ impl BindingExpression {
     /// Merge the other into this one. Normally, &self is kept intact (has priority), unless two ways binding are
     /// involved then they need to be merged.
     /// Also the animation is taken if the other don't have one
-    pub fn merge_with(&mut self, other: &Self) {
+    ///
+    /// Returns true if the other expression was taken
+    pub fn merge_with(&mut self, other: &Self) -> bool {
         fn maybe_merge_two_ways(
             binding: &mut Expression,
             priority: &mut i32,
             original: &BindingExpression,
-        ) {
+        ) -> bool {
             match (binding, &original.expression) {
                 (Expression::TwoWayBinding(_, Some(ref mut x)), _) => {
-                    maybe_merge_two_ways(&mut *x, priority, original);
+                    maybe_merge_two_ways(&mut *x, priority, original)
                 }
                 (Expression::TwoWayBinding(_, x), o) => {
-                    *priority = original.priority + 1;
+                    *priority = original.priority;
                     *x = Some(Box::new(o.clone()));
+                    true
                 }
                 (ref mut x, Expression::TwoWayBinding(nr, _)) => {
                     let n =
                         Expression::TwoWayBinding(nr.clone(), Some(Box::new(std::mem::take(*x))));
-                    **x = n
+                    **x = n;
+                    false
                 }
-                _ => *priority += 1,
+                _ => false,
             }
         }
-        if matches!(self.expression, Expression::Invalid) {
-            self.priority = other.priority + 1;
-            self.expression = other.expression.clone();
-        } else {
-            maybe_merge_two_ways(&mut self.expression, &mut self.priority, other);
-        }
         if self.animation.is_none() {
             self.animation = other.animation.clone();
         }
+        if matches!(self.expression, Expression::Invalid) {
+            self.priority = other.priority;
+            self.expression = other.expression.clone();
+            true
+        } else {
+            maybe_merge_two_ways(&mut self.expression, &mut self.priority, other)
+        }
     }
 }
 
diff --git a/sixtyfps_compiler/passes/inlining.rs b/sixtyfps_compiler/passes/inlining.rs
index bf86b3cc2..758a60a3b 100644
--- a/sixtyfps_compiler/passes/inlining.rs
+++ b/sixtyfps_compiler/passes/inlining.rs
@@ -114,7 +114,9 @@ fn inline_element(
                 entry.insert(val.clone()).priority += 1;
             }
             std::collections::btree_map::Entry::Occupied(mut entry) => {
-                entry.get_mut().merge_with(val);
+                if entry.get_mut().merge_with(val) {
+                    entry.get_mut().priority += 1;
+                }
             }
         }
     }
diff --git a/tests/cases/bindings/two_way_priority.60 b/tests/cases/bindings/two_way_priority.60
index fa534851b..92e1c2d84 100644
--- a/tests/cases/bindings/two_way_priority.60
+++ b/tests/cases/bindings/two_way_priority.60
@@ -8,6 +8,13 @@
     Please contact info@sixtyfps.io for more information.
 LICENSE END */
 
+export OpacityTwoWay := Rectangle {
+    property <float> sub_opacity <=> rect.opacity;
+    rect := Rectangle {
+        opacity: 0.75;
+    }
+}
+
 export Compo := Rectangle {
     property<int> foo <=> self.bar;
     property<int> bar: 120;
@@ -18,8 +25,16 @@ TestCase := Window {
     compo1 := Compo {}
     compo2 := Compo { foo: compo1.foo + 10; }
 
+    Rectangle {
+        otw := OpacityTwoWay { sub_opacity: 0.5; }
+        otw2 := OpacityTwoWay { sub_opacity: 0.5; }
+    }
+
     property <int> compo0_foo: compo0.foo;
     property <int> compo2_foo: compo2.foo;
+    property <float> otw_opacity <=> otw.sub_opacity;
+
+    property <bool> test: compo0_foo == 140 && compo2_foo == 130 && otw_opacity == 0.5 && otw2.sub_opacity == 0.5;
 }
 
 /*
@@ -28,6 +43,7 @@ TestCase := Window {
 let instance = TestCase::new();
 assert_eq!(instance.get_compo0_foo(), 140);
 assert_eq!(instance.get_compo2_foo(), 130);
+assert_eq!(instance.get_otw_opacity(), 0.5);
 ```
 
 
@@ -37,6 +53,7 @@ auto handle = TestCase::create();
 const TestCase &instance = *handle;
 assert_eq(instance.get_compo0_foo(), 140);
 assert_eq(instance.get_compo2_foo(), 130);
+assert_eq(instance.get_otw_opacity(), 0.5);
 ```
 
 
@@ -44,6 +61,7 @@ assert_eq(instance.get_compo2_foo(), 130);
 let instance = new sixtyfps.TestCase({});
 assert.equal(instance.compo0_foo, 140);
 assert.equal(instance.compo2_foo, 130);
+assert.equal(instance.otw_opacity, 0.5);
 ```
 
 */