Skip to content

Conversation

@zanmato
Copy link
Contributor

@zanmato zanmato commented Nov 19, 2025

Fixes #1643 and partially #1489

Adds .context_menu for ListItem

@zanmato
Copy link
Contributor Author

zanmato commented Nov 19, 2025

After making this I'm not sure if it's needed, or the docs could be updated to add context_menu on the list item child instead?

tree(&tree_state, |ix, entry, selected, window, cx| {
    ListItem::new(ix)
        .selected(selected)
        .child(
            div()
                .child(entry.item().label.clone())
                .context_menu(|menu, _, _| {
                    menu.menu("Rename", Box::new(Rename))
                        .menu("Delete", Box::new(Delete))
                        .separator()
                        .menu("Copy Path", Box::new(CopyPath))
                })
        )
})

Copy link
Member

@huacnlee huacnlee left a comment

Choose a reason for hiding this comment

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

I think, we should not add a context_menu to a ListItem, it should add to it parent container.

You can see the example in Table:

.context_menu({
let view = cx.entity().clone();
move |this, window: &mut Window, cx: &mut Context<PopupMenu>| {
if let Some(row_ix) = view.read(cx).right_clicked_row {
view.update(cx, |menu, cx| {
menu.delegate().context_menu(row_ix, this, window, cx)
})
} else {
this
}
}
})

@zanmato
Copy link
Contributor Author

zanmato commented Nov 19, 2025

There is a bug with this approach, you have to right click twice, because it seems like the window.on_mouse_event in ContextMenu fires before the on_mouse_down event of the table (or tree in this case). You can see it in the table_story. Probably because the uniform list is further down in the element tree than the context menu on the parent/root element?

@huacnlee huacnlee changed the title list: Add context menu support tree: Add context menu support Nov 20, 2025
@zanmato
Copy link
Contributor Author

zanmato commented Nov 20, 2025

It also means that it is showing the wrong context menu, since the right clicked index is the previous one 😞 Perhaps context menu needs to be something "global" like dialog/sheet? So we can force it open in the correct event listener?

@zanmato
Copy link
Contributor Author

zanmato commented Nov 20, 2025

@huacnlee Updated it to use the same pattern as table with a delegate. It suffers from the same issue as table though, see #1651

Should we keep tree()?

@huacnlee
Copy link
Member

huacnlee commented Nov 21, 2025

No, we can not change the tree API.

You're doing this effected this PR more complex now.

Small improve must be a small changes.


I don't know how to do this better right now, but I am sure we can't to change the Tree API. I will consider this when have a rest time.

[SettingField]: https://docs.rs/gpui-component/latest/gpui_component/setting/enum.SettingField.html
[NumberFieldOptions]: https://docs.rs/gpui-component/latest/gpui_component/setting/struct.NumberFieldOptions.html
[GroupBox]: ./group_box.md
[GroupBox]: ./group-box.md
Copy link
Member

Choose a reason for hiding this comment

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

Split this into a single PR, so we can merge first.

@zanmato
Copy link
Contributor Author

zanmato commented Nov 21, 2025

No, we can not change the tree API.

You're doing this effected this PR more complex now.

Small improve must be a small changes.

I don't know how to do this better right now, but I am sure we can't to change the Tree API. I will consider this when have a rest time.

OK! I will close this and open a PR with the docs fix. We could do tree(...).context_menu({}) without breaking the API, but that would require storing the context menu handler in the TreeState.

@zanmato zanmato closed this Nov 21, 2025
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.

docs(tree): ListItem doesn't have on_secondary_mouse_down

2 participants