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: remove Linux menubar workaround #11259

Closed
wants to merge 3 commits into from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 8, 2023

It appears this is causing issues 1 on recent Ubuntu 22.04 1 and also 20.04 2 with the defaut window manager, as well as 2 with (unofficial) window manager extensions that enable the so-called 'global menu' (menubar is integrated in the window title bar) after toggling fullscreen (e.g. when switching skins while in fullscreen mode):

  • 1 + 2 menubar hotkeys not working anymore
  • 2 menubar is not restored in the title bar but in the windwo itself

I also saw leaked controls lately when toggling fullscreen repeatedly, the VisibiltyConnections created in WMainMenubar.

Edit now this also includes a fix for #11281
Edit2 now also trying to fix #11294

TODO
verify no regressions on supported distros with and without global menu, e.g.

  • Ubuntu 20.04 (Gnome desktop)
  • Ubuntu 20.04 with vala-panel-appmenu
  • Ubuntu 22.04 with Gnome desktop (default) 3
  • Ubuntu 22.04 with Unity desktop 4
  • Linux Mint
  • Kubuntu 22.04 with Plasma's Application Menu bar 5

Fixes #11281

Footnotes

  1. with 2.3.x https://mixxx.discourse.group/t/bug-with-full-screen-and-big-library-ubuntu/26596/6, no bug report yet

  2. with 2.5-prealpha, experienced myself, with Ubuntu Studio 20.04 (xfce desktop + vala-panel-appmenu)

  3. hotkey issues fixed, also confirmed by the original bug reporter https://mixxx.discourse.group/t/bug-with-full-screen-and-big-library-ubuntu/26596/13

  4. no regression, like: View menu hotkeys cease to work after toggling fullscreen anyway

  5. the App bar is not visible while apps are fullscreen 🤷 or at least I didn't figure howto.. anyways, all hotkeys work after toggling fullscreen repeatedly

@ronso0 ronso0 added this to the 2.3.4 milestone Feb 8, 2023
@ronso0 ronso0 marked this pull request as ready for review February 9, 2023 23:46
@ronso0
Copy link
Member Author

ronso0 commented Feb 10, 2023

I'll grab a thumbdrive this weekend (hopefully) and will test in Live sessions of those Ubuntus and Mint.
Are there any special distros that should be tested?

@daschuer
Copy link
Member

Unfortunately the issue #6072 comes back with this PR testing on Ubuntu Focal 20.04 and Unity Desktop. The Gnome Based "Ubuntu" desktop is not affected. It has no integrated menu bar.
Maybe this is only a Unity issue?

Maybe we can check the environment

echo $XDG_SESSION_DESKTOP
unity

@daschuer
Copy link
Member

I don't think we need to check all old and exotic configurations. It would be easy to bring it back conditionally if we have users complaining. Relevant are the current desktops using the global menu bar.

@ronso0 ronso0 force-pushed the fullscreen-remove-linux-workaround branch from a94fe73 to 7e58af7 Compare February 11, 2023 17:22
@ronso0
Copy link
Member Author

ronso0 commented Feb 11, 2023

Maybe we can check the environment

echo $XDG_SESSION_DESKTOP

Good idea! I now do this with qgetenv("XDG_SESSION_DESKTOP")

The hotkey issue issue is fixed for vala-panel-appmenu which seems to be the top recommendation to get a gloabl app menu on other distros (and if you don't want the entire unity de).

@ronso0 ronso0 force-pushed the fullscreen-remove-linux-workaround branch 2 times, most recently from 70d4378 to 5eb9e58 Compare February 11, 2023 18:12
@ronso0 ronso0 marked this pull request as draft February 11, 2023 18:13
@ronso0 ronso0 mentioned this pull request Feb 15, 2023
@ronso0 ronso0 marked this pull request as ready for review February 15, 2023 09:45
@ronso0
Copy link
Member Author

ronso0 commented Feb 16, 2023

I tested affected Ubuntu 22.04 with Gnome and Unity, and for the former the hotkey issues are fixed (Spacebar for max. library). Also confirmed by the bug reporter https://mixxx.discourse.group/t/bug-with-full-screen-and-big-library-ubuntu/26596/13
In Ubuntu with Unity desktop (global menu enabled by default), the Ctrl hotkeys from the view (the VisibilityControlConnections) cease to work after toggling fullscreen, i.e.after the menubar is recreated and reconnected.

It appears this is causing issues 1) on recent Ubuntu 22.04 and also 20.04 with the defaut window
manager, as well as 2) with (unofficial) window manager extensions that enable the so-called 'global menu'
(menubar is integrated in the desktop's top bar) after toggling fullscreen, e.g. when switching skins
while in fullscreen mode:
* 1) 2) menubar hotkeys not working anymore
* 2) menubar is not restored in the title bar but in the windwo itself
@ronso0 ronso0 force-pushed the fullscreen-remove-linux-workaround branch from 5eb9e58 to b65693c Compare February 17, 2023 21:31
@github-actions github-actions bot added the ui label Feb 17, 2023
@ronso0
Copy link
Member Author

ronso0 commented Feb 17, 2023

Now this also includes a fix for #11281
The VisibilityControlConnections were not connected to the respective controls, or actually the proxies didn't exist because they are only created in slotReconnectControl which is triggered only by WMainMenuBar::internalOnNewSkinLoaded.
Now this is also triggered by MixxxMainWindow::fullScreenChanged. Like after loading a skin, this is emitted on all target system but it doesn't hurt.

@daschuer please give this a shot on 20.04 with Unity

@ronso0 ronso0 linked an issue Feb 17, 2023 that may be closed by this pull request
@daschuer
Copy link
Member

@daschuer please give this a shot on 20.04 with Unity

It works well as before.

@daschuer
Copy link
Member

It does not longer work with Linux Mint Mate "Global Application Menu" on Tara (Ubuntu Bionic 18.4)
XDG_SESSION_DESKTOP is "mate" in this case.

@ronso0
Copy link
Member Author

ronso0 commented Feb 20, 2023

What does not work? Hotkeys or is the menu gone?

I looked into it again yesterday and I maybe we need to check for [affected DEs] and isNativeMenuBar.

@daschuer
Copy link
Member

The manu bar is gone with MATE after going into full screen mode using F11.

@daschuer
Copy link
Member

I suggest to exclude Mate also like Unity.

@ronso0
Copy link
Member Author

ronso0 commented Feb 20, 2023

I suggest to exclude Mate also like Unity.

Only if we can guarantee that it works with recent Mint with newer Gnome, not just v9 Tara with discontinued "Global Application Menu" extension (which can't be used in newer Mints IIUC).

@daschuer
Copy link
Member

Do we still want to put that in 2.3.4? If Yes, I suggest to revert the approach.
Currently we are about to revert an established hack for all distros and special case the failing variants we are discovering during testing. The hack was in place since > 10 Years and now it turns out that it is broken with some modern global menu bars, so let's special case them and use the old code for all unknown cases. This way we only change the code for setups we can test.
This is IMHO better than postpone the fix because of the limited test resources.

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

The hack was in place since > 10 Years and now it turns out that it is broken with some modern global menu bars

IIUC the hack was only added because there were issues with Ubuntu & Unity back then. And the hack didn't cause any issues on other distros & desktops. If there were no one noticed them, and amongst those who did no one reported it. Me for example, I thought seeing it failing with the vala appmenu was not Mixxx' fault but that of the vala appmenu plugin (working fine with all other apps btw). But now the hack also fails on a default Ubuntu 22.04 LTS without global menu (unmodified Gnome).

What we know is, the hack is appearantly still required for some desktops that feature a global menu: the Ubuntu Unity desktop and Linux Mint with the Mate desktop (+ global menu enabled via a custom Gnome extension).
Without the hack it works with KDE Plasma's global menu and with the vala appmenu plugin (on xfce4). And that's about it, I didn't find any other relevant (i.e. usable and still maintained) global appmenu plugins for Linux.

So I think we should go with the blacklist = enable the hack only on demand. I'll test the Mate desktop and if isNativeMenuBar is helpful.

And yes, I think this should be in 2.3.4, but we should of course test more and if that means releasing 2.3.4 unmodified, I'm fine with that. We release the fix in 2.3.5 later on.

@daschuer
Copy link
Member

This is the only PR that holds back the 2.3.4 release. We try to raise the test coverage but we will unlikely ever reach 100%. So we will finally have three groups:

  • Known fixed distrios
  • Known regression distrios
  • Unknown

Options:
1: Merge this now = high risk of regression in the unknown group
2: Postpone the PR = bug remains unfixed for 2.3.4 for all in the fixed group and and some in the unknown group.
3: Explicit enable known fixed = bug remains unfixed in some of the unknown group

I vote for 3 and I don't want 1.
Can we decide today that we can either work on 3. or release 2.3.4 immediately?

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

As I stated multiple times: if 2.3.4 should be released ASAP then do so.

edit I agree with these categories, and I agree 3 would be the best, but..
I don't see how we can reliably whitelist (opt-out, disable the hack) distros/desktop with XDG_SESSION_DESKTOP only. For example, if we'd whitelist Ubuntu with XDG_SESSION_DESKTOP == ubuntu (Gnome desktop) that would include desktops with the Gnome global menu extension, which would be broken then. And that applies to all distros with the Gnome desktop.
The only desktop I'm sure we can whitelist is xfce because it has no global menu or uses vala appmenu, with which (like the global menu on KDE Plasma) View hotkeys always work and which does not show the menubar in fullscreen mode also in other apps, so that's something users expect and the Mixxx hack would actually break that desktop design pattern.

So my preferred way is the blacklist (opt-in, eable the hack) by testing for desktops known to require the hack with XDG_SESSION_DESKTOP == unity and XDG_SESSION_DESKTOP == ubuntu | mate && m_pMenubar->isNativeMenuBar. Additionally, we could add a command line argument --reconnect-menu-bar (default off) to enable the hack on demand.
This will not be ready today, so let's release 2.3.4 now

FYI I just tested vanilla Mint 19 Tara (based on Ubuntu 18.04) with Mate desktop (based on Gnome2), and View hotkeys are also broken there. I can install 2.3 builds but not 2.3 CI builds (depend on non-existing libgcc-s1), and I didn't figure how to install the Gnome extension you mentioned.

@uklotzde
Copy link
Contributor

uklotzde commented Feb 21, 2023

@ronso0 I agree, opt-in to the hack is the way to go.

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

@uklotzde Do you experience any issues after toggling fullscreen on Fedora? Hotkeys in the View menu should always work (Ctrl+1, Spacebar)

@daschuer
Copy link
Member

For my understanding the second commit is independent. It fixes the view hotkey in all of my tests. Mate/Ubuntu/Unity.
Can you do a PR for that against 2.3.
Than merge and release?
What do you think?

The command line option is probably the best solution to build up the list of conditions for the hack. I consider the window menu after coming form full screen mode as a minor problem compared to having no menu bar at all. So for now, am convinced not to risk the this for all unknown/untested distros. Once the test coverage is big enough this question is no longer relevant.
I think it is reasonable to look at this again during the 2.4 release.

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

Yes, I can confirm the second commit fixes the hotkey also on the distros I tested.
And with that we don't need to remove the workaround.
I attached another commit to fix #11294

I'll open another PR with just those commits ASAP and that should then definitely go into 2.3.4

As much as I like the cmdline method I wonder if we should bother with that at all.

Btw, the longer I deal with fullscreen I disagree the original issue (no menu bar in fullscreen) is a bug actually.
At least in Unity, if you go fullscreen with an app that has a menu the menubar is gone in fullscreen (it's fullscreen after all).
(except apps like LibreOffice that do fake fullscreen by just hiding the toolbars and other widgets).

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

@daschuer Please test for #11294 with cf508af

@ronso0
Copy link
Member Author

ronso0 commented Feb 21, 2023

continued in #11295

@daschuer
Copy link
Member

daschuer commented Feb 22, 2023

Thank you for the fast fix.

Btw, the longer I deal with fullscreen I disagree the original issue (no menu bar in fullscreen) is a bug actually.

I think that pretty much depends on the application specific use case. In a presentation mode like a video player or web browser in kiosk mode the menu bar is anoying. For productive apps where you use full-screen just to make the best of the display space the menu bar is required.

Mixxx is somwhere in between which is also the source if the discussion to get rid of the menu bar in windowed mode. Still like the discussed option to hide/show the menu bar with the Alt-key. Maybe this is also a "fix" for the disappeared menu bar in this case (2.4-alpha)

@daschuer daschuer removed this from the 2.3.4 milestone Feb 23, 2023
@ronso0
Copy link
Member Author

ronso0 commented Feb 23, 2023

Closing this for now even though #11295 is only a working around a workaround.

@ronso0 ronso0 closed this Feb 23, 2023
@ronso0 ronso0 deleted the fullscreen-remove-linux-workaround branch March 9, 2023 00:08
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.

View menu hotkeys don't work after toggling fullscreen on Ubuntu 22.04
3 participants