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

fullscreen: rebuild & reconnect menu only on desktops with global menu #11350

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Mar 9, 2023

follow-up for #11328:
keep the menubar on all Linux desktops that don't use global menubars to avoid pointless operations.

@ronso0 ronso0 force-pushed the menubar-rebuild-only-if-required branch from 4e0961c to 83595c7 Compare March 9, 2023 22:55
@daschuer
Copy link
Member

For my understanding Qt::AA_DontUseNativeMenuBar is always false.

This change has been introduced in Qt 5.7.1 (Ubuntu Bionic has 5.9):
qt/qtbase@8cd28ea

According to the comment we can check whether the menu bar is already non-native before chaining to full screen using MenuBar::isNativeMenuBar().

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Is there a Linux distro that has no native menu bar (DBus)?
If not we can just close this?

@ronso0
Copy link
Member Author

ronso0 commented Mar 12, 2023

Oh, you got me there, I didn't investigat thoroughly.

What I'm after is actually the unchangeable 'supports global menu' property, like qt5ct reports it when Mixxx is started.
I changed the approach and adopted the way qt5ct detects it:
https://github.com/desktop-app/qt5ct/blob/9f60cd2352a4dcc55c8ca267f29bd8fff5c6a659/src/qt5ct-qtplugin/qt5ctplatformtheme.cpp#L102-L105

QDBusConnection conn = QDBusConnection::sessionBus();
m_dbusGlobalMenuAvailable = conn.interface()->isServiceRegistered("com.canonical.AppMenu.Registrar");
qCDebug(lqt5ct) << "D-Bus global menu:" << (m_dbusGlobalMenuAvailable ? "yes" : "no");

Is this okay in your opinion?

@ronso0 ronso0 force-pushed the menubar-rebuild-only-if-required branch from 83595c7 to 84264c6 Compare March 12, 2023 21:52
src/mixxx.cpp Outdated
#ifdef __LINUX__
inline bool supportsGlobalMenu() {
QDBusConnection conn = QDBusConnection::sessionBus();
return conn.interface()->isServiceRegistered("com.canonical.AppMenu.Registrar");
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

For completeness you may also check the QT_NO_DBUS macro.

@daschuer
Copy link
Member

Is this okay in your opinion?

Yes.

@ronso0
Copy link
Member Author

ronso0 commented Mar 13, 2023

Yep, implemented as you suggested. Thanks for you help!

@ronso0 ronso0 force-pushed the menubar-rebuild-only-if-required branch from 84264c6 to dcd0f69 Compare March 13, 2023 00:09
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@daschuer daschuer merged commit 41ba3d6 into mixxxdj:2.3 Mar 13, 2023
@ronso0 ronso0 deleted the menubar-rebuild-only-if-required branch March 13, 2023 11:25
@daschuer daschuer added this to the 2.3.5 milestone Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants