Skip to content
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

Merged
merged 46 commits into from
Oct 26, 2021
Merged

Context menu #543

merged 46 commits into from
Oct 26, 2021

Conversation

mankinskin
Copy link
Contributor

@mankinskin mankinskin commented Jul 3, 2021

(updated 8.9.21)

  • ContextMenuSystem: Stored by the Context, stores menu state across frames

  • context_menu can be called on Response. It checks if a context menu should be shown and takes a builder closure for the context menu's contents:

ui.horizontal(|ui| {
    ui.label("title:");
    ui.text_edit_singleline(title);
})
.response
.context_menu(|ui| {
    if ui.button("Clear..").clicked() {
        *title = String::new();
        ui.close();
    }
});
  • the Ui is extended to support creating and closing of menus via the menu and close methods. When menu is called from within a Ui for a menu, it creates a button with an expandable sub-menu, otherwise it creates an inline drop-down menu. Calling close clears the internal menu state.
.response
.context_menu(|ui| {
    if ui.button("Open...").clicked() {
        ui.close();
    }
    ui.menu("SubMenu", |ui| {
        ui.menu("SubMenu", |ui| {
            if ui.button("Open...").clicked() {
                ui.close();
            }
            let _ = ui.button("Item");
        });
        if ui.button("Item").clicked() {
            ui.close();
        }
    });
});

Peek 2021-08-28 11-45

To-Do:

Needs new PR:

Let me know if you have any suggestions or find any problems :)

@emilk
Copy link
Owner

emilk commented Jul 6, 2021

Interesting! Have you considered how this relates to egui::menu? It would be great if all menus could be unified, as there are a lot of similar things to solve (submenus, when to close the main menu, etc).

@mankinskin
Copy link
Contributor Author

Have you considered how this relates to egui::menu?

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 Frame inside an Area, so this part could probably be unified. I would like to keep things separate for now though, until the functionality is complete and we can work on the styling.

I am now trying to make ctx.context_menu(add_contents) and ui.context_menu(add_contents) work and respect the clicked area to show context related context menus.

@mankinskin
Copy link
Contributor Author

mankinskin commented Jul 6, 2021

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 using

ui.context_menu(|ui, menu_state| {
    if ui.button("Add...").clicked() {
        self.add();
        menu_state.close();
    }
});

for example. The ContextMenuSystem is now stored by the egui::Context and uses Ui Id's to manage which context menu to show. Basically a context menu will only be shown when a proper click happens in its max_rect_finite(). I am not sure if this is the best Rect to use, but it was the one that worked best so far. I added some examples to the demo:

  • top bar
  • backend panel
  • widget gallery plot
  • drag and drop left column

Peek 2021-07-07 00-38

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 Ui.

@emilk do you maybe have a suggestion which Rect I should use to detect clicks on the content of a Ui? I am currently using ui.rect_contains_pointer(ui.max_rect_finite()).

@mankinskin mankinskin force-pushed the context-menu branch 2 times, most recently from e5e6e3c to 701baf9 Compare July 7, 2021 13:59
@mankinskin
Copy link
Contributor Author

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.

@parasyte
Copy link
Contributor

parasyte commented Jul 8, 2021

Some nitpicks from a UX point of view:

The menu_state callbacks look unusual and creates flow control that is difficult to reason about. I'm thinking it would make more sense to have the add_content() closure return an enum that will be actionable by the caller:

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 Option<MenuState>, so the conditions can return Some(MenuState::Close) or None when there is no action to perform. This is a matter of personal preference.

Going over the code, there is also a public callback named get_submenu which returns a value. I'm not entirely sure what this is for. Shouldn't sub-menus share all (most?) of the same code as the main context menu? There is probably a good reason for this separation, I'm just unfamiliar with the evolution and design of this PR.

@mankinskin
Copy link
Contributor Author

Ah, MenuState::get_submenu shouldn't be public, thanks for pointing out. the root menu and sub menus are basically the same, only that the root also has to store the Id of the element it was attached to. This is to discriminate two different context menus.

return an enum

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 context_menu_with_state in addition to that if you ever need to manipulate menu state more extensively.

@mankinskin
Copy link
Contributor Author

mankinskin commented Jul 8, 2021

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.

Peek 2021-07-08 16-18

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

@mankinskin
Copy link
Contributor Author

@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.

@parasyte
Copy link
Contributor

parasyte commented Jul 8, 2021

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.

@mankinskin
Copy link
Contributor Author

The MenuState only allows creating submenus and closing the menu hierarchy. Everything else is private. I don't see another way letting users create submenus in a closure than passing them a handle to the state they need to interact with. Anything else would require probably a much tighter API that would have to mimic Ui to manipulate MenuState behind the scenes..
The MenuState is completely opaque and only exposes close() and submenu(). I don't see a problem with this.

