slint icon indicating copy to clipboard operation
slint copied to clipboard

TODO example UI isn't updated correctly when changing model from Rust/C++

Open tronical opened this issue 2 years ago • 8 comments

Modify the Todo example like this:

diff --git a/examples/todo/cpp/main.cpp b/examples/todo/cpp/main.cpp
index bc614ca8..d203f353 100644
--- a/examples/todo/cpp/main.cpp
+++ b/examples/todo/cpp/main.cpp
@@ -30,13 +26,10 @@ int main()
     });

     demo->on_remove_done([todo_model] {
-        int offset = 0;
-        int count = todo_model->row_count();
-        for (int i = 0; i < count; ++i) {
-            if (todo_model->row_data(i - offset).checked) {
-                todo_model->erase(i - offset);
-                offset += 1;
-            }
+        for (auto i = 0; i < todo_model->row_count(); ++i) {
+            auto todo = todo_model->row_data(i);
+            todo.checked = true;
+            todo_model->set_row_data(i, todo);
         }
     });

diff --git a/examples/todo/rust/main.rs b/examples/todo/rust/main.rs
index b29ef759..9738caf6 100644
--- a/examples/todo/rust/main.rs
+++ b/examples/todo/rust/main.rs
@@ -42,12 +42,10 @@ pub fn main() {
     main_window.on_remove_done({
         let todo_model = todo_model.clone();
         move || {
-            let mut offset = 0;
             for i in 0..todo_model.row_count() {
-                if todo_model.row_data(i - offset).checked {
-                    todo_model.remove(i - offset);
-                    offset += 1;
-                }
+                let mut bulb = todo_model.row_data(i);
+                bulb.checked = true;
+                todo_model.set_row_data(i, bulb);
             }
         }
     });

Then when running, pressing the "remove all done" button will toggle all todos. That works the first time. Then turn off a few toggles in the UI and press the "remove all done" button again.

Expected Result: All todos are checked again. Actual Result: Nothing changes in the UI.

tronical avatar Oct 07 '21 15:10 tronical

At the heart of the issue lies that the binding for the checked property is destroyed:

CheckBox {
    text: todo.title;
    checked: todo.checked;
    toggled => {
        todo.checked = checked;
    }
}

Initially the binding checked: todo.checked; correctly fetches the data out of the model (model_data property). But when the user clicks on the CheckBox, the style runs this code in the TouchArea:

touch := TouchArea {
        clicked => {
            if (root.enabled) {
                root.checked = !root.checked;
                root.toggled();
            }
        }
    }

and the assignment to root.checked destroys the data model binding.

tronical avatar Oct 08 '21 10:10 tronical

We would somehow need to support two ways binding with model properties this is not easy because the runtime really expect properties so they can be merged into one property.

ogoffart avatar Oct 08 '21 10:10 ogoffart

I've begun prototyping an approach that tries to detect two-way bindings in model data structures, synthesise a property for it and update it in the model update function.

Diagnostics wise it might be interesting to also have a pass that tries to detect cases where we know that a property has a binding and is written to somewhere else - in order to issue a warning that perhaps a two-way binding is desired.

tronical avatar Oct 08 '21 14:10 tronical

Diagnostics wise it might be interesting to also have a pass that tries to detect cases where we know that a property has a binding and is written to somewhere else

I think we really need to be able to declare property are meant to be "readonly" or something (like the pressed property of a TouchArea) or read-write (like the checked property of a checkbox) or meant to be set like the background property for example. Issue https://github.com/sixtyfpsui/sixtyfps/issues/191

For example the compiler can't know if the NativeCheckBox assignthe checked property. However we already annotate some property as being set. But it'd be better to be exlicit about it i think.

ogoffart avatar Oct 08 '21 14:10 ogoffart

We discussed the two-way bindings separately a bit further and with regards to the model we could implement tow-way bindings for model data by using the existing callback functionality of struct PropertyTracker<ChangeHandler = ()> in generated code to write changes back into the model.

tronical avatar Oct 11 '21 08:10 tronical

@ogoffart I was just hit by this as well. Is there some suggested workaround for now? I can't control the checked property of a CheckBox from Rust anymore as soon as it was toggled once which basically makes it impossible to implement my desired UX.

levrik avatar May 20 '22 13:05 levrik

Is there some suggested workaround for now?

It depends a bit of what you're doing. But we can use a workaround similar to the missing changed event https://github.com/slint-ui/slint/issues/112#issuecomment-1065866865

      CheckBox {
            text: {
                checked = the_property;    // <- Ugly workaround right there        
                " Check me ";
            }
            checked: the_property;
            toggled => {
                the_property = checked;
            }
        }

code editor link of example

This works because the text property is evaluated (indirectly) by the rendering engine. And it depends on the the_property and do side effect in a binding (which is supposed to be pure, but nobody is checking).

ogoffart avatar May 20 '22 14:05 ogoffart

@ogoffart I see. I worked around by using my local fork and slightly modifying CheckBoxs implementation. I could propose this change in a PR when I have a free moment but it might be a breaking change so unsure about it.

levrik avatar May 20 '22 14:05 levrik

Closing this as a duplicate of https://github.com/slint-ui/slint/issues/814

ogoffart avatar Nov 29 '22 11:11 ogoffart