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

Move global_menu_* methods to a separate NativeMenu class. #87452

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 21, 2024

Implements godotengine/godot-proposals#8685

@bruvzg bruvzg added this to the 4.x milestone Jan 21, 2024
@bruvzg bruvzg force-pushed the native_menu branch 3 times, most recently from 4eaef59 to e4fe4df Compare January 22, 2024 05:47
@bruvzg bruvzg marked this pull request as ready for review January 22, 2024 06:24
@bruvzg bruvzg requested review from a team as code owners January 22, 2024 06:24
servers/native_menu.h Outdated Show resolved Hide resolved
servers/display_server.cpp Outdated Show resolved Hide resolved
servers/display_server.cpp Outdated Show resolved Hide resolved
servers/display_server.cpp Show resolved Hide resolved
servers/display_server.cpp Show resolved Hide resolved
platform/macos/native_menu_macos.mm Outdated Show resolved Hide resolved
platform/macos/native_menu_macos.mm Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Since we change the API we should mark it as breaking compat, even if we have compatibility methods. But it's a very much welcome change!

@@ -156,6 +156,7 @@ class DisplayServerX11 : public DisplayServer {
#ifdef SPEECHD_ENABLED
TTS_Linux *tts = nullptr;
#endif
NativeMenu *native_menu = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this? Can't we just add the variable and check later, if we ever add proper native menu support?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some dummy instance should exist, since it can be accessed in any platform. Also, even if we won't have global menu support we might implement native popup menu at some point (can be useful for the tray indicator menus, and I think the only way tray indicators can be done on X11).

@Mickeon
Copy link
Contributor

Mickeon commented Jan 25, 2024

The existing methods in DisplayServer should be marked as deprecated, shouldn't they? I don't see this being done in the current PR.

@bruvzg bruvzg force-pushed the native_menu branch 2 times, most recently from 6678dd3 to 7778d37 Compare January 31, 2024 16:48
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Docs are decent. They could be worded better but that's outside the scope of the PR, since the descriptions are all copy-pasted. What matters more is that the DisplayServer's methods are deprecated properly.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks really good to me!

@akien-mga akien-mga merged commit 13954fc into godotengine:master Mar 6, 2024
16 checks passed
@akien-mga
Copy link
Member

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.

6 participants