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

Reset Memory::popup to None if a popup was abandoned #4697

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

juancampa
Copy link
Contributor

@juancampa juancampa commented Jun 23, 2024

BEFORE

Screenshot.2024-06-23.at.18.41.41.mp4

AFTER

Screenshot.2024-06-23.at.18.56.01.mp4

Notice that WRONG flashes for one frame. That's expected because on the frame the combo box disappears, any_popup_open still returns true, since we don't know it has disappeared until the end of the frame.

#[cfg_attr(feature = "persistence", serde(skip))]
popup: Option<Id>,
popup: Option<(Id, std::sync::Arc<AtomicBool>)>,
Copy link
Contributor Author

@juancampa juancampa Jun 23, 2024

Choose a reason for hiding this comment

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

I'm using an AtomicBool here because I didn't want to change is_popup_open to take &mut self.

An alternative is to add another function, something like retain_popup(&mut self) that explicitly sets a retained bool to true. But would will be a breaking change and existing apps will need to call it or popups will only last for one frame.

I'm open to suggestions.

Copy link
Owner

@emilk emilk Jul 15, 2024

Choose a reason for hiding this comment

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

it's not at all obvious from reading this what the bool is for though., or why it is behind an Arc.

It seems to answer the question "Was is_popup_open called for this this frame?", i.e. "Does anyone care that this is open?".

I suggest making a struct with a name field so the motivation behind this can be documented.

We should also document that if is_popup_open is not called during a frame, the popup auto-closes.
Since this in fact makes it change state, I think making it take a &mut makes sense.

So I suggest

struct OpenPopup {
    id: Id,
    shown_this_frame: bool,
}

…

popup: Option<OpenPopup>,

@juancampa juancampa marked this pull request as ready for review June 23, 2024 23:11
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.

any_popup_open() remains true even when there are no popups
2 participants