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

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

Closed
kanashimia opened this issue May 16, 2024 · 2 comments · Fixed by #5347
Closed

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

kanashimia opened this issue May 16, 2024 · 2 comments · Fixed by #5347
Labels
a:compiler Slint compiler internal (not the codegen, not the parser) bug Something isn't working

Comments

@kanashimia
Copy link

kanashimia commented May 16, 2024

Background:

I've been implementing node graph, with code based on this: #3608 (comment)

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
Copy link
Member

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

@tronical tronical added bug Something isn't working a:compiler Slint compiler internal (not the codegen, not the parser) labels May 16, 2024
@ogoffart
Copy link
Member

ogoffart commented Jun 4, 2024

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.

@ogoffart ogoffart changed the title Different reactive behaviour on array models in slint-lsp or slint-viewer compared to slintpad or cargo build Const propagation is thinking model content is const when it shouldn't Jun 4, 2024
ogoffart added a commit that referenced this issue Jun 4, 2024
They are model and if the model is copied, the changes in model need to
apply to all copies.

Fixes #5249
ogoffart added a commit that referenced this issue Jun 4, 2024
They are model and if the model is copied, the changes in model need to
apply to all copies.

Fixes #5249
ogoffart added a commit that referenced this issue Jun 4, 2024
They are model and if the model is copied, the changes in model need to
apply to all copies.

Fixes #5249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:compiler Slint compiler internal (not the codegen, not the parser) bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants