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

Dirty model property shouldn't cause re-creation of all elements in a for #7245

Open
shrydar opened this issue Jan 1, 2025 · 4 comments · May be fixed by #7280
Open

Dirty model property shouldn't cause re-creation of all elements in a for #7245

shrydar opened this issue Jan 1, 2025 · 4 comments · May be fixed by #7280
Labels
a:models&views The implementation of the `for` and ListView (mO,bF) bug Something isn't working

Comments

@shrydar
Copy link

shrydar commented Jan 1, 2025

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

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

@shrydar shrydar added bug Something isn't working need triaging Issue that the owner of the area still need to triage labels Jan 1, 2025
@tronical
Copy link
Member

tronical commented Jan 2, 2025

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.

@shrydar
Copy link
Author

shrydar commented Jan 2, 2025

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.

@ogoffart
Copy link
Member

ogoffart commented Jan 6, 2025

@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: #3953

This could be mitigated with something like this: #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

@tronical
Copy link
Member

tronical commented Jan 6, 2025

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

@ogoffart ogoffart changed the title State change animation fails to trigger within a component list from a sibling change. Dirty model property shouldn't cause re-creation of all elements in a for Jan 6, 2025
@ogoffart ogoffart added a:models&views The implementation of the `for` and ListView (mO,bF) and removed need triaging Issue that the owner of the area still need to triage labels Jan 6, 2025
ogoffart added a commit that referenced this issue Jan 6, 2025
Being dirty is not enough

Fixes #7245

ChangeLog: Elements of a `for` now only get re-created if the model is
changed, not if it is only dirty
ogoffart added a commit that referenced this issue Jan 6, 2025
Being dirty is not enough

Fixes #7245

ChangeLog: Elements of a `for` now only get re-created if the model is
changed, not if it is only dirty
ogoffart added a commit that referenced this issue Jan 6, 2025
Being dirty is not enough

Fixes #7245

ChangeLog: Elements of a `for` now only get re-created if the model is
changed, not if it is only dirty
ogoffart added a commit that referenced this issue Jan 6, 2025
Being dirty is not enough

Fixes #7245

ChangeLog: Elements of a `for` now only get re-created if the model is
changed, not if it is only dirty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:models&views The implementation of the `for` and ListView (mO,bF) bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants