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

Menus #193

Open
mworzala opened this issue Feb 2, 2022 · 26 comments
Open

Menus #193

mworzala opened this issue Feb 2, 2022 · 26 comments

Comments

@mworzala
Copy link
Contributor

mworzala commented Feb 2, 2022

Is there any plan for implementing menus? Specifically window menus, task bar context menu, and tray menus (though this requires more thought on macOS perhaps).

I assume there will be some wrapper of the base menu elements, but interaction poses a question: callbacks? events? something else entirely?

There is also the role property from Electron for system defined menus which needs to be considered.

@tonsky
Copy link
Collaborator

tonsky commented Feb 3, 2022

I would like to have menus! I would even say they are essential. I have no idea on the API design or even scope yet

@bumfo
Copy link
Contributor

bumfo commented Apr 6, 2022

@mworzala Do you have some basic thoughts on Menu API design? I would like to implement some working draft first before taking more considerations.

@bumfo
Copy link
Contributor

bumfo commented Apr 10, 2022

I would like to have menus! I would even say they are essential. I have no idea on the API design or even scope yet

@tonsky I came up with some basic API design, along with some code to sync it with NSMenu & NSItem on macOS.

Menu menubar = Menu.make();
menubar
    .addItem(new MenuItem().setTitle("App").setSubmenu(new Menu()
        .addItem(new MenuItem().setTitle("About"))
        .addItem(new MenuItem().setTitle("Quit"))))
    .addItem(new MenuItem().setTitle("View").setSubmenu(new Menu()
        .addItem(new MenuItem().setTitle("Enter Full Screen"))));

if (menubar instanceof MenuMac menuMac) {
    MenuMac.setApplicationMenu(menuMac);
}

working

Although it looks a little bit verbose, it works fine so far and can be used as some scaffold to some cleaner, declarative design. Let me know if you have some better ideas to this.


Also, event is not supported yet, so the menu isn't functional for now. Maybe we need a separate AppEventListener for this, and let the user to dispatch to active window, if necessary.
And the tricky part is, Windows attach menus to window, instead of App. I haven't came up with some good abstraction over this yet.

@bumfo
Copy link
Contributor

bumfo commented Apr 10, 2022

There is also the role property from Electron for system defined menus which needs to be considered.

About the role part, macOS seems to recognize "Help" as help menu, and "View" as view menu. service and window menu needs to be set in code, but we could adapt this convention.

void jwm::MenuMac::setTitle(NSString* title) {
    if ([title isEqualToString:@"Window"]) {
        NSApp.windowsMenu = fNSMenu;
    }
    // ...
}

@mworzala
Copy link
Contributor Author

Sorry for the late reply - I did not previously come up with any api for menus.

I do have some comments/questions (opinions):

  • At the moment, what happens if you add another menu item after setting the app menu? It should either change in response to mutation or Menu should be immutable.
  • I don't have any issue with using the name instead of an explicit role property if thats what macOS does. I haven't been able to find documentation on that, please let me know if you have.
  • r.e. functionality of menus, I assume that events will be used as opposed to callbacks (as have been in most places), but I would say it makes sense to either dispatch to the key window or to all windows (assuming Menus are scoped to Windows concept to accommodate Windows)
  • There is also the consideration of any support for custom views inside menus. macOS allows any view to be added to a menu, but particularly a search bar (see Safari Help menu) might be helpful.

And the tricky part is, Windows attach menus to window, instead of App. I haven't came up with some good abstraction over this yet.

In other cases the more restrictive case has been taken. For example, Window#setProgressBar is not on App because Windows (and X11, though I guess that is a slightly weirder case) handle it per window, even though macOS handles it per app.

@moritzruth
Copy link

Mutability: I would prefer Menu to be immutable. In most apps the menu rarely changes, so it shouldn’t be a problem performance-wise.

Callback vs. event: Using events would mean having menu-related code in two different locations. The existing events all make sense because they belong to a window, but menu items are separate objects. Using callbacks would also eliminate the question to which window the event should be dispatched.

@bumfo
Copy link
Contributor

bumfo commented Apr 11, 2022

And the tricky part is, Windows attach menus to window, instead of App. I haven't came up with some good abstraction over this yet.

In other cases the more restrictive case has been taken. For example, Window#setProgressBar is not on App because Windows (and X11, though I guess that is a slightly weirder case) handle it per window, even though macOS handles it per app.

What do you expect to set menus for a window, while the OS has only App level menus?
Option 1: Setting a menu for any window sets the App menu.
Option 2: Setting a menu applies to that window only. Menus are switched on window focus. But how about event scope? What if two windows share the same menu?

@mworzala
Copy link
Contributor Author

What do you expect to set menus for a window, while the OS has only App level menus?
Option 1. The library should match the os. Users can set the menu on window focus if they desire.

Callback vs. event: I agree that callbacks would be generally easier to use for the developer, though it does diverge from the use of events everywhere else in the library. Im not sure the best solution there.

@tonsky
Copy link
Collaborator

tonsky commented Apr 12, 2022

I think we can resolve macOS / Windows problem by allowing to create a menu per window and switching them on macOS when user switches between app windows.

We can probably do without custom widgets, text only. At least for starters.

As for callbacks/events, I am not sure. Events do sound a little simpler and will be in line with the rest of the design. But what I like the most, as menu changes over time, you don’t have to keep track of callbacks.

Questions I am curious about and would like to see in design draft:

  • How will we identify menu item in the event? Title / full path / unique keys (user-assigned?)
  • How to enable/disable menu items?
  • Most menus are static (created once), but some are dynamic (e.g. recent tabs is Firefox History menu). How will we update menu after it’s been created? Recreate everything? Just replace some sub-path?
  • Delimeters/grouping. Is it a thing on Windows?
  • Shortcuts

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

Forcing menus to be window specific should mostly work, which is the same as what AWT does. However, this approach has a few drawbacks:

  1. When switching to a window that has no menu, should we keep the current menu or clear it? Note that standard macOS apps should always have menus, especially App, Window & Help menus.
  2. Auxiliary windows should not have menus, but menus should be the same as main window when they got focused. But this cannot be achieved via simply not changing menus, e.g. when switching to another window with different menu, and switch to the auxiliary window of the main window.

My suggestion is to provide a way to set App menu for all platforms, which is applied to every window (which isn't auxiliary) by default, if not set explicitly. When menus are set explicitly per window, we switch the menus on macOS.

Electron does similar thing IIRC.

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

What I don’t like about this approach is that you have to “undo” behavior you don’t need (remove/replace default menu) instead of doing what you need (adding menus). It’s also one more thing to keep in mind.

I like the simplicity of: set menu and your window gets a menu. Need same menu on multiple windows? Set same menu on multiple windows. It’s explicit, it’s one API to remember.

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

The callback approach has one crucial benefit, that the developers don't need to separate menu declarations and actions, and don't need to write ugly switch to identify menus.

The other benefit is that, callback based approach can be easily used to implement event based approach, one can simply pass the same callback for every menu item, and use switch to identify them. Being able to automatically enable/disable menus based on set/unset of callbacks is also a side benefit, which is impossible in the (fully dynamic) event approach.

And about identity, there's one natural identity created by JVM, which is the object's identity. The object used to create menus can also be used to update menus, enable/disable menu items.

And finally, delimiters and shortcuts can be implemented as a property of MenuItem.

The above API design does not sound fancy, but is robust enough to use. Fancy APIs can be built on top, to provide syntactic sugars for limited but frequent usages, such as building menus from template, the Electron style.

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

What I don’t like about this approach is that you have to “undo” behavior you don’t need (remove/replace default menu) instead of doing what you need (adding menus). It’s also one more thing to keep in mind.

I like the simplicity of: set menu and your window gets a menu. Need same menu on multiple windows? Set same menu on multiple windows. It’s explicit, it’s one API to remember.

Caveat: auxiliary windows (e.g. Tool windows, Dialogs) should not have menubar on Windows, but they do need App menubar on macOS as usual. This is the main drawback of AWT menus, and there's no way to overcome without explicit App menu concept.

Bottom line: don't force developers to write a lot of platform dependent code, in order to solve a simple platform dependent difference.

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

Another window/mac menubar misalign is the first menu. Windows apps tend to put application level menus into "File", such as Quit, Preferences, while mac apps put them in the first menu with the name of the app.

If we don't do something in the API design, the developer either get the File menu displayed with the name of the app, or get an empty first menu on macOS. Neither is optimal.

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

Oops there's one problem of shortcuts if we allow window specific menus.

In macOS, menus are App level, so shortcuts are global, which makes sense, since most shortcuts are inherent to be app level, such as Preference, Quit. It does not make sense if the user switch to an Auxiliary window (e.g. Dialog, Tool window) and cannot Cmd + Q.

But in Windows, menus are attached to windows, therefore some menu items are logically global, but some are local to the window. It's way too verbose to force the developer to specify whether a shortcut is app specific or window specific, and even if we allow this, how about two different menus set on two windows, and having an item with the same global shortcut with different behavior?

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

You bring a lot of interesting points!

  • I don’t have a good argument for/against callbacks/events. I guess either is fine.
  • I don’t really like disabling menu items via adding/removing callback. I prefer these to be separate. Mainly because I want to set callback once and never think about it again
  • Object identity might be inconvenient because you have to store it somewhere. E.g. somewhere in the middle of my program I decide that I need to disable Tools > Sign In. With object identity, I’d have to keep a reference to Sign in menu object somewhere, e.g. as a global variable. I would prefer to do something like Menu.findByKey("sign_in").disable() instead.

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

Windows is the same in context of window-level menus as macOS if you consider multi-document app on macOS. I guess just attach menus to windows, put “app-level” to every window, no?

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

First menu (File/app name) problem is interesting. What do AWT, JavaFX, Electron do?

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

What I don’t like about this approach is that you have to “undo” behavior you don’t need (remove/replace default menu) instead of doing what you need (adding menus). It’s also one more thing to keep in mind.

I like the simplicity of: set menu and your window gets a menu. Need same menu on multiple windows? Set same menu on multiple windows. It’s explicit, it’s one API to remember.

There's another API design principle, that makes correct behavior easier than mistakes. If menus are in "addition logic", it's quite easy to have a window without app menu, making the app inconsistent. In contrast, for most apps, the developer need only to set one default menu, and everything works fine automatically.

The default menu approach works because most apps have one single main window (or multiple instance of it), and the rest of the windows are all auxiliary, which inherits shortcuts, shares the same app menu, without displaying a menubar on them (on Windows).

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

OK there's one more problem, how to identify the active window in menus, no matter the callbacks way, or the event way (are menu events app level, menu level or window level?).

And if we goes the callbacks way, and allow window specific menu, what if the callbacks are created in one window (therefore references variables specific to that window), and is reused in another window, causing the event to be actually applied to the wrong window?

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

Note that macOS menus have target and selector set separately, so the selector is always performed on the right target (e.g. active window). But we cannot use the same approach, because it's way too Objective-C style.

@bumfo
Copy link
Contributor

bumfo commented Apr 13, 2022

I would prefer to do something like Menu.findByKey("sign_in").disable() instead.

I agree, object identities are useful, but there should be support in the API to identify them in easier ways.

Since the title of menu items should be unique locally (in the menu it belongs to), the path of the menu item is the identifier we get for free. Special needs can be supported by optionally setting custom menu item identifier, which has the scope of the root menu.

Changing title on the fly can be alternatively approached by show/hide different menus (or recreating subpath), so this isn't a problem.

The ugly (but robust, and yet can be improved with abstractions on top) design for this:

menubar.getMenuItem("Tools").getSubmenu().getMenuItem("Sign In").disable();

Example of (one of) the possible syntactic sugar on top:

menubar.findByPath("Tools > Sign In").disable();

Drawback: renaming title of menu items in the code needs text search and is error-prune.

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

Using text for id is not a good idea because of translations. We need keys, not labels

@tonsky
Copy link
Collaborator

tonsky commented Apr 13, 2022

OK there's one more problem, how to identify the active window in menus, no matter the callbacks way, or the event way

That’s the problem with callbacks. You do

var menu = new MenuItem("Sign In", () -> { System.out.println("Window 1" + window1); });
window1.setMenu(menu);

But now nothing stops you from adding the same menu to another window:

window2.setMenu(menu);

This might cause confusion (same callback, what if user made an assumption or even closed over window object?)

I also imagine this to be a common pattern for multi-document apps (create menu once and add it to every window). But callbacks should probably be different?

@bumfo
Copy link
Contributor

bumfo commented Apr 14, 2022

Sum up the above discussions, I propose the following API design draft:

  1. A Menu is specified per window, event is sent to the corresponding window.
  2. Menu objects are templates, meaning that they can be created once and reused on every window. Modification of templates only take effect when set again.
  3. id is optional, and is unique to the template, but must be set in order to identify it in events and disable / enable it. This enables fast prototyping where id is not used.

@tonsky
Copy link
Collaborator

tonsky commented Apr 14, 2022

Sounds great!

@bumfo bumfo mentioned this issue Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants