Repository: slint-ui/slint (language: rust)

Issue:
Dirty model property shouldn't cause re-creation of all elements in a `for`
When the model is marked as dirty, it will cause all elements to be re-created. 
Ideally, it shouldn't be re-created if the model didn't actually change

## Original description

### Bug Description

Animations don't trigger from state changes if they're on a component rendered inside a loop over a model where the model is a field in the same struct as the field whose change triggered the state change.

I would expect the window to fade to green when it is clicked, instead it snaps to green. (The bug doesn't occur if the two fields are in separate structs)

You can reproduce it by running `slint-viewer states.slint`, and clicking on the window.

### Reproducible Code

```slint
export component App {
    width: 120px;
    height: 120px;
    property<{on:bool, arr:[int]}> p: { arr: [1] };
    TouchArea {
        clicked =>  {p.on = !p.on}
        for x[i] in p.arr: Rectangle { 
            states [
                bright when p.on: {
                    background: #90f020;
                    in  {
                        animate background {duration: 300ms;}
                    }
                }
            ]
        }
    }
}
```


### Environment Details

- Slint Version: 1.9.1  (was also present in 1.8.x)
- Platform/OS: MacOS Sequoia (15.2)
- Programming Language: Rust
- Backend/Renderer: winit


### Product Impact

I'm currently working on a retrocomputing music editor (specifically a SID tracker), hobby project, likely freeware.
Mild cosmetic issue, noncritical (I can work around the bug by wrapping my list selection component in a dummy component that breaks up the {list,selected_id} pair).


Discussion / hints:
Thanks for the report and reduction.

AFAICS the issue is that `on` and `arr` are fields of the struct in the same property. It seems as if the fields act like separate properties that can be independently changed, but that's not the case. For a `property` holding a structure, any change to a field results in the entire structure being exchanged, and thus after `p.on = !p.on;`, the `p.arr` expression will also be re-evaluated, which means the `Rectangle` elements in the for loop are also re-created. So there's never a transition on an existing `Rectangle`.

Differently put:

`p.on = !p.on;` is more like `p = { on: !p.on; arr: p.arr; }` and thus an entirely "new" `p` is written.

I'm not sure if or how we can fix that. Seeking @ogoffart 's opinion before completing triaging.
Ah! That makes sense.  Ok, that gives me a better idea of how best to restructure my code if this turns out to be a "won't fix."

Thanks again, and I'll watch this space.


@tronical is correct.
The model is marked as dirty so everything is re-created, even if in that case the model did not change. This is similar to this other issue: https://github.com/slint-ui/slint/issues/3953

This could be mitigated with something like this: https://github.com/slint-ui/slint/pull/1954 but this was reverted for good reason. But we could make that when the model actually didn't change (eg, pointer is the same value) then we don't re-create the elements
>  But we could make that when the model actually didn't change (eg, pointer is the same value) then we don't re-create the elements

That's a neat idea :)

Fix patch:
diff --git a/api/cpp/include/slint.h b/api/cpp/include/slint.h
index 59a38a9efd8..6d0eaae3c62 100644
--- a/api/cpp/include/slint.h
+++ b/api/cpp/include/slint.h
@@ -1146,10 +1146,14 @@ class Repeater
     void ensure_updated(const Parent *parent) const
     {
         if (model.is_dirty()) {
-            inner = std::make_shared<RepeaterInner>();
-            if (auto m = model.get()) {
-                inner->model = m;
-                m->attach_peer(inner);
+            auto old_model = model.get_internal();
+            auto m = model.get();
+            if (!inner || old_model != m) {
+                inner = std::make_shared<RepeaterInner>();
+                if (m) {
+                    inner->model = m;
+                    m->attach_peer(inner);
+                }
             }
         }
 
diff --git a/api/cpp/include/slint_properties.h b/api/cpp/include/slint_properties.h
index d880d77eed2..6dfd193b47b 100644
--- a/api/cpp/include/slint_properties.h
+++ b/api/cpp/include/slint_properties.h
@@ -184,6 +184,8 @@ struct Property
     {
     }
 
+    const T &get_internal() const { return value; }
+
 private:
     cbindgen_private::PropertyHandleOpaque inner;
     mutable T value {};
diff --git a/internal/core/model.rs b/internal/core/model.rs
index cb4f33e8031..ab655791aee 100644
--- a/internal/core/model.rs
+++ b/internal/core/model.rs
@@ -895,15 +895,17 @@ impl<C: RepeatedItemTree + 'static> Repeater<C> {
     }
 
     fn model(self: Pin<&Self>) -> ModelRc<C::Data> {
-        // Safety: Repeater does not implement drop and never allows access to model as mutable
         let model = self.data().project_ref().model;
 
         if model.is_dirty() {
-            *self.data().inner.borrow_mut() = RepeaterInner::default();
-            self.data().is_dirty.set(true);
+            let old_model = model.get_internal();
             let m = model.get();
-            let peer = self.project_ref().0.model_peer();
-            m.model_tracker().attach_peer(peer);
+            if old_model != m {
+                *self.data().inner.borrow_mut() = RepeaterInner::default();
+                self.data().is_dirty.set(true);
+                let peer = self.project_ref().0.model_peer();
+                m.model_tracker().attach_peer(peer);
+            }
             m
         } else {
             model.get()
diff --git a/internal/core/properties.rs b/internal/core/properties.rs
index 5650f5a212f..f08f88e6b9a 100644
--- a/internal/core/properties.rs
+++ b/internal/core/properties.rs
@@ -861,8 +861,8 @@ impl<T: Clone> Property<T> {
         self.get_internal()
     }
 
-    /// Get the value without registering any dependencies or executing any binding
-    fn get_internal(&self) -> T {
+    /// Get the cached value without registering any dependencies or executing any binding
+    pub fn get_internal(&self) -> T {
         self.handle.access(|_| {
             // Safety: PropertyHandle::access ensure that the value is locked
             unsafe { (*self.value.get()).clone() }
diff --git a/internal/interpreter/dynamic_item_tree.rs b/internal/interpreter/dynamic_item_tree.rs
index 7e2324928e2..2f4618aad17 100644
--- a/internal/interpreter/dynamic_item_tree.rs
+++ b/internal/interpreter/dynamic_item_tree.rs
@@ -1726,7 +1726,11 @@ pub fn instantiate(
         let model_binding_closure = make_binding_eval_closure(expr, &self_weak);
         repeater.set_model_binding(move || {
             let m = model_binding_closure();
-            i_slint_core::model::ModelRc::new(crate::value_model::ValueModel::new(m))
+            if let Value::Model(m) = m {
+                m.clone()
+            } else {
+                i_slint_core::model::ModelRc::new(crate::value_model::ValueModel::new(m))
+            }
         });
     }
 


Test patch:
diff --git a/tests/cases/models/dirty_model.slint b/tests/cases/models/dirty_model.slint
new file mode 100644
index 00000000000..0fa13835532
--- /dev/null
+++ b/tests/cases/models/dirty_model.slint
@@ -0,0 +1,92 @@
+// Copyright © SixtyFPS GmbH <info@slint.dev>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+// Test that having the model being dirty doesn't re-create items
+// Only actually changing the model does.
+
+export component TestCase inherits Window {
+    property <{xx: int, model :[string]}> model: { model: ["AA", "BB"] };
+    in-out property <string> result ;
+    public function mark_dirty() {
+        model.xx += 1;
+    }
+    public function change() {
+        model.model = ["CC"];
+    }
+
+    HorizontalLayout {
+        Rectangle {}
+        for m in model.model : Rectangle {
+            init => {
+                result += "Init '" + m + "' (" + model.xx + ")\n";
+            }
+        }
+    }
+
+}
+
+/*
+```rust
+let instance = TestCase::new().unwrap();
+
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq!(instance.get_result(), "Init 'AA' (0)\nInit 'BB' (0)\n");
+instance.set_result("".into());
+slint_testing::send_mouse_click(&instance, 15., 5.);
+instance.invoke_mark_dirty();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq!(instance.get_result(), "");
+slint_testing::send_mouse_click(&instance, 15., 5.);
+instance.invoke_change();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq!(instance.get_result(), "Init 'CC' (1)\n");
+instance.set_result("".into());
+instance.invoke_mark_dirty();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq!(instance.get_result(), "");
+
+```
+
+```cpp
+auto handle = TestCase::create();
+const TestCase &instance = *handle;
+
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq(instance.get_result(), "Init 'AA' (0)\nInit 'BB' (0)\n");
+instance.set_result("");
+slint_testing::send_mouse_click(&instance, 15., 5.);
+instance.invoke_mark_dirty();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq(instance.get_result(), "");
+slint_testing::send_mouse_click(&instance, 15., 5.);
+instance.invoke_change();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq(instance.get_result(), "Init 'CC' (1)\n");
+instance.set_result("");
+instance.invoke_mark_dirty();
+slint_testing::send_mouse_click(&instance, 15., 5.);
+assert_eq(instance.get_result(), "");
+
+```
+
+
+```js
+var instance = new slint.TestCase({});
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+assert.equal(instance.result, "Init 'AA' (0)\nInit 'BB' (0)\n");
+instance.result = "";
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+instance.mark_dirty();
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+assert.equal(instance.result, "");
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+instance.change();
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+assert.equal(instance.result, "Init 'CC' (1)\n");
+instance.result = "";
+instance.mark_dirty();
+slintlib.private_api.send_mouse_click(instance, 15., 5.);
+assert.equal(instance.result, "");
+```
+*/
+