Always keep the binding of the right in a two way binding

When having a binding such as
```
  foo <=> bar
```
The default value will always be the value of `bar` regardless what's
the value of foo.

This change of behavior is the only one that makes sense bacause if we
are having repeater or if, this will be a problem. eg:
```
property <xxx> bar;
if (some_cndition) : SomeElement {
   foo <=> bar;
}
```
Then we can't possibly take the default value of foo for the value of
bar since it depends on the condition. (and it is even worse in case of
repeater)

This is a change of behevior, this is why the tests have changed. The
cse of tests/cases/bindings/* were already covered by a warning since
0.3.0 so that should be fine. But the warning did not trigger for
builtin property such as `visible`  (eg, input/visible_mouse test)

Also some internal two way bindings had to be reversed.

cc: slint-ui#1394

internal/compiler/expression_tree.rs

@@ -1240,8 +1240,9 @@ impl BindingExpression {
         if self.animation.is_none() {
             self.animation = other.animation.clone();
         }
+        let has_binding = self.has_binding();
         self.two_way_bindings.extend_from_slice(&other.two_way_bindings);
-        if matches!(self.expression, Expression::Invalid) {
+        if !has_binding {
             self.priority = other.priority;
             self.expression = other.expression.clone();
             true

internal/compiler/passes/flickable.rs

@@ -42,29 +42,29 @@ pub fn handle_flickable(root_component: &Rc<Component>, tr: &TypeRegister) {
     )
 }
 
-fn create_viewport_element(flickable_elem: &ElementRc, native_empty: &Rc<NativeClass>) {
-    let mut flickable = flickable_elem.borrow_mut();
-    let flickable = &mut *flickable;
+fn create_viewport_element(flickable: &ElementRc, native_empty: &Rc<NativeClass>) {
+    let children = std::mem::take(&mut flickable.borrow_mut().children);
     let viewport = Rc::new(RefCell::new(Element {
-        id: format!("{}-viewport", flickable.id),
+        id: format!("{}-viewport", flickable.borrow().id),
         base_type: ElementType::Native(native_empty.clone()),
-        children: std::mem::take(&mut flickable.children),
-        enclosing_component: flickable.enclosing_component.clone(),
+        children,
+        enclosing_component: flickable.borrow().enclosing_component.clone(),
         is_flickable_viewport: true,
         ..Element::default()
     }));
-    for (prop, info) in &flickable.base_type.as_builtin().properties {
+    let element_type = flickable.borrow().base_type.clone();
+    for (prop, info) in &element_type.as_builtin().properties {
         if let Some(vp_prop) = prop.strip_prefix("viewport-") {
-            let nr = NamedReference::new(&viewport, vp_prop);
-            flickable.property_declarations.insert(prop.to_owned(), info.ty.clone().into());
-            match flickable.bindings.entry(prop.to_owned()) {
-                std::collections::btree_map::Entry::Occupied(entry) => {
-                    entry.into_mut().get_mut().two_way_bindings.push(nr);
-                }
-                std::collections::btree_map::Entry::Vacant(entry) => {
-                    entry.insert(BindingExpression::new_two_way(nr).into());
-                }
-            }
+            // materialize the viewport properties
+            flickable
+                .borrow_mut()
+                .property_declarations
+                .insert(prop.to_owned(), info.ty.clone().into());
+            // bind the viewport's property to the flickable property, such as:  `width <=> parent.viewport-width`
+            viewport.borrow_mut().bindings.insert(
+                vp_prop.to_owned(),
+                BindingExpression::new_two_way(NamedReference::new(flickable, prop)).into(),
+            );
         }
     }
     viewport
@@ -81,7 +81,7 @@ fn create_viewport_element(flickable_elem: &ElementRc, native_empty: &Rc<NativeC
         .entry("x".into())
         .or_default()
         .is_set_externally = true;
-    flickable.children.push(viewport);
+    flickable.borrow_mut().children.push(viewport);
 }
 
 fn fixup_geometry(flickable_elem: &ElementRc) {

internal/compiler/passes/remove_aliases.rs

@@ -4,12 +4,13 @@
 //! This pass removes the property used in a two ways bindings
 
 use crate::diagnostics::BuildDiagnostics;
-use crate::expression_tree::{BindingExpression, NamedReference};
+use crate::expression_tree::{BindingExpression, Expression, NamedReference};
 use crate::object_tree::*;
 use std::cell::RefCell;
 use std::collections::{btree_map::Entry, HashMap, HashSet};
 use std::rc::Rc;
 
+// The property in the key is to be removed, and replaced by the property in the value
 type Mapping = HashMap<NamedReference, NamedReference>;
 
 #[derive(Default, Debug)]
@@ -112,46 +113,37 @@ pub fn remove_aliases(component: &Rc<Component>, diag: &mut BuildDiagnostics) {
     // Remove the properties
     for (remove, to) in aliases_to_remove {
         let elem = remove.element();
+        let to_elem = to.element();
 
         // adjust the bindings
         let old_binding = elem.borrow_mut().bindings.remove(remove.name());
-        let must_simplify = if let Some(mut binding) = old_binding.map(RefCell::into_inner) {
-            remove_from_binding_expression(&mut binding, &to);
-            if binding.has_binding() {
-                let to_elem = to.element();
-                match to_elem.borrow_mut().bindings.entry(to.name().to_owned()) {
-                    Entry::Occupied(mut e) => {
-                        let b = e.get_mut().get_mut();
-                        remove_from_binding_expression(b, &to);
-                        if b.priority < binding.priority || !b.has_binding() {
-                            b.merge_with(&binding);
-                        } else {
-                            binding.merge_with(b);
-                            *b = binding;
-                        }
-                    }
-                    Entry::Vacant(e) => {
-                        e.insert(binding.into());
-                    }
-                };
-                false
-            } else {
-                true
+        let mut old_binding = old_binding.map(RefCell::into_inner).unwrap_or_else(|| {
+            // ensure that we set an expression, because the right hand side of a binding always wins,
+            // and if that was not set, we must still kee the default then
+            let mut b = BindingExpression::from(Expression::default_value_for_type(&to.ty()));
+            b.priority =
+                to_elem.borrow_mut().bindings.get(to.name()).map_or(0, |x| x.borrow().priority);
+            b
+        });
+
+        remove_from_binding_expression(&mut old_binding, &to);
+        match to_elem.borrow_mut().bindings.entry(to.name().to_owned()) {
+            Entry::Occupied(mut e) => {
+                let b = e.get_mut().get_mut();
+                remove_from_binding_expression(b, &to);
+                if b.priority <= old_binding.priority || !b.has_binding() {
+                    b.merge_with(&old_binding);
+                } else {
+                    old_binding.merge_with(b);
+                    *b = old_binding;
+                }
             }
-        } else {
-            true
-        };
-
-        if must_simplify {
-            let to_elem = to.element();
-            let mut to_elem = to_elem.borrow_mut();
-            if let Some(b) = to_elem.bindings.get_mut(to.name()) {
-                remove_from_binding_expression(b.get_mut(), &to);
-                if !b.get_mut().has_binding() {
-                    to_elem.bindings.remove(to.name());
+            Entry::Vacant(e) => {
+                if old_binding.has_binding() {
+                    e.insert(old_binding.into());
                 }
             }
-        }
+        };
 
         // Remove the declaration
         {

tests/cases/bindings/two_way_binding.slint

@@ -82,9 +82,9 @@ assert_eq!(instance.get_ti1_text(), slint::SharedString::from("Hallo"));
 assert_eq!(instance.get_ti2_text(), slint::SharedString::from("Bonjour"));
 assert_eq!(instance.get_text_item_text(), slint::SharedString::from("Bonjour"));
 
-assert_eq!(instance.get_othercomp_some_value(), 42);
-assert_eq!(instance.get_othercomp_some_value_alias(), 42);
-assert_eq!(instance.get_othercomp_some_value_alias2(), 42);
+assert_eq!(instance.get_othercomp_some_value(), 0);
+assert_eq!(instance.get_othercomp_some_value_alias(), 0);
+assert_eq!(instance.get_othercomp_some_value_alias2(), 0);
 instance.set_othercomp_some_value(88);
 assert_eq!(instance.get_othercomp_some_value(), 88);
 assert_eq!(instance.get_othercomp_some_value_alias(), 88);
@@ -131,9 +131,9 @@ assert_eq(instance.get_ti1_text(), slint::SharedString("Hallo"));
 assert_eq(instance.get_ti2_text(), slint::SharedString("Bonjour"));
 assert_eq(instance.get_text_item_text(), slint::SharedString("Bonjour"));
 
-assert_eq(instance.get_othercomp_some_value(), 42);
-assert_eq(instance.get_othercomp_some_value_alias(), 42);
-assert_eq(instance.get_othercomp_some_value_alias2(), 42);
+assert_eq(instance.get_othercomp_some_value(), 0);
+assert_eq(instance.get_othercomp_some_value_alias(), 0);
+assert_eq(instance.get_othercomp_some_value_alias2(), 0);
 instance.set_othercomp_some_value(88);
 assert_eq(instance.get_othercomp_some_value(), 88);
 assert_eq(instance.get_othercomp_some_value_alias(), 88);
@@ -176,9 +176,9 @@ assert.equal(instance.ti1_text, "Hallo");
 assert.equal(instance.ti2_text, "Bonjour");
 assert.equal(instance.text_item_text, "Bonjour");
 
-assert.equal(instance.othercomp_some_value, 42);
-assert.equal(instance.othercomp_some_value_alias, 42);
-assert.equal(instance.othercomp_some_value_alias2, 42);
+assert.equal(instance.othercomp_some_value, 0);
+assert.equal(instance.othercomp_some_value_alias, 0);
+assert.equal(instance.othercomp_some_value_alias2, 0);
 instance.othercomp_some_value = 88;
 assert.equal(instance.othercomp_some_value, 88);
 assert.equal(instance.othercomp_some_value_alias, 88);

tests/cases/bindings/two_way_binding2.slint

@@ -85,9 +85,9 @@ assert_eq!(instance.get_ti1_text(), slint::SharedString::from("Hallo"));
 assert_eq!(instance.get_ti2_text(), slint::SharedString::from("Bonjour"));
 assert_eq!(instance.get_text_item_text(), slint::SharedString::from("Bonjour"));
 
-assert_eq!(instance.get_othercomp_some_value(), 42);
-assert_eq!(instance.get_othercomp_some_value_alias(), 42);
-assert_eq!(instance.get_othercomp_some_value_alias2(), 42);
+assert_eq!(instance.get_othercomp_some_value(), 0);
+assert_eq!(instance.get_othercomp_some_value_alias(), 0);
+assert_eq!(instance.get_othercomp_some_value_alias2(), 0);
 instance.set_othercomp_some_value(88);
 assert_eq!(instance.get_othercomp_some_value(), 88);
 assert_eq!(instance.get_othercomp_some_value_alias(), 88);
@@ -134,9 +134,9 @@ assert_eq(instance.get_ti1_text(), slint::SharedString("Hallo"));
 assert_eq(instance.get_ti2_text(), slint::SharedString("Bonjour"));
 assert_eq(instance.get_text_item_text(), slint::SharedString("Bonjour"));
 
-assert_eq(instance.get_othercomp_some_value(), 42);
-assert_eq(instance.get_othercomp_some_value_alias(), 42);
-assert_eq(instance.get_othercomp_some_value_alias2(), 42);
+assert_eq(instance.get_othercomp_some_value(), 0);
+assert_eq(instance.get_othercomp_some_value_alias(), 0);
+assert_eq(instance.get_othercomp_some_value_alias2(), 0);
 instance.set_othercomp_some_value(88);
 assert_eq(instance.get_othercomp_some_value(), 88);
 assert_eq(instance.get_othercomp_some_value_alias(), 88);
@@ -179,9 +179,9 @@ assert.equal(instance.ti1_text, "Hallo");
 assert.equal(instance.ti2_text, "Bonjour");
 assert.equal(instance.text_item_text, "Bonjour");
 
-assert.equal(instance.othercomp_some_value, 42);
-assert.equal(instance.othercomp_some_value_alias, 42);
-assert.equal(instance.othercomp_some_value_alias2, 42);
+assert.equal(instance.othercomp_some_value, 0);
+assert.equal(instance.othercomp_some_value_alias, 0);
+assert.equal(instance.othercomp_some_value_alias2, 0);
 instance.othercomp_some_value = 88;
 assert.equal(instance.othercomp_some_value, 88);
 assert.equal(instance.othercomp_some_value_alias, 88);

tests/cases/input/visible_mouse.slint

@@ -23,7 +23,7 @@ TestCase := Rectangle {
     width: 100phx;
     property <int> touch1 <=> el1.touch;
     property <int> touch2 <=> el2.touch;
-    property <bool> el1visible;
+    property <bool> el1visible : true;
 
     el1 := MaybeVisible {
         visible <=> el1visible;