-
Notifications
You must be signed in to change notification settings - Fork 33
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
Use native menubar on macOS, not elsewhere (i.e. Linux) #695
Use native menubar on macOS, not elsewhere (i.e. Linux) #695
Conversation
WalkthroughThe recent changes enhance the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/python/main/ayab/menu.py (2 hunks)
- src/main/python/main/ayab/menu_gui.ui (4 hunks)
Additional comments not posted (5)
src/main/python/main/ayab/menu.py (2)
22-22
: LGTM! Import statement forQSysInfo
.The import statement for
QSysInfo
fromPySide6.QtCore
is appropriate for determining the operating system type.
43-45
: LGTM! Conditional logic for platform-specific menu bar behavior.The conditional logic to set the menu bar to non-native if the OS is not macOS is correctly implemented.
src/main/python/main/ayab/menu_gui.ui (3)
68-70
: LGTM! Removal ofnativeMenuBar
property and addition ofmenuRole
foraction_quit
.The removal of the
nativeMenuBar
property and the addition of themenuRole
property foraction_quit
are appropriate changes to improve platform-specific behavior and semantic meaning.
102-104
: LGTM! Addition ofmenuRole
foraction_about
.The addition of the
menuRole
property foraction_about
is an appropriate change to improve the semantic meaning of the action.
198-200
: LGTM! Addition ofmenuRole
foraction_set_preferences
.The addition of the
menuRole
property foraction_set_preferences
is an appropriate change to improve the semantic meaning of the action.
Adding "menuRole" properties to standard menu actions (About, Preferences, Quit) lets Qt move them to their standard place in the macOS menu bar (without having to rely on their text content, which breaks when localizing). This also automatically sets the "Preferences" shortcut to the standard "Cmd+," instead of "Cmd+P". This should have no impact on other platforms. See https://doc.qt.io/qt-6/qmenubar.html#qmenubar-as-a-global-menu-bar
03915b7
to
bda83d4
Compare
Quality Gate passedIssues Measures |
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/main/python/main/ayab/menu.py (2 hunks)
- src/main/python/main/ayab/menu_gui.ui (4 hunks)
Files skipped from review due to trivial changes (1)
- src/main/python/main/ayab/menu_gui.ui
Files skipped from review as they are similar to previous changes (1)
- src/main/python/main/ayab/menu.py
Fixes #694 .
This PR makes AYAB only disable the native menu bar when an OS other than macOS is detected. Or rather, it un-disables it if macOS is detected. Oh well, you know what I mean 😅.
In addition, Qt's menu roles are leveraged to annotate the relevant actions (
About
,Preferences
,Quit
) so that they are automatically migrated to their standardized locations in the native menu. Of course, the migrated menu items function as expected.Here is the result:
ayab-mac-menu-1.0-fixed.mov
Summary by CodeRabbit
New Features
Bug Fixes