Make Memory::keep_popup_open less of a footgun
#7297
Conversation
|
Preview available at https://egui-pr-preview.github.io/pr/7297-lucaspopup-footgun |
| self.popups | ||
| .get(&self.viewport_id) | ||
| .is_some_and(|state| state.id == popup_id) | ||
| || self.everything_is_visible() | ||
| } | ||
|
|
||
| /// Check if the popup is open and keep it open. 8000 | ||
| pub fn show_popup(&mut self, popup_id: Id) -> bool { |
There was a problem hiding this comment.
The docstring for open_popup should link to this function, and vice-versa, to explain their difference..
Also: wouldn't it make sense if this called self.open_popup?
There was a problem hiding this comment.
No, if this called open_popup, you couldn't use it to check if we should show the popup.
The docs should also mention it needs to be called every frame.
There was a problem hiding this comment.
I think this is just another shape of footgun.
Based only on the names of the functions, I would expect this to always hold:
mem.show_popup(popup_id);
assert!(mem.is_showing_popup(popup_id));Which would then lead to the question "what is open_popup for?"
| @@ -531,7 +531,7 @@ impl<'a> Popup<'a> { | |||
| mem.toggle_popup(id); | |||
| } | |||
| None => { | |||
| mem.keep_popup_open(id); | |||
| mem.show_popup(id); | |||
There was a problem hiding this comment.
Shouldn't we always call this unless we return None below? 🤔
Memory::keep_popup_openless of a footgun #7037The breaking change is intentional, to force people to think about this, since otherwise the keep_popup_open thing would silently break popups.