dan/gpui-component
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

menu: Use defer to avoid race conditions with click listeners (#1651)

Browse source 
When using `.context_menu` on a parent element, the
`window.on_mouse_event` would fire before, for example, the tables
`on_mouse_down` so it'd be like this:
1. First right click = no context menu, right_clicked_row is set
2. Second right click = context menu, but with the row from the first
click

Before: (Look at `Selected row`)  


https://github.com/user-attachments/assets/5eb89a00-f1ce-4423-b8d7-3ec61b848b83

After:


https://github.com/user-attachments/assets/10033ca4-4c2c-47b5-ace5-6ba745fcfee5

---------

Co-authored-by: Jason Lee <huacnlee@gmail.com>
This commit is contained in:
Andreas Johansson 2025-11-21 03:27:21 +01:00 committed by GitHub
parent c8652f9010
commit b6bf2d8c5e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 35 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
crates/ui/src/menu/context_menu.rs
View file

@ -29,7 +29,7 @@ impl<E: ParentElement + Styled> ContextMenuExt for E {}
pub struct ContextMenu<E: ParentElement + Styled + Sized> { pub struct ContextMenu<E: ParentElement + Styled + Sized> {
id: ElementId, id: ElementId,
element: Option<E>, element: Option<E>,
menu: Option<Box<dyn Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu>>, menu: Option<Rc<dyn Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu>>,
// This is not in use, just for style refinement forwarding. // This is not in use, just for style refinement forwarding.
_ignore_style: StyleRefinement, _ignore_style: StyleRefinement,
anchor: Corner, anchor: Corner,
@ -53,7 +53,7 @@ impl<E: ParentElement + Styled> ContextMenu<E> {
where where
F: Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu + 'static, F: Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu + 'static,
{ {
self.menu = Some(Box::new(builder)); self.menu = Some(Rc::new(builder));
self self
} }
@ -235,9 +235,8 @@ impl<E: ParentElement + Styled + IntoElement + 'static> Element for ContextMenu<
element.paint(window, cx); element.paint(window, cx);
} }
let Some(builder) = self.menu.take() else { // Take the builder before setting up element state to avoid borrow issues
return; let builder = self.menu.clone();
};
self.with_element_state( self.with_element_state(
id.unwrap(), id.unwrap(),
@ -254,26 +253,44 @@ impl<E: ParentElement + Styled + IntoElement + 'static> Element for ContextMenu<
{ {
{ {
let mut shared_state = shared_state.borrow_mut(); let mut shared_state = shared_state.borrow_mut();
// Clear any existing menu view to allow immediate replacement
// Set the new position and open the menu
shared_state.menu_view = None;
shared_state._subscription = None;
shared_state.position = event.position; shared_state.position = event.position;
shared_state.open = true; shared_state.open = true;
} }
let menu = PopupMenu::build(window, cx, |menu, window, cx| { // Use defer to build the menu in the next frame, avoiding race conditions
(builder)(menu, window, cx) window.defer(cx, {
})
.into_element();
let _subscription = window.subscribe(&menu, cx, {
let shared_state = shared_state.clone(); let shared_state = shared_state.clone();
move |_, _: &DismissEvent, window, _| { let builder = builder.clone();
shared_state.borrow_mut().open = false; move |window, cx| {
window.refresh(); let menu = PopupMenu::build(window, cx, move |menu, window, cx| {
let Some(build) = &builder else {
return menu;
};
build(menu, window, cx)
});
// Set up the subscription for dismiss handling
let _subscription = window.subscribe(&menu, cx, {
let shared_state = shared_state.clone();
move |_, _: &DismissEvent, window, _cx| {
shared_state.borrow_mut().open = false;
window.refresh();
}
});
// Update the shared state with the built menu and subscription
{
let mut state = shared_state.borrow_mut();
state.menu_view = Some(menu.clone());
state._subscription = Some(_subscription);
window.refresh();
}
} }
}); });
shared_state.borrow_mut().menu_view = Some(menu.clone());
shared_state.borrow_mut()._subscription = Some(_subscription);
window.refresh();
} }
}); });
}, },