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.

Sign up for GitHub

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

Jump to bottom

Fix panic when code in init callback of conditional/repeater re-triggers repeater traversal #3215

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix panic when code in init callback of conditional/repeater re-triggers repeater traversal #3215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions internal/core/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,27 +896,30 @@ impl<C: RepeatedItemTree + 'static> Repeater<C> {
model: &ModelRc<C::Data>,
count: usize,
) -> bool {
let mut indices_to_init = Vec::new();
let mut inner = self.0.inner.borrow_mut();
inner.instances.resize_with(count, || (RepeatedInstanceState::Dirty, None));
let offset = inner.offset;
let mut any_items_created = false;
for (i, c) in inner.instances.iter_mut().enumerate() {
if c.0 == RepeatedInstanceState::Dirty {
let created = if c.1.is_none() {
if c.1.is_none() {
any_items_created = true;
c.1 = Some(init());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this call to init() also recurse?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, true, there's probably a way of doing that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think just by adding another layer:

// added if true there:
if true: layout := VerticalLayout {
        if true: TextInput {
            // Trigger a call into the layout
            init => { debug(self.y); }
        }
    }

But maybe self.y don't cross layers, but some other properties certainly will

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how to fix this. I mean, I made a local change to separate out the init() creation without borrowing the inner, but conceptually this would still recurse because the dirty() flag is true.

Perhaps like this?

  1. Replace inner.components with an empty vec; set dirty to false.
  2. Update any dirty components, call init, etc() - during that time any recursion will find an empty repeater.
  3. Replace inner.components with the updated vec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep a Vec of ItemTreeRc and init them when the RefCell is not borrowed?

true
} else {
false
indices_to_init.push(i);
};
c.1.as_ref().unwrap().update(i + offset, model.row_data(i + offset).unwrap());
if created {
c.1.as_ref().unwrap().init();
}
c.0 = RepeatedInstanceState::Clean;
}
}
self.data().is_dirty.set(false);

drop(inner);
let inner = self.0.inner.borrow();
for item in indices_to_init.into_iter().filter_map(|index| inner.instances.get(index)) {
item.1.as_ref().unwrap().init();
}

any_items_created
}

Expand Down
36 changes: 36 additions & 0 deletions tests/cases/models/init_recursion.slint
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright © SixtyFPS GmbH <[email protected]>
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-1.1 OR LicenseRef-Slint-commercial

TestCase := Rectangle {
property <length> test-height: layout.preferred-height;
property <bool> test: self.test-height != 0;

layout := VerticalLayout {
if true: TextInput {
// Trigger a call into the layout
init => { debug(self.y); }
}
}
}

/*
```cpp
auto handle = TestCase::create();
const TestCase &instance = *handle;

assert(instance.get_test_height() != 0.);
```


```rust
let instance = TestCase::new().unwrap();

assert!(instance.get_test_height() != 0.);
```

```js
var instance = new slint.TestCase();

assert(instance.test_height != 0.);
```
*/