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

[macOS] Add default Window and Help menus, allow special menu customization. #83987

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Oct 26, 2023

More customizable version of #83408


@bruvzg bruvzg added this to the 4.x milestone Oct 26, 2023
@bruvzg bruvzg force-pushed the macos_window_and_help branch from cad48f7 to 069dd12 Compare October 26, 2023 08:54
@bruvzg bruvzg marked this pull request as ready for review October 29, 2023 18:16
@bruvzg bruvzg requested review from a team as code owners October 29, 2023 18:16
@ztc0611
Copy link
Contributor

ztc0611 commented Dec 2, 2023

Excited for this one! 😄 Having ⌘+M for minimize is a great usability improvement. I also might actually use multi window mode in the editor whenever this merges.

However, the editor settings under the Apple menu doesn't work for me, only the original one under Editor. ⌘+, also activates the binding under the Apple menu, so it doesn't do anything besides blink the menu title.

Edit: I fixed it, there was a binding issue. How do I get the fix to you?

@Calinou
Copy link
Member

Calinou commented Dec 7, 2023

Edit: I fixed it, there was a binding issue. How do I get the fix to you?

I suggest pushing a branch with your changes to your fork and linking it here.

@ztc0611
Copy link
Contributor

ztc0611 commented Dec 7, 2023

Pretty minor adjustment. 🙂 ztc0611@be3a6e5

@bruvzg bruvzg force-pushed the macos_window_and_help branch from 069dd12 to 2a3354d Compare December 7, 2023 22:02
@bruvzg
Copy link
Member Author

bruvzg commented Dec 7, 2023

Pretty minor adjustment.

Thanks. Rebased and added missing apple_menu->connect.

doc/classes/PopupMenu.xml Outdated Show resolved Hide resolved
Comment on lines 236 to +240
"_main" - Main menu (macOS).
"_dock" - Dock popup menu (macOS).
"_apple" - Apple menu (macOS, custom items added before "Services").
"_window" - Window menu (macOS, custom items added after "Bring All to Front").
"_help" - Help menu (macOS).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should've probably identified this before, but this has to be done with an enum rather than a bunch of hardcoded strings. We went to great lengths to reduce the dependency on strings in Godot 4 and it's a shame to start introducing something like this back.

I'm not sure how much of a bother it would be to rework it now. Although given it only affects macOS for now, I think breaking compatibility right now is a valid option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be enum, since non-standard menus use it as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What non-standard menus and why couldn't they create their own custom IDs for this purpose instead of using strings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every global menu (or submenu) except the hard-coded list above. It can create its own ID (the only requirement is having some globally unique ID, so it can be RID issued by the DisplayServer, and it's probably would make more sense). But this will be compatibility breaking. Originally, it was intended to be used in the similar manner as PopupMenu string submenu property.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, it was intended to be used in the similar manner as PopupMenu string submenu property.

The way it works is an oversight and should've been changed in Godot 4.0. We're now moving away from strings there as well, see #85477.

Which is what I mean by my original comment, we're now introducing new API (or rather introduced a few versions ago) which follows a bad pattern from the old API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're now introducing new API (or rather introduced a few versions ago) which follows a bad pattern from the old API.

It's not new, API is from the 3.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, didn't realize we supported global menus in 3.x. Well, I still think we should be looking for a way to get away from strings, especially if we're going to be extending this feature more and more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question is, do we want to:

  • Assume that these DisplayServer functions aren't widely used and break compatibility (this won't break existing MenuBar / PopupMenus).
  • Add 35 compatibility functions (and String ↔ RID hashmap).
  • Or just keep it as is.

Changing ID's to RID would allow simplifying code quite a bit, so it's not a bad idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIWAGOS disclaimer :)

My general feeling is that I'm not fond of 48 macOS specific global_menu_* methods in the generic DisplayServer, but that's something I had already raised in #25656 and now I've accepted that it's just the way we can handle platform-specific APIs currently. But that's a different problem and out of scope for this discuss, it just colors my relationship to these APIs still :P

And so likewise for these string IDs, there's a lot of "sunken cost" that makes me wary of asking for major changes. I think there's room for improvement to have more user-friendly and self-contained API (random idea, a GlobalMenu singleton to host those 48 methods) and it can be worth thinking about, but at least for this PR it's also out of scope IMO, as this just reusing the current status quo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a bit of discussion I agree with the above. Let's just keep it somewhere in our mind that it would still be worth it to rework the API to organize it better and de-bloat some global classes. When we do that, we can also consider the RID alternative approach to simplify things.

scene/gui/popup_menu.h Outdated Show resolved Hide resolved
@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 13, 2023
@bruvzg bruvzg force-pushed the macos_window_and_help branch from 2a3354d to 0d44b50 Compare December 13, 2023 21:20
@YuriSizov YuriSizov merged commit 644e236 into godotengine:master Dec 18, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement macOS menubar Window section
5 participants