-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Context menu #543
Context menu #543
Conversation
Interesting! Have you considered how this relates to |
Yes, I drew inspiration from it, as this was pretty much my first experience with egui, and now they both render the contents inside a I am now trying to make |
I am pretty happy so far, but it doesn't work perfectly yet. It is now possible to define a context menu for an individual ui.context_menu(|ui, menu_state| {
if ui.button("Add...").clicked() {
self.add();
menu_state.close();
}
}); for example. The
Unfortunately I was not able to attach context menus to the items in the drag and drop example properly. When right clicking them I only ever get the context menu for the top left item of all columns, and it mostly closes immediately and only stays open a few of the times I click. I suspect it is something with the bounding boxes of the items, where I am probably not using an accurate one to detect clicks on the respective @emilk do you maybe have a suggestion which Rect I should use to detect clicks on the content of a |
e5e6e3c
to
701baf9
Compare
Problem solved, the context menu is now applied to Responses, which hold the accurate rect of the relevant Ui and their Id. Next step is to improve submenu navigation. |
Some nitpicks from a UX point of view: The ui.add(example_plot(plot))
.context_menu(|ui| {
if ui.button("Sin").clicked() {
*plot = Plot::Sin;
MenuState::Close
} else if ui.button("Bell").clicked() {
*plot = Plot::Bell;
MenuState::Close
} else if ui.button("Sigmoid").clicked() {
*plot = Plot::Sigmoid;
MenuState::Close
} else {
MenuState::Continue
}
}); It could also be an Going over the code, there is also a public callback named |
Ah,
I thought about this too but I kind of liked the state argument better because it felt more flexible. I am not sure how this feature is going to evolve and if there will be other functions you might want to call from the context menu ui. If you wanted to make multiple calls this would probably be more ergonomic. But I don't see any reason not to return enums by default and have something like |
The submenu navigation works a lot better now, the code checks if the pointer is travelling towards an open submenu and allows it to cross over other items if it is. It also requests a repaint if it is travelling towards a submenu, so that when the pointer stops moving, this can be detected even in reactive mode. There is one problem with draggable elements though, like in the drag-and-drop example. Because drag is also enabled by right clicking, the draggable elements sense a drag instead of a click when right clicking them. I think it is rather untypical to drag using the right mouse button, and I think it would be reasonable to disable dragging with secondary pointer button by default, in favor of detecting secondary button clicks. #547 would also solve this |
@parasyte I also just noticed that the menu_state is needed to create SubMenus, as they use the parent state to know if they are open or not. |
Isn't that a concern that is internal to the implementation? I'm not sure the caller should be aware of or have access to that information. |
The |
Making some progress on the styling, but there seems to be an issue with overlap and hovering of the submenu button. My guess is that the submenu's frame margin or shadow covers a part of the button in the parent menu, and that causes the submenu to close and reopen repeatedly. I would be very glad if someone could take a look at the styling code and share some thoughts about it. I feel like I am making this harder than it should be. |
d75870a
to
18e3dc7
Compare
82df857
to
cfb3ba3
Compare
ea3e39a
to
23e1886
Compare
I think I will also look into implementing submenus in the existing menus this way. It will pretty much work the same way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very impressive work, but I have a concern about having to write menu_state.item("Click me").show(ui).clicked()
. I would prefer if one could simply write ui.button("Click me").clicked()
, and add any widget at all with just ui.add(…)
as anywhere else in egui. I understand we need something special for submenus, but I think we should be able to get other widgets to work.
The context menus should support all the widgets egui does, and should do so using the same interface.
@emilk Thanks for reviewing this! I agree about the API, that should all be hidden in .response
.context_menu(ui, |ui, menu_state| {
if ui.menu_item("Text", &menu_state).clicked() {
menu_state.close()
}
ui.sub_menu("Text", &mut menu_state, |ui, menu_state| {
if ui.menu_item("Nested item", &menu_state).clicked() {
menu_state.close()
}
})
}) The benefit here is that there is always a menu_state when there should be and no I will try to integrate your suggestions soon, so we can move on with this :) Edit: Another option would be to have separate type for a |
@coderedart Thanks for your input. There is no special handling for large menus yet, so the items would just go off-screen. I will try to get scrollable and multi-column menus working. About your second question: checkboxes will be easily possible, just like all other kinds of widgets. The menu will just be another Ui. Also the menu will be closed explicitly, so you can interact with widgets without closing the menu. |
wasn't really needed so far and is probably better for encapsulation
an InnerResponse for the SubMenu button Response with an optional inner Response for the extended SubMenu
clicks only occur on button release, button press feels more responsive
- refactoring - removed some demos - add doctest for ui.menu_button() - fixed menu_state not being cloned to child ui
36082c2
to
b54233c
Compare
|
I will add the changelog note tomorrow 👍 |
So happy to contribute to this awesome project! Thanks for all the support :) was great fun |
but what about the changelog now @emilk ? I didn't add that yet. But it is probably just one line anyways.. |
Follow-up to #543 * Add entry to CHANGELOG.md * Add entry to contributors in README.md * Improve documentation * Simplify demo
I just pushed a line to the changelog in 41f77ba! |
I have just created an egui discord server: https://discord.gg/qzvmcrUe - come join if you like! |
(updated 8.9.21)
ContextMenuSystem
: Stored by the Context, stores menu state across framescontext_menu
can be called onResponse
. It checks if a context menu should be shown and takes a builder closure for the context menu's contents:Ui
is extended to support creating and closing of menus via themenu
andclose
methods. Whenmenu
is called from within aUi
for a menu, it creates a button with an expandable sub-menu, otherwise it creates an inline drop-down menu. Callingclose
clears the internal menu state.To-Do:
SubMenu::show
:InnerResponse<Option<Response>>
)MenuUi
Needs new PR:
Let me know if you have any suggestions or find any problems :)