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

[NativeMenu] Implement native popup menu support on Windows. #89273

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Mar 8, 2024

Partial NativeMenu (since there's no "global menu") for Windows, useful for status indicator (tray icon) context menus, but can be used with any popup (PopupMenu).

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 8, 2024
doc/classes/NativeMenu.xml Show resolved Hide resolved
doc/classes/NativeMenu.xml Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Mar 8, 2024

The menu goes under task bar, which makes it difficult to use with tray icons:
image
Also it seems to be tied to the main window. When you minimize it, the menu can't appear.
Also also clicking outside the menu does not close it.

EDIT:
Also, not sure how controllable is that, but the menu does not follow the system theme. It should be dark in my case.

@bruvzg
Copy link
Member Author

bruvzg commented Mar 8, 2024

The menu goes under task bar
Also it seems to be tied to the main window. When you minimize it, the menu can't appear.
Also also clicking outside the menu does not close it.

Should be fixed, as well as support dark mode.

Screenshot 2024-03-08 2341336

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I didn't check the code, but did some brief testing and it works correctly.

The documentation could use some code examples.

@bruvzg
Copy link
Member Author

bruvzg commented Mar 13, 2024

Added a usage example to the docs, also added prefer_native_menu property to the PopupMenu to use it as a wrapper for native popups (more convenient that doing it from the code).

[b]Note:[/b] This is low-level API, consider using [MenuBar] with [member MenuBar.prefer_global_menu] set to [code]true[/code], and [PopupMenu] with [member PopupMenu.prefer_native_menu] set to [code]true[/code].
To create a menu, use [method create_menu], add menu items using [code]add_*_item[/code] methods. To remove a menu, use [method free_menu].
[codeblock]
var menu
Copy link
Contributor

@Mickeon Mickeon Mar 14, 2024

Choose a reason for hiding this comment

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

I'm fine with this but how does anyone else feel about it, over the more explicit var menu = null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems redundant to me (at least for non-typed code, and all doc examples seems to be non-typed).

@akien-mga akien-mga merged commit 4ca6cd0 into godotengine:master Mar 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Mar 14, 2024

So I didn't test the latest changes to PopupMenu, because I assumed they'd be alright, but there is error spam when opening editor:
image

@aaronfranke
Copy link
Member

This caused a regression in the macOS global menu: #89496

@jsjtxietian
Copy link
Contributor

there is error spam when opening editor:

I got this too.

image

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