Menus that use data #1625

jneem · 2021-02-26T17:55:14Z

This is a draft of my attempt at #965

jneem · 2021-02-28T16:33:11Z

I think this is ready for a look.

I think the new mac behavior is an improvement over what was there before, although I haven't been able to test it. Now, it keeps track of the most-recently-focused window with a menu and always treats that one as the global app menu. Previously, it would set the menu whenever the focus changed, but it would also change it on every SET_MENU command, meaning that a background window changing its menu would override the global app menu.

cmyr · 2021-03-03T01:52:26Z

@jneem very cool, will take a look at this in the next day or two!

cmyr

This looks great! I've left some preliminary comments inline, although I haven't tried playing around with it. My initial impression is that this looks like a huge improvement, though, thanks for taking it on :)

druid/src/app.rs

druid/src/menu/mod.rs

cmyr · 2021-03-07T17:13:37Z

druid/src/menu/mod.rs

+    needs_refresh: Option<MenuPredicate<T>>,
+    item: MenuItem<T>,
+    children: Vec<MenuEntry<T>>,
+    // bloom?


This would require children to have some sort of identity, and for there to be some benefit in only visiting specific children. Do you think this would make sense?

The MenuItems are supposed to have unique ids, so blooms would basically allow us to traverse directly to the right MenuItem when getting a menu command from the platform.

I didn't do it yet because:

the menu probably can't be big enough (or commands common enough) for traversing the menu tree to be a bottleneck, and

it might actually be more expensive to construct bloom filters each time the menu is rebuilt (I have more to say about rebuilding, but I'll put that elsewhere later)

druid/src/menu/mod.rs

jneem · 2021-03-08T21:33:14Z

Thanks for your review! I think I addressed your comments except for the context_menu one. Next, I'm going to go spend some time dogfooding this in my own druid project.

One remark about rebuilding: this implementation internally rebuilds the entire druid-shell menu from scratch each time anything changes, but I think that as long as the underlying platform API cooperates (I haven't checked anything except for GTK), we could do the updates entry-by-entry. It's a bit of a pain trying to coordinate the druid-shell changes across backends, so I haven't tried to do it here.

cmyr · 2021-03-08T21:45:00Z

Okay I'll take a closer look at the context menu situation and play around with it a bit; context menus are often presented in situations where the user doesn't have access to the root data (like in some widget deep in the tree) so I want to make sure there's a reasonable solution, there.

cmyr

Okay so I updated runebender to use this and ran into a few problems; the major one is that this breaks the built-in commands for open files etc.

There are a few other thoughts inline; in any case this is exciting. :)

cmyr · 2021-03-11T20:53:40Z

druid/src/win_handler.rs

-        }
+                .as_mut()
+                .map(|m| m.event(queue, cmd_id, data, env)),
+        };


So this was a bit fragile, and these changes break some things. The basic problem is that this assumes that all menus attached to a window are only sending events that are targeted to that window, which may not be case in general and which has another particular consequence: we use a bunch of special built-in commands for things like opening files, and these need to be handled outside of Inner; that's why the previous implementation returned a Command.

From a higher-level, I think in general it's best to try, wherever possible, to reuse existing code paths. We have a mechanism for dispatching commands from the WinHandler, and that code path is well-worn, and ensures that things like update happen correctly. By taking this short-cut and calling event directly, we're introducing a new codepath and that means we would have to take care, in the future, to ensure that any changes around how update happens or how events are routed also takes care of this special case. Because all events trigger updates of all windows, we need to make sure that all events originate outside of the Inner struct.

Thanks for the pointer! I ran into this myself today, but hadn't tracked down the reason yet...

Ok, so having dug into this a bit more, I'm not sure I understand your comment. m.event is the event method on MenuManager, which is basically responsible for pushing commands on the command queue, and then those commands get dispatched using the usual AppState command dispatching/updating mechanism. I did have a bug, however, where the target wasn't being set, which caused some commands to get dropped...

oooh, so here the menu receives the cmd_id, and then submits the command via the context? That sounds good. 👍

druid/src/menu/mod.rs

cmyr · 2021-03-11T21:10:41Z

druid/src/menu/mod.rs

+
+impl<T: Data> ContextMenu<T> {
+    /// Create a new [`ContextMenu`].
+    pub fn new(


So playing around with this in runebender, I wonder if we can simplify the context menu case?

My basic thinking: a context menu is a modal. It should never need to be updated, since it is dismissed if anything is clicked. Additionally, in a non-trivial application a context menu is going to be presented somewhere fairly deep in the widget hierarchy, where the root data is likely to be several lenses away.

For me, I think that having access to the root data (especially when it's basically untyped) in the context menu just isn't that helpful. I would much prefer if, in this case, I instantiate a single menu object and display it each time.

Does this make sense? It's a bit different from the API for other menus, but ultimately feels simpler than what we have now and potentially more flexible, since you can pass in whatever data you want when building your menu.

druid/src/menu/mod.rs

jneem · 2021-03-12T03:27:55Z

Dunno why, but github wouldn't let me reply to this directly:

So playing around with this in runebender, I wonder if we can simplify the context menu case?

My basic thinking: a context menu is a modal. It should never need to be updated, since it is dismissed if anything is clicked. Additionally, in a non-trivial application a context menu is going to be presented somewhere fairly deep in the widget hierarchy, where the root data is likely to be several lenses away.

For me, I think that having access to the root data (especially when it's basically untyped) in the context menu just isn't that helpful. I would much prefer if, in this case, I instantiate a single menu object and display it each time.

Does this make sense? It's a bit different from the API for other menus, but ultimately feels simpler than what we have now and potentially more flexible, since you can pass in whatever data you want when building your menu.

Are you just talking about the menu construction here, or more generally about the various data-accessing callbacks in the menu? I definitely agree that we don't need the "builder" callback. I also think that things like enabled taking a data callback are overkill, but I do think I'd like to have access to the (locally lensed, ideally) data in the on_activated callback. (One of my motivations for doing this rewrite was that I got annoyed at how menus forced me to do everything through commands.) What would you think about this API change:

make the context menu constructor take just the menu, not a "builder" function
make the context menu use the local data, the same way that subwindows do (I haven't thought through how this would be implemented, though)
add convenience functions for non-dynamic properties, like the way we have both hotkey and dynamic_hotkey

cmyr · 2021-03-12T03:39:12Z

Those ideas make sense, I think? I'm not sure about the local data bit; it isn't totally obvious to me how that would work. What I could imagine, although it's a bit less ergonomic, is making the caller build the menu themselves, using the available data and env, and passing the PlatformMenu to show_context_menu?

I'm generally a fan ofconvenience methods.

jneem · 2021-03-18T17:18:42Z

Ok, I think I've addressed the issues except for the question about access to the local data in context menus (which I haven't even looked into, and I'd prefer to punt on for now). What I did instead was make it so that all the relevant menu-building functions have both dynamic and static versions, so it's now much more convenient to construct a menu based on the current data that doesn't need access to future data.

cmyr · 2021-03-18T20:39:02Z

Will take another swing at this soon!

cmyr · 2021-03-19T02:01:52Z

I really like the API tweaks here, but I do have one lingering problem with the ContextMenu. Is there some way to let me create a ContextMenu with just a single, concrete Menu object? It's really a bit of a brain-twister that the function I use to create a menu is passed a type I don't have easy access to, and can not easily extract the information I need from, because the path from there to here involves passing through multiple non-trivial lenses.

I'm not trying to be difficult, here; I legitimately spent 20+ minutes getting the context menu to work in runebender, and it ended up with me just cloning my local data and moving that into the closure, ignoring the closure arguments altogether, like:

            let data = data.clone();
            let menu: ContextMenu<AppState> = ContextMenu::new(
                move |_, _| crate::menus::make_context_menu(&data, mouse_pos),
                m.window_pos,
            );

and this doesn't feel like a great idiom.

Other than that, this looks good. The window menus are working now and I'm not seeing the problem with dropped commands, although when I'm slightly more alert I will go and review my last comment there to make sure I'm not totally losing it.

Thanks again, this feels great!

jneem · 2021-03-19T03:15:25Z

Sorry, with all the other API changes that were supposed to make context menus more convenient, the one you actually asked for slipped through the cracks...

cmyr · 2021-03-19T13:07:21Z

Great that looks like a solid simplification.

Also I'm seeing a warning from CI about an 'unread field'?

I'm going to take one more pass through the root command handling logic, but modulo that this looks good to go!

cmyr

Looks good, thanks again for taking this on, it's a solid improvement.

druid/src/win_handler.rs

cmyr · 2021-03-19T13:09:57Z

druid/src/win_handler.rs

-        }
+                .as_mut()
+                .map(|m| m.event(queue, cmd_id, data, env)),
+        };


oooh, so here the menu receives the cmd_id, and then submits the command via the context? That sounds good. 👍

Menus are now updated semi-automatically from the app data.

Menus are now updated semi-automatically from the app data. The public menu API has been substantially changed. Fixes linebender#965

jneem marked this pull request as ready for review February 28, 2021 16:30

cmyr added the S-needs-review waits for review label Mar 5, 2021

cmyr reviewed Mar 7, 2021

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Mar 7, 2021

cmyr requested changes Mar 11, 2021

View reviewed changes

cmyr added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Mar 18, 2021

cmyr approved these changes Mar 19, 2021

View reviewed changes

jneem added 4 commits March 19, 2021 13:36

Redo the menu public API.

9e73f74

Menus are now updated semi-automatically from the app data.

Simplify context menus.

8e66c59

Silence a warning.

f820621

Update changelog.

f554501

jneem merged commit 8e09a6d into linebender:master Mar 19, 2021

richard-uk1 pushed a commit to richard-uk1/druid that referenced this pull request Apr 6, 2021

Menus that use data (linebender#1625)

094adca

Menus are now updated semi-automatically from the app data. The public menu API has been substantially changed. Fixes linebender#965

cmyr mentioned this pull request Apr 16, 2021

Convenience traits for code generic over contexts. #1611

Closed

maan2003 removed the S-needs-review waits for review label May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menus that use data #1625

Menus that use data #1625

jneem commented Feb 26, 2021

jneem commented Feb 28, 2021

cmyr commented Mar 3, 2021

cmyr left a comment

cmyr Mar 7, 2021

jneem Mar 8, 2021

jneem commented Mar 8, 2021

cmyr commented Mar 8, 2021

cmyr left a comment

cmyr Mar 11, 2021

jneem Mar 12, 2021

jneem Mar 18, 2021

cmyr Mar 19, 2021

cmyr Mar 11, 2021

jneem commented Mar 12, 2021

cmyr commented Mar 12, 2021

jneem commented Mar 18, 2021

cmyr commented Mar 18, 2021

cmyr commented Mar 19, 2021

jneem commented Mar 19, 2021

cmyr commented Mar 19, 2021

cmyr left a comment

cmyr Mar 19, 2021

Menus that use data #1625

Menus that use data #1625

Conversation

jneem commented Feb 26, 2021

jneem commented Feb 28, 2021

cmyr commented Mar 3, 2021

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented Mar 8, 2021

cmyr commented Mar 8, 2021

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented Mar 12, 2021

cmyr commented Mar 12, 2021

jneem commented Mar 18, 2021

cmyr commented Mar 18, 2021

cmyr commented Mar 19, 2021

jneem commented Mar 19, 2021

cmyr commented Mar 19, 2021

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment