Rework context menu's custom element API to handle clicks properly

Kirill Bulatov created

Change summary

crates/language_tools2/src/lsp_log.rs     | 86 ++++++++++----------
crates/ui2/src/components/context_menu.rs | 98 +++++++++++++-----------
2 files changed, 97 insertions(+), 87 deletions(-)

Detailed changes

crates/language_tools2/src/lsp_log.rs 🔗

@@ -3,9 +3,8 @@ use editor::{Editor, EditorElement, EditorEvent, MoveToEnd};
 use futures::{channel::mpsc, StreamExt};
 use gpui::{
     actions, div, AnchorCorner, AnyElement, AppContext, Context, Div, EventEmitter, FocusHandle,
-    FocusableView, InteractiveElement, IntoElement, Model, ModelContext, MouseButton,
-    ParentElement, Render, Styled, Subscription, View, ViewContext, VisualContext, WeakModel,
-    WindowContext,
+    FocusableView, IntoElement, Model, ModelContext, ParentElement, Render, Styled, Subscription,
+    View, ViewContext, VisualContext, WeakModel, WindowContext,
 };
 use language::{LanguageServerId, LanguageServerName};
 use lsp::IoKind;
@@ -776,47 +775,48 @@ impl Render for LspLogToolbarItemView {
                             );
                         }
 
-                        menu = menu.custom_entry({
-                            let log_view = log_view.clone();
-                            let log_toolbar_view = log_toolbar_view.clone();
-                            move |cx| {
-                                h_stack()
-                                    .w_full()
-                                    .justify_between()
-                                    .child(Label::new(RPC_MESSAGES))
-                                    .child(
-                                        Checkbox::new(
-                                            ix,
-                                            if row.rpc_trace_enabled {
-                                                Selection::Selected
-                                            } else {
-                                                Selection::Unselected
-                                            },
-                                        )
-                                        .on_click(
-                                            cx.listener_for(
-                                                &log_toolbar_view,
-                                                move |view, selection, cx| {
-                                                    let enabled =
-                                                        matches!(selection, Selection::Selected);
-                                                    view.toggle_logging_for_server(
-                                                        row.server_id,
-                                                        enabled,
-                                                        cx,
-                                                    );
-                                                },
+                        menu = menu.custom_entry(
+                            {
+                                let log_toolbar_view = log_toolbar_view.clone();
+                                move |cx| {
+                                    h_stack()
+                                        .w_full()
+                                        .justify_between()
+                                        .child(Label::new(RPC_MESSAGES))
+                                        .child(
+                                            div().z_index(120).child(
+                                                Checkbox::new(
+                                                    ix,
+                                                    if row.rpc_trace_enabled {
+                                                        Selection::Selected
+                                                    } else {
+                                                        Selection::Unselected
+                                                    },
+                                                )
+                                                .on_click(cx.listener_for(
+                                                    &log_toolbar_view,
+                                                    move |view, selection, cx| {
+                                                        let enabled = matches!(
+                                                            selection,
+                                                            Selection::Selected
+                                                        );
+                                                        view.toggle_logging_for_server(
+                                                            row.server_id,
+                                                            enabled,
+                                                            cx,
+                                                        );
+                                                        cx.stop_propagation();
+                                                    },
+                                                )),
                                             ),
-                                        ),
-                                    )
-                                    .on_mouse_down(
-                                        MouseButton::Left,
-                                        cx.listener_for(&log_view, move |view, _, cx| {
-                                            view.show_rpc_trace_for_server(row.server_id, cx);
-                                        }),
-                                    )
-                                    .into_any_element()
-                            }
-                        });
+                                        )
+                                        .into_any_element()
+                                }
+                            },
+                            cx.handler_for(&log_view, move |view, cx| {
+                                view.show_rpc_trace_for_server(row.server_id, cx);
+                            }),
+                        );
                         if server_selected && row.rpc_trace_selected {
                             debug_assert_eq!(
                                 Some(ix * 3 + 2),

crates/ui2/src/components/context_menu.rs 🔗

@@ -20,6 +20,7 @@ enum ContextMenuItem {
     },
     CustomEntry {
         entry_render: Box<dyn Fn(&mut WindowContext) -> AnyElement>,
+        handler: Rc<dyn Fn(&mut WindowContext)>,
     },
 }
 
@@ -89,9 +90,11 @@ impl ContextMenu {
     pub fn custom_entry(
         mut self,
         entry_render: impl Fn(&mut WindowContext) -> AnyElement + 'static,
+        handler: impl Fn(&mut WindowContext) + 'static,
     ) -> Self {
         self.items.push(ContextMenuItem::CustomEntry {
             entry_render: Box::new(entry_render),
+            handler: Rc::new(handler),
         });
         self
     }
@@ -117,10 +120,12 @@ impl ContextMenu {
     }
 
     pub fn confirm(&mut self, _: &menu::Confirm, cx: &mut ViewContext<Self>) {
-        if let Some(ContextMenuItem::Entry { handler, .. }) =
-            self.selected_index.and_then(|ix| self.items.get(ix))
-        {
-            (handler)(cx)
+        match self.selected_index.and_then(|ix| self.items.get(ix)) {
+            Some(
+                ContextMenuItem::Entry { handler, .. }
+                | ContextMenuItem::CustomEntry { handler, .. },
+            ) => (handler)(cx),
+            _ => {}
         }
 
         cx.emit(DismissEvent);
@@ -251,51 +256,56 @@ impl Render for ContextMenu {
                 })
                 .flex_none()
                 .child(List::new().children(self.items.iter_mut().enumerate().map(
-                    |(ix, item)| {
-                        match item {
-                            ContextMenuItem::Separator => ListSeparator.into_any_element(),
-                            ContextMenuItem::Header(header) => {
-                                ListSubHeader::new(header.clone()).into_any_element()
-                            }
-                            ContextMenuItem::Entry {
-                                label,
-                                handler,
-                                icon,
-                                action,
-                            } => {
-                                let handler = handler.clone();
-
-                                let label_element = if let Some(icon) = icon {
-                                    h_stack()
-                                        .gap_1()
-                                        .child(Label::new(label.clone()))
-                                        .child(IconElement::new(*icon))
-                                        .into_any_element()
-                                } else {
-                                    Label::new(label.clone()).into_any_element()
-                                };
+                    |(ix, item)| match item {
+                        ContextMenuItem::Separator => ListSeparator.into_any_element(),
+                        ContextMenuItem::Header(header) => {
+                            ListSubHeader::new(header.clone()).into_any_element()
+                        }
+                        ContextMenuItem::Entry {
+                            label,
+                            handler,
+                            icon,
+                            action,
+                        } => {
+                            let handler = handler.clone();
 
-                                ListItem::new(ix)
-                                    .inset(true)
-                                    .selected(Some(ix) == self.selected_index)
-                                    .on_click(move |_, cx| handler(cx))
-                                    .child(
-                                        h_stack()
-                                            .w_full()
-                                            .justify_between()
-                                            .child(label_element)
-                                            .children(action.as_ref().and_then(|action| {
-                                                KeyBinding::for_action(&**action, cx)
-                                                    .map(|binding| div().ml_1().child(binding))
-                                            })),
-                                    )
+                            let label_element = if let Some(icon) = icon {
+                                h_stack()
+                                    .gap_1()
+                                    .child(Label::new(label.clone()))
+                                    .child(IconElement::new(*icon))
                                     .into_any_element()
-                            }
-                            ContextMenuItem::CustomEntry { entry_render } => ListItem::new(ix)
+                            } else {
+                                Label::new(label.clone()).into_any_element()
+                            };
+
+                            ListItem::new(ix)
+                                .inset(true)
+                                .selected(Some(ix) == self.selected_index)
+                                .on_click(move |_, cx| handler(cx))
+                                .child(
+                                    h_stack()
+                                        .w_full()
+                                        .justify_between()
+                                        .child(label_element)
+                                        .children(action.as_ref().and_then(|action| {
+                                            KeyBinding::for_action(&**action, cx)
+                                                .map(|binding| div().ml_1().child(binding))
+                                        })),
+                                )
+                                .into_any_element()
+                        }
+                        ContextMenuItem::CustomEntry {
+                            entry_render,
+                            handler,
+                        } => {
+                            let handler = handler.clone();
+                            ListItem::new(ix)
                                 .inset(true)
                                 .selected(Some(ix) == self.selected_index)
+                                .on_click(move |_, cx| handler(cx))
                                 .child(entry_render(cx))
-                                .into_any_element(),
+                                .into_any_element()
                         }
                     },
                 ))),