@mankinskin
Copy link
Contributor Author

mankinskin commented Jul 8, 2021

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.

Peek 2021-07-09 01-52

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.

@mankinskin mankinskin force-pushed the context-menu branch 2 times, most recently from d75870a to 18e3dc7 Compare July 9, 2021 13:53
@mankinskin mankinskin force-pushed the context-menu branch 3 times, most recently from 82df857 to cfb3ba3 Compare July 23, 2021 20:08
@mankinskin mankinskin force-pushed the context-menu branch 3 times, most recently from ea3e39a to 23e1886 Compare July 27, 2021 01:08
@mankinskin
Copy link
Contributor Author

I think I will also look into implementing submenus in the existing menus this way. It will pretty much work the same way.

Copy link
Owner

@emilk emilk left a 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.

egui/src/containers/window.rs Outdated Show resolved Hide resolved
egui/src/context_menu.rs Outdated Show resolved Hide resolved
egui/src/context_menu.rs Outdated Show resolved Hide resolved
egui_demo_lib/src/backend_panel.rs Outdated Show resolved Hide resolved
egui_demo_lib/src/wrap_app.rs Outdated Show resolved Hide resolved
egui/src/context_menu.rs Outdated Show resolved Hide resolved
egui/src/context_menu.rs Outdated Show resolved Hide resolved
egui/src/ui.rs Outdated Show resolved Hide resolved
egui_demo_lib/src/apps/demo/drag_and_drop.rs Outdated Show resolved Hide resolved
egui_demo_lib/src/apps/demo/drag_and_drop.rs Outdated Show resolved Hide resolved
@mankinskin
Copy link
Contributor Author

mankinskin commented Aug 17, 2021

@emilk Thanks for reviewing this! I agree about the API, that should all be hidden in Ui. We do need some way to create a submenu and close the menu though, and they need access to some state for the context menu. If this state was managed internally, we would have this interface return Options or Results when the Ui is not of a submenu, i.e. there is no internal menu state. I think this would be kind of unsound and a potential risk, because people would have to call .unwrap() on something like ui.submenu(..) when there is really no reason there wouldn't be a menu state when they are inside a context_menu builder. So I am thinking about an API like this

.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 unwrap calls are needed. also the menu_item and sub_menu builders would fit with the other widget builders in Ui. The menu_item builder would be needed to have specialized visuals for menu items, instead of using the regular button widget, which for example doesn't respect if a submenu is open and the cursor is moving towards it, i.e. there shouldn't be any visuals on other items even when they are hovered shortly. Maybe the API would even benefit from some getters for these conditions so people can integrate their own widgets more easily.

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 MenuUi which would be a superset of the Ui API and always has a menu state to use. That would hide the menu_state management and avoid unwrap.

@coderedart
Copy link
Contributor

coderedart commented Aug 25, 2021

don't know if this is the right place for it to mention. i am waiting on egui menus/submenus for my app. as my app is a rewrite/port of another app that had custom ui framework, i read upon a couple issues they had with menus. thought i should mention them here for consideration.

  1. What happens if menu is too big, like bigger than the height of window/monitor, are the menus beyond the border inaccessible? or will the menu have scrolling? or will there be a new column to show the rest of the items.
  2. is it possible to have checkboxes in menu items, and will the menu disappear when clicked on the checkbox to toggle it, so if the user wants to toggle multiple checkboxes, does he/she need to open menu, toggle item, and repeat ? the use case for it is when there's options that are toggleable and when enabled those options themselves have suboptions.
    the following screenshot should explain what i'm trying to say.
    image
    image

@mankinskin
Copy link
Contributor Author

@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.

@emilk
Copy link
Owner

emilk commented Oct 25, 2021

cargo clippy and cargo fmt are still unhappy!

@mankinskin
Copy link
Contributor Author

I will add the changelog note tomorrow 👍

@emilk emilk merged commit 46fb9ff into emilk:master Oct 26, 2021
@mankinskin
Copy link
Contributor Author

So happy to contribute to this awesome project! Thanks for all the support :) was great fun

@mankinskin
Copy link
Contributor Author

but what about the changelog now @emilk ? I didn't add that yet. But it is probably just one line anyways..

emilk added a commit that referenced this pull request Oct 26, 2021
Follow-up to #543

* Add entry to CHANGELOG.md
* Add entry to contributors in README.md
* Improve documentation
* Simplify demo
@emilk
Copy link
Owner

emilk commented Oct 26, 2021

I just pushed a line to the changelog in 41f77ba!

@emilk
Copy link
Owner

emilk commented Oct 26, 2021

I have just created an egui discord server: https://discord.gg/qzvmcrUe - come join if you like!

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.

5 participants