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

Better implementation of native menu bar on Linux #18593

Merged
merged 2 commits into from
Nov 17, 2023

Conversation

adazem009
Copy link
Contributor

@adazem009 adazem009 commented Jul 14, 2023

Resolves: #18572
Resolves: #16094

This PR includes these fixes:

  • I disabled loading menu subitems when a menu is created before because it caused a crash on launch. Loading subitems after adding the menu to the menu bar fixes the crash.
  • Re-creating menu items when a menu is about to show causes a crash on quit and flickering glitches when opening menus. I made it create the menu items only when the app starts and when the subitems property changes (see the connections in the load() function). The titles, checked, enabled, etc. properties are updated when a menu is about to show.

Note: I didn't test this on macOS. The original menu bar implementation should be used there.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@Eism
Copy link
Contributor

Eism commented Jul 17, 2023

@adazem009
It would be great to figure out why this started crashing. After a quick code review of your previous PR #15826 and the current master branch, I don't see any differences.
Can you reproduce the bug on your previous PR? (you'll need to build that branch)

@adazem009
Copy link
Contributor Author

@adazem009 It would be great to figure out why this started crashing. After a quick code review of your previous PR #15826 and the current master branch, I don't see any differences. Can you reproduce the bug on your previous PR? (you'll need to build that branch)

Yes, that was the first thing I had done before I came up with this fix on the master branch.

@adazem009 adazem009 force-pushed the fix_linux_crash_on_quit branch from 4d29ae4 to 622bd9e Compare July 17, 2023 21:52
@adazem009
Copy link
Contributor Author

adazem009 commented Jul 17, 2023

@Eism I really think this is a Qt bug. I should test it with a newer Qt 5.15 release instead of doing weird workarounds.

This crash was happening at the time of merging #15826, but I just didn't notice it.

By the way, I think the menu bar needs a different implementation. What's the point of re-creating menu items every time when they're shown to the user (the aboutToShow signal)? Do the menus ever change (except the enabled property)? Maybe we should ask @RomanPudashkin about this. See this commit: ec8c194

@adazem009 adazem009 requested a review from Eism July 17, 2023 21:59
@cbjeukendrup
Copy link
Contributor

Regarding using a newer Qt version, this might be helpful, from another PR that was also about a Qt bug: #17997 (comment)

@adazem009 adazem009 force-pushed the fix_linux_crash_on_quit branch from 622bd9e to 388e696 Compare July 18, 2023 14:05
@adazem009 adazem009 changed the title Fix #18572: Fix crash on quit (when destroying Linux platform menu bar) Better implementation of native menu bar Jul 18, 2023
@adazem009
Copy link
Contributor Author

So, I've come up with a better implementation of the native menu bar and it fixes 2 issues.
@cbjeukendrup Do you think it's better to avoid re-creating menu items every time a menu is shown to the user?
By the way, I'm going to test this with Qt 5.15.10 now just to see if it fixes some of these issues in the old implementation...

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Jul 18, 2023

I don't know if this it actually relevant, but as long as you're looking at how menus are created on Linux, it sure would be nice if the new approach somehow managed to work on Wayland as per #15285 (see in particular the comment about global menus in #15285 (comment))

@adazem009
Copy link
Contributor Author

I don't know if this it actually relevant, but as long as you're looking at how menus are created on Linux, it sure would be nice if the new approach somehow managed to work on Wayland as per #15285 (see in particular the comment about global menus in #15285 (comment))

That issue is related to the non-native menu bar. I tried to launch MuseScore under Wayland but it doesn't start (see #18598).
However, it works when I build it from source and I can reproduce the issue you've linked. I've also noticed that the native menu bar doesn't even show, so there isn't any menu bar when global menu bar is enabled in the desktop environment. But this should go into a new issue.

@cbjeukendrup
Copy link
Contributor

@cbjeukendrup Do you think it's better to avoid re-creating menu items every time a menu is shown to the user?

I'll defer this question to @RomanPudashkin and @Eism, since I'm not sure why this was done in the first place; but if it works without it, I can imagine that would be preferable because more efficient.

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Jul 19, 2023

I tried out the latest version on macOS and found some strange things: "About MuseScore" now opens the "About MusicXML" dialog, and the "Preferences" item calls the "revert-factory" action. This seems related to the menuItem.role property, since all the affected menu items do something with this property.
Since this property influences in which menu a menu item appears, it might be necessary to set this very early, or for example to make sure that all MenuItems have this property set before the menus containing them are attached to the menu bar.

@cbjeukendrup
Copy link
Contributor

I found another thing: directly after launching MuseScore, these affected items initially don't appear on macOS.
On macOS, they are supposed to appear in the "MuseScore" menu, which is added by macOS as the leftmost menu. We simply add them to the "Edit" and "Help" menus, where they are supposed to be on other platforms, but because of the role property, they will be moved automatically from those menus to the MuseScore menu.
So, initially, after launching the app, these items don't appear in the MuseScore menu. But once you've opened the "Edit" menu, the "Preferences" item is added to the MuseScore menu (but does not do the thing it's supposed to do). And once you've opened the "Help" menu, the "About" items appear in the MuseScore menu too. Maybe that's useful info.

If it turns out to be difficult to solve, we can decide to make two independent native menu bars, one for Mac and one for Linux.

@adazem009
Copy link
Contributor Author

@cbjeukendrup This doesn't happen on Linux, but I think it's caused by how the menus are created. When you launch MuseScore, the load() function is called on each menu. This creates empty menu items and submenus (and items in the submenus). When a menu is about to show (onAboutToShow), the update() function is called on the menu which sets the properties of each menu item.
Maybe calling update() on each menu when MuseScore starts would fix this, but I'll need some help with this because I don't have a Mac and it'd take ages to launch MuseScore on my Hackintosh VM :)

@cbjeukendrup
Copy link
Contributor

I see. Then the best option is probably that I take a look at these issues myself :)

@adazem009 adazem009 force-pushed the fix_linux_crash_on_quit branch 2 times, most recently from baa4b85 to 5971188 Compare July 24, 2023 12:00
@adazem009 adazem009 changed the title Better implementation of native menu bar Better implementation of native menu bar on Linux Jul 24, 2023
@adazem009
Copy link
Contributor Author

@cbjeukendrup I've made the independent menu bars for Linux and macOS.
macOS should now use the old implementation if I did it correctly.

@DmitryArefiev
Copy link
Contributor

@adazem009 Hi! Can you rebase please?

I disabled loading subitems on Linux before because it caused MuseScore to crash on launch.
macOS will still use the old implementation
The new implementation creates menu items when the app starts and reuses them to update their properties later.
@adazem009 adazem009 force-pushed the fix_linux_crash_on_quit branch from 5971188 to bc54999 Compare November 13, 2023 17:13
@adazem009
Copy link
Contributor Author

@adazem009 Hi! Can you rebase please?

Done

@DmitryArefiev
Copy link
Contributor

Thanks!

I installed KDE plasma on my Ubuntu finally and able to reproduce crash from #18572 in master

Tested #18572 on Ubuntu 22.04 with KDE Plasma 5.24.7 - FIXED

Tested #16094 on Ubuntu 22.04 with KDE Plasma 5.24.7 - probably FIXED see #16094 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants