Fix stuck notes when menu is open

[necrashter]

Fixes #4892

I created a subclass of QMenu and overloaded the keyReleaseEvent to send the event to the parent widget. Passing the event directly didn't work. So I cloned the event in the stack and sent it to parent. This seems to be the usual practice.

This does NOT solve the related bug #3654 because it's about keyPressEvent. Also QMenu blocks the CTRL key. Try opening the edit menu and then pressing CTRL+S. QMenu only sends the S key.

[PhysSong]

Could you add license information at the beginning of the new files?

[necrashter]

Could you add license information at the beginning of the new files?

I added the license information, but I didn't add any name. Should I add my legal name?

[JohannesLorenz]

I just tested exactly what @SecondFlight mentioned in the issue description. The problem still seems to exist. Am I testing right?

[necrashter]

@JohannesLorenz I tried this PR again. It seems to fix the problem for me. I am running on Ubuntu 19.04, perhaps the problem is platform-dependent?

Could you please provide more information? I open the edit menu on top and play some notes, and the notes don't get stuck unlike before.

[JohannesLorenz]

I just rebuilt at 8a4636a to make absolutely sure. It still fails. Like you, I open 3osc, play some notes pressing keyboard keys (works as usual), then click edit, and while the edit menu is open, press the keyboard keys again. Then the 3osc notes get green and play (correct behavior), but releasing the key, they remain green and remain playing (incorrect).

My system is Manjaro, updated regularly.

[JohannesLorenz]

As necrashter assumed, the issue is Qt4 vs Qt5: The fix works with -DWANT_QT5=ON, but not without. I just confirmed it on the same machine.

[zonkmachine]

As necrashter assumed, the issue is Qt4 vs Qt5: The fix works with -DWANT_QT5=ON

So, the easiest fix then is to retarget this to master, which is Qt5 only.

[JohannesLorenz]

I wonder if this is a "critical" issue - It's not "normal" to play notes while opening a menu, and even when you do, the stuck notes can be easily resetted by leaving the menu and playing the stuck notes again.

I'm OK with a Qt5-only solution, or no solution at all (though a Qt4+5 solution would be nice).

Opinions?

[ghost]

Can you try this against Qt 5.6 (or lower)? In #5328 @he29-net and I tested identical solution for QColorDialog instead of QMenu and it didn't work on Qt 5.6 but did for Qt 5.11 and higher (didn't test any version between Qt 5.6 and 5.11).

[DomClark]

Is there a more general solution for this? As @cyberdevilnl mentioned, this also occurs for QColorDialog in #5328, and it happens for me with all the current dialogs as well (open, save, settings, about, soundfont patch, etc.). It would be nice if we could avoid needing to add this code to everything that can steal key events. Also, the bug occurs with all menus in LMMS, including context, combo-box, track button, etc., so those will also need to use the new Menu class.

[JohannesLorenz]

1. Is it a stable-1.2 bug? Just close the menu and you can continue using LMMS as usual.
2. Is it even a bug? It's not wrong behavior to block the key events, it's just one possible behaviour.
3. Is it even worth a feature request? Why shall this be changed?

[PhysSong]

Is it even a bug?

I think yes because it's not normal to make keys stuck in that situation.

Is it a stable-1.2 bug?

It's a bad UX, but I don't think this is critical.

didn't test any version between Qt 5.6 and 5.11

I can try with 5.9.

[PhysSong]

It works well with Qt 5.9.

[JohannesLorenz]

OK, let's treat it as a bug which shall be fixed on master.

@necrashter I'll change the PR base to master if you don't disagree within 7 days..

[necrashter]

@JohannesLorenz OK, please go ahead.

[tresf]

Also, the bug occurs with all menus in LMMS, including context, combo-box, track button, etc., so those will also need to use the new Menu class.

I find this question to be very important yet not answered. Is the intention to fix it holistically?

[JohannesLorenz]

@necrashter What are you going to do with this PR?

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.31%2Bg76c45ee-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10895-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.31%2Bg76c45ee-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10895?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10897-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10897?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10898-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10898?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/r6v2g4wuhddxnqa1/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36565278"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/f64qoy24nrhuf3f5/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36565278"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10896-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.31%2Bg76c45ee4e-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10896?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "8350adb532f5045ee6f91b7fbe0cdf5db3dc69c0"}

[necrashter]

I checked the Qt documentation for a more general solution, but couldn't find any. In fact, the documentation says that the default implementation of keyReleaseEvent ignores the event, so that the widget's parent can interpret it. What we are trying to achieve seems to be the expected behavior. We can close this if you think this bug is related to Qt, not LMMS.

Also in the latest version, context menus seem to block all key events. Should we do the same with QMenu for more consistent behavior? If so, I can change this PR accordingly, and change other QMenus to use the new Menu class.