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

crash when pressing menu hotkey (Alt+F) in fullscreen mode on Linux with a global menu #11320

Closed
ronso0 opened this issue Mar 2, 2023 · 16 comments

Comments

@ronso0
Copy link
Member

ronso0 commented Mar 2, 2023

Bug Description

Affects Linux desktops with a global menu bar (Unity desktop, or xfce4 desktop + vala appmenu plugin)

  • enter fullscreen mode
  • press a menu hotkey (with english locale: Alt+F, Alt+V, Alt+L etc.)

Expected:

  • respective menu gets keyboardfocus and unrolls

Actual result:

  • SIGSEV

In initial windowed mode and after returning to that from fullscreen, hotkeys work as expeced.
When clicking a menu keyboard navigation works as expected.

Conclusion:
after m_pMenuBar->setNativeMenuBar(false) when going fullscreen, Qt can't handle the hotkeys anymore.

backtrace.txt
Zulip 2.3.4 testing

Version

2.3.3 and newer

OS

Ubuntu 20.04.5 + 22.04 (both with Unity desktop), Ubuntu 20.04.5 with Xfce desktop and vala appmenu plugin

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

Fix

TL;DR

don't hack arond xxx with m_pMenuBar->setNativeMenuBar(false)

Long story

Because of #11294 Mixxx does recreate and reconnect the main menubar each time fullscreen is toggled on Linux, in case the desktop features a global menu, it's moved to the window so it's also available in fullscreen mode.
This works 'fine' (finally, after #11304), however it's really just required for global menus.

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

Proposal #1 Quick fix

Simply don't touch the menubar at all when toggling fullscreen. Brings back #11294

= hotkeys always work (if they work in other 'real fullscreen' apps)

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

Proposal #2

Disable global menu (except on macOS, of course) with QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar);

= menu always in window
= hotkeys always work

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

Proposal #3

#1 + popup when going fullscreen that displays the fullscreen hotkey
image
(already implemented in #11304)
Unfortunately, that would not work if the (Linux) desktop is not affected by #11321.

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

Proposal #4

#3 + rework fullscreen toggle + toggle menu with Alt key (Linux and Windows only)

This is the current state (WIP) of #11313 which I built on top of #11304

= menu always in window
= hotkeys always work
= clean fullscreen GUI on all supported OS
= like #2 and #3 this breaks the global menu design on some Linux desktops

This is my favourite because it basically solves all fullscreen related issues on Linux.

@ronso0
Copy link
Member Author

ronso0 commented Mar 2, 2023

As you can see I'm working on it anyway for 2.4, so it's up to dev team to decide which fix to chose in the short and long term. Rebasing both #11304 and #1131 onto 2.3 is possible but I'll do it only if there's a consesus to merge it.

@daschuer
Copy link
Member

daschuer commented Mar 3, 2023

This commit gives some hints, not sure if it is useful:
qt/qtbase@5d6878f

@daschuer
Copy link
Member

daschuer commented Mar 3, 2023

I cannot confirm that with Ubuntu Focal 20.4 Ubuntu And Unity desktop.
Can you confirm that this is unrelated to #11295?
For my understanding yes. So we can proceed with the 2.3.4 release.

  • Proposal 1# will come with a strong usability regression for some users
  • Proposal 2# is OK compared to a crasher (not yet confirmed though)
  • Proposal 3# feels cumbersome
  • Proposal 4# I agree that seems to be the best

You have not mention:

  • Proposal 5# Debug Qt and fix the crash. This is the most sustainable solution but only reasonable if we have a way to reproduce the issue and confirm that it is gone. I can't today.

@ronso0
Copy link
Member Author

ronso0 commented Mar 3, 2023

Can you confirm that this is unrelated to #11295?

Yes, as I wrote in the report, I tested 2.3.3 (HEAD) built on June 21 2022. Note that it works after I quit fullscreen (new global menu). 2.4 in the 22.04 VM also crashes.

What shall I say, this happened in both VMs with freshly installed Ubuntu 20.04 / 22.04, packages updated and "just" ubuntu-unity-desktop installed. I can't tell if there are additional packages required, and a regular user doesn't know either and would blame Mixxx :
And now it also crashes on my production machine. I don't see a reason for that, except some recent Ubuntu or (unlikely) Qt update, because I did test exactly that with my menubar PR in both 20.04 and 22.04 (VM) earlier, and both didn't crash.
Do you have any idea?

Yes, #1 would only be an immediate fix to avoid the crash
#2 is like #1, just a bit better
#3 the code is ready, works on Linux #11304
#4 #3 + fullscreen rework: the code is ready, works on Linux #11313

@ronso0
Copy link
Member Author

ronso0 commented Mar 3, 2023

waaah, this is driving me crazy:
Now it works reliably with #11313 (based on 2.4) (also with hidden menubar which is shown directly after the keypress, bound to QMenu::aboutToShow)
With 2.3 it still crashes locally (with xfce4), as well in VM with Unity

so there is definitely something wrong...

@daschuer
Copy link
Member

daschuer commented Mar 3, 2023

I dived into the qt source and it looks like as if the platform menu bar is first created and than deleted after setNativeMenuBar(false). Maybe there is a dangling pointer to the platform menu bar somewhere causing the crash.

We can work around this by not using the probably evil setNativeMenuBar(false). This can be done by adjusting the Qt::AA_DontUseNativeMenuBar before createMenuBar()

An alternative would be to set
Qt::AA_DontUseNativeMenuBar
and use setNativeMenuBar(true) instead.

Unfortunately I can't verify this here, because it dies not crash.

@ronso0
Copy link
Member Author

ronso0 commented Mar 3, 2023

I tried setting QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar) at runtime, just before creating the menubar.
I does not yield consistent results, seems rather random. When toggling fullscreen repeatedly the menubar is often shown in the window, but sometimes not.
It is not mentioned anywhere whether this attribute may be changed at runtime or not.

Oh, and I forgot I'm already setting this in main.cpp in my fullscreen-rework which doesn't crash.
So basically, all Mixxx versions that setNativeMenuBar(false) do crash here...

@daschuer
Copy link
Member

daschuer commented Mar 4, 2023

Cool, that confirms my assumption that setNativeMenuBar(false) is evil.

So how about trying my second proposal to use setNativeMenuBar(true) with
A single QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar) call.

@ronso0
Copy link
Member Author

ronso0 commented Mar 4, 2023

must admit I don't have a clue what you're up to.
when and where exactly would these calls happen?

edit Oh, you want to set and call that always (on Linux) when going fullscreen? I'll try that later on.

@ronso0
Copy link
Member Author

ronso0 commented Mar 4, 2023

So how about trying my second proposal to use setNativeMenuBar(true) with
A single QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar) call.

With QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar) in main.cpp and setNativeMenuBar(true) when leaving fullscreen, there's no global menu anymore. It's in the window in fullscreen mode, but that's about it.

If I toggle Qt::AA_DontUseNativeMenuBar each time fullscreen is toggled, then create a new menubar and setNativeMenuBar(true) when leaving fullscreen, it'll work in the window (fullscreen) but the one in the panel (windowed) will remain and stop working (actions do nothing).

I will stop experimenting with that, it's annoying. #11313 works beautifully for me, + there are no controls leaked anymore. Users who have a global menu will get used to it in Mixxx, like most got used to no global menu in firefox/thunderbird, and like these apps, Mixxx will likely be used maximized or in fullscreen so an in-window on-demand menubar will be acceptable IMHO.

@ronso0
Copy link
Member Author

ronso0 commented Mar 23, 2023

fixed / worked around with #11328

@ronso0 ronso0 closed this as completed Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants