slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
17.55k stars 601 forks source link

Const propagation is thinking model content is const when it shouldn't #5249

Closed kanashimia closed 5 months ago

kanashimia commented 5 months ago

Background:

I've been implementing node graph, with code based on this: https://github.com/slint-ui/slint/discussions/3608#discussioncomment-7207488

And there seems to be this one line: nodes[idx] = n;, the problem occurs when you remove it.

Problem:

Basically in slint-lsp or slint-viewer when you try to move Node then Link attached to it doesn't update, it looks like this:

image

But when you try to move it in the slintpad or in the compiled rust project with slint builder it looks like this:

image

Expected:

Both behave the same, I expected the second outcome.

Test:

[SlintPad preview]

To test you can just open SlintPad, and verify that it works, than save that file and open it on local machine with slint-viewer and verify that it doesn't work, uncomment the line, it will work.

Conclusion:

Is this a bug, or a skill issue? In any ways it is weird that the behaviour is different.

It looks like some update dependency isn't being set when it should, or in reverse.

APPENDIX 1: What I actually wanted is to move out Node from the loop into its own definition, but than there is a different problem with two-way bindings not supported, may be related here? That seems to only be implementable by passing whole array and index as properties, which is annoying. if you have idea on how to do it in a different way please say.

APPENDIX 2: Actually after looking into I think the issue is that one way binding acts like a two way binding on cargo run and slintpad? Here is the link: SlintPad weird example 2 You see, it has nodes: nodes; there right, so in the slintpad it propagates the modified state to the parent, but in the slint-lsp it doesn't, when you change it to nodes <=> nodes; then it works in both.

tronical commented 5 months ago

Wow, this is unexpected. Good that you found a workaround - this seems like a bug to me.

ogoffart commented 5 months ago

Thanks for filling a bug.

Here is a smaller testcase:

export component Btn {
  in-out property <[int]> model;
  width: 30px;
  height: 30px;
  Rectangle {
    background: red;
  }
  TouchArea {
    clicked => {
      model[0] += 1;
    }
  }
}

export component MainWindow inherits Window {
    preferred-height: 500px;
    preferred-width: 500px;
    //in-out     //changing the property to in-out fixes it in the viewer
    property <[int]> model: [42];

    // adding a change in the same component fixes the problem
    // TouchArea {  enabled: false; clicked => { model[0] += 10; }  }

    Btn{ model: model; }
    Text { text: model[0]; }
}

In that testcase, clicking on the red rectangle doesn't increment the text value. That's because the const propagation phase thinks this is a const model as it doesn't "see" the change. Changing the property to be in fixes it on the viewer but not on the LSP or slintpad because they add an extra layer so the property is still considered constant. Adding a setter somewhere does workaround the problem though, as then the compiler sees it changes.

The problem is that the const propagation think that the model is const because nothing gets assigned to it. But this analysis is incorrect for model that are assigned to other property and then get modified trough it.