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

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

Conversation

tronical
Copy link
Member

@tronical tronical commented Aug 3, 2023

Don't call init() while mutably borrowing the repeater's inner.

Fixes #3214

@tronical tronical requested a review from ogoffart August 3, 2023 12:24
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I wonder if there is a way to un-borrow the inner when calling the init methods.

Alternative could be to try to detect recursion and bail out?

let mut inner = self.0.inner.borrow_mut();
inner.components.resize_with(count, || (RepeatedComponentState::Dirty, None));
let offset = inner.offset;
let mut any_items_created = false;
for (i, c) in inner.components.iter_mut().enumerate() {
if c.0 == RepeatedComponentState::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?

@tronical
Copy link
Member Author

tronical commented Mar 8, 2024

(just a rebase for now)

@tronical tronical marked this pull request as draft March 8, 2024 07:57
…ers repeater traversal

Don't call init() while mutably borrowing the repeater's inner.

Fixes #3214
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when conditional/repeated element ends up traversing the item tree during init()
2 participants