Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

two way binding to struct fields #556

Closed
tronical opened this issue Oct 7, 2021 · 9 comments
Closed

two way binding to struct fields #556

tronical opened this issue Oct 7, 2021 · 9 comments
Labels
duplicate This issue or pull request already exists enhancement New feature or request

Comments

@tronical
Copy link
Member

tronical commented Oct 7, 2021

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 tronical added the bug Something isn't working label Oct 7, 2021
@tronical tronical changed the title UI isn't updated correctly when changing model from Rust/C++ TODO example UI isn't updated correctly when changing model from Rust/C++ Oct 8, 2021
@tronical
Copy link
Member Author

tronical commented Oct 8, 2021

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.

@ogoffart
Copy link
Member

ogoffart commented Oct 8, 2021

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.

@tronical
Copy link
Member Author

tronical commented Oct 8, 2021

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.

@ogoffart
Copy link
Member

ogoffart commented Oct 8, 2021

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 #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.

@tronical
Copy link
Member Author

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.

@levrik
Copy link
Contributor

levrik commented May 20, 2022

@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.

@ogoffart
Copy link
Member

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 #112 (comment)

      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).

@levrik
Copy link
Contributor

levrik commented May 20, 2022

@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.

@ogoffart ogoffart changed the title TODO example UI isn't updated correctly when changing model from Rust/C++ two way binding to struct fields Nov 29, 2022
@ogoffart ogoffart added enhancement New feature or request and removed bug Something isn't working labels Nov 29, 2022
@ogoffart
Copy link
Member

Closing this as a duplicate of #814

@ogoffart ogoffart closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2022
@ogoffart ogoffart added the duplicate This issue or pull request already exists label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants