-
Notifications
You must be signed in to change notification settings - Fork 899
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
feat: added MacOS menu #1583
feat: added MacOS menu #1583
Conversation
@kchibisov, you mentioned you had some ideas here. Do you mind sharing them here? |
# Conflicts: # Cargo.toml
Cool feature. Hope this gets merged! |
+1 for this; I won't use winit because winit apps don't behave anything like a standard Mac app, and this is a big step forward. This is very English-centric, should you perhaps call NSLocalizedString (or if winit has a blessed localization solution, possibly use that) for the menu & item titles? |
This has been open six months now, what do we need to do to get it merged? |
I haven't seen macOS maintainers in winit for similar amount of time, so someone with macOS should review it. I have macOS machine now, will try to look into it tomorrow, but I'm not a macOS maintainer. |
I found a error myself where the application is not interactable initially launched. If you switch app, and then back then you can interacte with the menu. We should handle this in this PR before we merge. |
Also, how exactly can my app observe that a menu item has been clicked? Sorry if this is an obvious question. |
I think that we need to think about the API. The way it works right now is that I think that a better API would be adding a new WindowEvent case (compiled optionally for macos), called something like The way to do this is adding a new struct pub struct MenuItem {
name: String,
...
} and then add some APIs to Window to manage (add, delete) menu items. When you register a new menu item, winit dynamically adds a new method (named the same as the menu item) to the application delegate. When the application delegate receives the message that the item has been clicked, it simply injects a menu item event into the event stream. |
The thing is that I'm not sure that there should be any special handling for them, since the handling is always the same for everyone? Like if you do I'm not sure about providing custom API right now, since it seems like a very complicated approach and requiring user to match arbitrary strings isn't rust API either. Right now I think providing a common menu handler for everything(that's what glfw does for example) seems like a reasonable enough solution. |
@kchibisov I don't follow. When I register these menu items and one gets clicked, how does is that information delivered to my application? |
With the current API it doesn't, macOS just calls a selector you've specified, and I'm not sure if you can generate selectors on the fly, especially if we talk about rust side, however I'm not familiar with macOS API. @casperstorm do you aware if there's a way to provide a custom selector and identify in that selector from which method you've been called, thus all menu items can share a selector? |
@kchibisov there is not a simple way to provide a method that would be called by all menu items. You can always do something like forwardInvocation https://stackoverflow.com/questions/9800293/equivalent-of-ruby-method-missing-in-objective-c-ios however then you have to know if the method you have been passed is a method from menu item. |
@kchibisov but yes, you can generate selectors on the fly with NSSelectorFromString. The trick is that we would be doing this only on the objective c side, on the rust side, we would just inject a |
Another way would be to use the NSMenuItem tag property to allow all menu items to share the same selector. This approach might make more sense since you don't have to register the selectors dynamically. We could then have a struct MenuItem<T: From<u64> + Into<u64>> {
tag: T,
title: String,
} you would then have to implement an enum that's convertible to u64. winit would then insert a NSMenuItem with selector set to "handleMenuItem" and with the tag set to tag converted to u64. Then, in the handler, we would convert the tag back into I think this is as good as it gets to be honest. |
I think it's worth distinguishing between "winit needs working menus to act like a standard mac app" and "winit should provide a general menu interface". The latter is a huge problem, with implications for Windows and Linux as well. The former only implies handling a few standard commands in standard ways. I don't think we should gate this PR on developing a general cross-platform menu interface. About, Hide/Show, Quit, can be handled in the standard ways without modifying winit's APIs or sending new events to the user. The other items can be disabled if they can't be omitted. |
It doesn't need to be general, it can be conditionally compiled for now, but ideally I should be able to use winit to add new menu items right? |
Speaking for myself (but also as someone used to consuming GLFW, SDL, etc) I really don't care about custom menu items. I care about "clicking the menubar shows a menu" and "command-Q works as expected and by default". Not saying winit shouldn't get a customizable menu system in the future, I just think that this is a critical step forward that doesn't require it, and that will be delayed by trying to get that right. |
@adamncasey if you have a solution(code) to a global problem of menu items for macOS, I'm all ears, but I don't care about custom menu items, especially if you really want them you can add some in downstream without many issues. |
What this PR delivers already, is all that GLFW and SDL deliver. Parity there is a great first step, and doesn't preclude doing something better in future. |
That's my point as well, since it does precisely what glfw does, and winit is kind of on par with glfw in what it should do (maybe except some minor things like providing OpenGL/Vulkan surface, but that's it). |
Btw instead of |
Also, there's this project called stella2 that does cross platform menu handling https://github.com/yvt/Stella2/blob/c0524f1a18ad4ee72460a0db0d2bdb9fcb48e3a2/stella2/src/view/global.rs#L16 |
I think this PR should be merged as-is. Allowing for custom menus can be done later but this is something that is needed now. |
@kchibisov let me know if there's anything I need to do here. |
I think they're waiting on i18n of the menu text. You may as well do that anyway because it's important |
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.
I welcome this addition, I think it's something that winit
should definitely support out of the box!
However, I'm working on a library myself for supporting this, and for that it would be really nice this was an optional feature (either compilation or a setting on the event loop), so winit
and my custom library don't both try to set the main menu!
Haven't looked at NSAutoreleasePool
in this review, don't know much about it.
|
||
// Seperator menu item | ||
let sep_first = NSMenuItem::separatorItem(nil).autorelease(); | ||
|
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.
We should add the Services
menu here (can be done by creating a menu item with an empty menu, and registering that as the servicesMenu
)
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.
Hm, not sure it make sense if we don't have any services to add?
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.
I looked into how the Services menu is used. Winit uses a custom NSView
subclass for the entire window surface. Applications and custom NSView
types have to implement a certain protocol (see "Using Services" in the Apple Developer Documentation Archive). More specifically the NSView
has to provide an implementation for validRequestorForSendType
through which it must specify whether the currently selected thing is a valid target for a service that operates on the provided types of data. However winit has no idea what is selected currently. Given that winit doesn't provide APIs to handle validRequestorForSendType
messages, it's currently not possible to use the Services menu through winit (if I understand correctly). This is somewhat related to #1759 in that it's a macOS specific "event" that a higher level code may want to be able to handle.
Anyhow I don't think we should be concerned about that for this PR. So with all that, just like @casperstorm wrote, it seems there's no point in adding the Services menu.
Can't you grab the main menu after the fact and modify it? |
Easily, but would like to avoid adding creating a menu twice, but mostly it's because I'm not sure if changing the main menu after it's been assigned would introduce flicker |
You won't have to create a second menu if a menu is already assigned. Pretty standard "only create if it doesn't already exist" pattern. |
Sure, I could just support that usecase in my library (which it probably should anyhow), and the flicker, if even present, is probably not that big of a deal; it was just a nicety for me, as a user, to have 😉 |
It's not uncommon for an application to launch without a menu bar and then take seconds to fill in the categories. Seriously, if your library works fast enough for the delay to be called nothing more than a flicker, I'll be impressed. |
As far as I understand My knowledge is very limited on this and for the most part I don't know what I'm talking about so please correct me if I'm wrong. If I'm right however, then I don't think this PR should be concerned with localization. |
@casperstorm if you address @madsmtm's comments then I think I would be happy to merge this PR. |
Thanks for the suggestions everyone. I have updated, and fixed a few things. There's still some outstandings I would like some help and feedback to solve. |
@casperstorm I added a commit to this PR, please pull and test to see if it works as expected. If you are fine with the changes I made, I think we can merge this. |
Thanks, @ArturKovacs. It works as expected here. |
Shoot! I completely forgot about the activation policy thing. I removed the line that set the activation policy and it still seems to work fine for me. What was the reason you originally put it there @casperstorm? Does my latest commit still work for you? |
Last commit works for me. Honestly I can't remember why I put it there. It's been quite awhile :) |
Thanks for the contribution! |
Thank you @casperstorm for doing the work and @ArturKovacs for getting it merged! Been waiting a long time for this one :) |
This is an WIP PR which shows a way we could add a generic MacOS menu into winit.
Let me know what you think, if this is something you could see in this repo, then we could collaborate about the details, formats, where to initialize the menu etc.
cargo fmt
has been run on this branchcargo doc
builds successfullyCHANGELOG.md
if knowledge of this change could be valuable to usersFixes #41.