Knobs/sliders: avoid stuck 'pressed' state after resetting with Ctrl + left-click on macOS

[ronso0]

On macOS, Ctrl + left-click is a native solution to emulate right-clicks
with a one-button mouse / touchpad. Unfortunately, this 'right-click'
doesn't emit a release event, thus, when issueing such a right-click,
to reset a control to default value, m_rightButtonPressed is never reset
and the knob / slider doesn't react on further mouse input until Ctrl is
pressed/released again.

Simply using QContextMenuEvent seems to be the easiest way to fix this,
which also allows to use any physical button for dragging knob and slider
handles.

Closes #10831

[ronso0]

@foss- @m0dB please test if this fixes the issue.

Also test if dragging with left (or middle, or any) button still works as desired, espcially if the sliders are dragged at/with a point that is not the handle.

[m0dB]

The ctrl-click now resets the sliders and the knobs without making them unresponsive. Great! But there is a new issue now: after a ctrl-click, the knob gets in a state where it hides the mouse cursor on hovering (just like how the mouse cursor disappears when the knob is being dragged). The sliders don't have this problem, but I guess that is not surprising, since they don't hide the cursor on drag either. After doing a real right-click or after left-click-dragging (changing value) the knob gets back into it's normal state.

On a sidenote, I am surprised that you have to resort to using QContextMenuEvent. I am developing with Qt as well and I don't think I ever have had issues with receiving simulated right button press and release events. I will test this tomorrow. What version of Qt are you using?

[m0dB]

I have been looking at the code and I am wondering if the problem isn't in WWidget::event, where mouse events are being synthesized from touch events. This seems a likely spot for the right click to get lost. I will try to get set up to build mixxx myself and run it in the debugger, but I see that I need to build some required third-party libraries first. Is there a list of dependencies somewhere?

Anyway, it might be interesting to revert your changes and disabling touch events altogether (not calling setAttribute(Qt::WA_AcceptTouchEvents); in WWidget constructor), and leave the touch event to mouse events mapping to Qt / the OS. I guess there are situations where you do need to do this yourself, to support certain hardware, but in my experience, if you don't call setAttribute(Qt::WA_AcceptTouchEvents); you'll get mouse events from the macbook trackpad just fine.

[ronso0]

Thanks for testing and investigating the issue further.

I see that I need to build some required third-party libraries first. Is there a list of dependencies somewhere?

take a look at https://github.com/mixxxdj/mixxx/#building-mixxx
IIRC the buildenv script should download the vcpkg with prebuilt dependencies.
Btw Mixxx is currently lacking developers on macOS, recently it was even considered to hold back the 2.4 macOS release due to performance issues that can only be resolved with access to a mac machine and reasonable c++/Qt dev experience...
So any help on macOS is greatly appreciated!
If you're interested join us on Zulip.

If you find an alternative solution, great!
One thing to note: due to issues waveform engine Mixxx still needs to be built with Qt 5.12.8 on macOS, also an alternative fix needs to be verified with various macOS versions.
Thus I consider the contextMenuEvent fix safe to be included into the next 2.3 bugfix release. More elaborate changes would go to the main branch.

after a ctrl-click, the knob gets in a state where it hides the mouse cursor on hovering (just like how the mouse cursor disappears when the knob is being dragged)

Oh, so it still suffers the issue of lacking a mouse release event (cursor is unset in mouseReleaseEvent).
Actaully I'm surprised Mixxx receives both a contextMenuEvent and a mousePressEvent...
I'll revert to ignore right-click clicks so they are captured only by contextMenuEvent.

Anyway, it might be interesting to revert your changes and disabling touch events altogether (not calling setAttribute(Qt::WA_AcceptTouchEvents); in WWidget constructor), and leave the touch event to mouse events mapping to Qt / the OS. I guess there are situations where you do need to do this yourself, to support certain hardware, but in my experience, if you don't call setAttribute(Qt::WA_AcceptTouchEvents); you'll get mouse events from the macbook trackpad just fine.

IIRC the click event synthesization in WWidget::event is required only for the [Controls], touch_shift control.

[m0dB]

Ok, after running source tools/macos_buildenv.sh setup I was able to start a build. That was surprisingly easy!

I will investigate the issue further when I have some time. I also have experience with moving from Qt 5.12.x to Qt 5.15.9, so I can give that a try as well. (Qt 5.15.9 provides M1 support, so that could be interesting as well).

I still suspect the event synthesization in WWidget::event, in combination with the code in MixxxApplication::notify, to be at fault. I'll let you know.

[ronso0]

Good to hear you're up & running!

The 2nd commit should fix the blank cursor issue (and ignores undesired mouseMoveEvents), please test that.

I still suspect the event synthesization in WWidget::event, in combination with the code in MixxxApplication::notify, to be at fault. I'll let you know.

Sure, go ahead!

I also have experience with moving from Qt 5.12.x to Qt 5.15.9, so I can give that a try as well.

Great. Though, what's holding back the Qt upgrade to 5.15 is that the rather complicated waveform code relies on QGLWidget which has some nasty, platform-specific regressions in 5.15. IIUC the current goal is to rewrite the GUI with QML
Before investing any work on that I strongly suggest you join us on Zulip > https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/mixxx.202.2E4.20macos.20regressions

[m0dB]

Ok, I was wrong about that. Without setAttribute(Qt::WA_AcceptTouchEvents); and without any Touch.. event handling or mouse event synthesis the problem still occurs; no event arrives at all on release. Very strange. I'll investigate a bit more later on.

[m0dB]

I am logging press and release events in MixxxApplication::notify. I do get the simulated right click event there. But: I get all press and release events twice and the simulated right click release only once. Maybe this helps.

[ronso0]

Thanks for the info.

For now: @foss- @m0dB please test again

Also test if dragging with left (or middle, or any) button still works as desired, espcially if the sliders are dragged at/with a point that is not the handle.

[daschuer]

Did you try to keep the Qt::RightButton case and just remove the m_bRightButtonPressed flag.

For my understanding is the remove of that flag the actual fix and not the different event, that is also triggered by the menu button and Shift-F10 https://docs.microsoft.com/en-us/windows/win32/menurc/wm-contextmenu

I have crawled the QT source and could not clearly identify a cocoa signal creates the context menu event like in windows. My guess is that the signal is a windows only signal and QT fakes it for the others targets.

The Crtl modifier in cocoa is also Implemented in QT itself:
https://github.com/qt/qtbase/blob/d8efc8d718e3b3a0464f321e740541f5b221a5d6/src/plugins/platforms/cocoa/qnsview_mouse.mm#L430
The related release code is in place here:
https://github.com/qt/qtbase/blob/5.12.4/src/plugins/platforms/cocoa/qnsview_mouse.mm#L355

We may single step through the code to check if the release event is even received or if it is just not forwarded.
Is there a stray left button release event instead?

There are some related changes in that file (among others):
qt/qtbase@3dedb52
But no obvious fix.

A side catch is this code:
https://github.com/qt/qtbase/blob/accc833e556ba54038d1cc7e261a936492145240/src/widgets/widgets/qcombobox.cpp#L2528

[daschuer]

Even if the issue is fixed here for the knobs, the issue will probably remain for buttons.

[ronso0]

Did you try to keep the Qt::RightButton case and just remove the m_bRightButtonPressed flag.

For my understanding is the remove of that flag the actual fix and not the different event, that is also triggered by the menu button and Shift-F10 https://docs.microsoft.com/en-us/windows/win32/menurc/wm-contextmenu

That alone would allow moving sliders with the right button which is unusable because the control is already reset on press.
So, removing the m_bRightButtonPressed flag + rejecting moveEvents for the right button would work, yes.

I have crawled the QT source and could not clearly identify a cocoa signal creates the context menu event like in windows. My guess is that the signal is a windows only signal and QT fakes it for the others targets.

It works well, though of course the alternative is preferred if it works well, too.

The Crtl modifier in cocoa is also Implemented in QT itself:
...
The related release code is in place here:
https://github.com/qt/qtbase/blob/5.12.4/src/plugins/platforms/cocoa/qnsview_mouse.mm#L355

Thanks for digging!
So the right release event should be sent correctly. Maybe a regression from #3165 ?
@m0dB Where exactly in MixxxApplication::notify did you observe the signals?

[Swiftb0y]

Playing Be's role here: This issue will persist as long as we have our XML-based skin system.

[m0dB]

@ronso0 I am simply logging in

    switch (event->type()) {
    case QEvent::MouseButtonPress: {
    ....
    case QEvent::MouseButtonRelease: {
    ....

and I see that I get here twice (!) for each mouse button press or release, and only once on the simulated right button release.

The comment in the top mentions that "All touch events are translated into two simultaneous events".

Edit: okay, reading up on the Qt documentation of notify, I understand that it is expected that notify can get called multiple times for each event, for different receivers, so the question seems to be why does the widget stop being a receiver for this event.

[ronso0]

Edit: okay, reading up on the Qt documentation of notify, I understand that it is expected that notify can get called multiple times for each event, for different receivers, so the question seems to be why does the widget stop being a receiver for this event.

@m0dB Just for the fun of it, could you check which events are received if you Ctrl+click

• into an already focused text edit in the track properties window
• into an already focused beatsize spinbox

Just trying to figure if the issue is indead related to WWidget only or to QWidgets, too.

[m0dB]

Ok, after going through a lot of mixxx code (well, at least I learned a thing or two) I decided to try with a minimal Qt app. Should have done that first thing... because it shows that the problem is in Qt itself! Even with this minimal app there are no release events for emulated right-click... Code attached.

test.zip

[m0dB]

I just tried compiling the test against Qt 5.15.8, which works fine.

[Swiftb0y]

Welp, either we ditch that Qt version (requires GUI rewrite) or we backport yet another 5.15 patch...

[foss-]

This is an interesting rabbit hole. The odd behavior around release events may also explain the odd behavior of Mixxx on macOS when clicking on star ratings (#10110) which then would be just one of many strange side effects I guess.

Probably also related: #10614

The Qt version trouble really is a dilemma :(

[m0dB]

@Swiftb0y I looked for the fix in the Qt code and changelogs, but so far no luck.

I have a way to work around the Qt bug, which is posting the missing release event for the widget when we get the release event for the widgetwindow (and avoid posting it twice in case we do get the real release event after that). See attached file. I tried this in my test and it works fine, I will try using it in mixxx next. But of course this is not ideal either.

test2.zip

[m0dB]

Yes, this works. With this patch I can reset knobs and sliders with the simulated right click and at a first glance I haven't noticed any side effects. I don't know if this approach is preferable over @ronso0's contentMenuEvent solution though.

post_release_event_copy.patch.txt

[ronso0]

As discussed above the context menu method is not preferred. Tracking left & middle pressed state and remove tracking the right button works as well and is cleaner.
I'll polish that and force-push later on, so we have a fix handy at least for the knobs and sliders, just in case patching vcpkg for macOS is not desired.
I don't have access to a mac so that'll be it, nuff time spent on this now that we (you) found the root cause.

[m0dB]

Sounds good. I'll test the fix once you have it ready.

I joined Zulip as you suggested, @ronso0 , to ask about building with Qt 5.15.x. I am new to Zulip so I hope I followed the correct procedure, creating a stream under development.

[ronso0]

I'll polish that and force-push later on, so we have a fix handy at least for the knobs and sliders, just in case patching vcpkg for macOS is not desired.

Done. Works fine as before here (Linux with Qt 5.12.8)
Please test @m0dB @foss-

[foss-]

Sliders and Knobs remain usable after being reset with control+click using 2.3.3-32-geabf91c612 (HEAD) on macOS 12.5.1.

[ronso0]

Sliders and Knobs remain usable after being reset with control+click using 2.3.3-32-geabf91c612 (HEAD) on macOS 12.5.1.

Is this the macOS CI build of this PR?

[foss-]

Is this the macOS CI build of this PR?

Yes

[Be-ing]

Welp, either we ditch that Qt version (requires GUI rewrite) or we backport yet another 5.15 patch...

Backporting a bugfix from 5.15 is a more practical approach than trying to fix the problems with 5.15.

[daschuer]

@m0dB Is there a left button release event instead of a right button release event in case of Qt 5.12 and a Ctrl click?

Can you please share backtraces of a break point in the TestWidget::mousePressEvent and TestWidget::mouseReleaseEvent using a Ctrl click in Qt 5.15 and Qt 5.12

There have been some changes applies to https://github.com/qt/qtbase/blob/5.12.4/src/plugins/platforms/cocoa/qnsview_mouse.mm
Maybe by luck the change is in that file.

[m0dB]

Indeed the fix is qnsview_mouse.mm, it was not easy to find. As discussed in Zulip:

5.12.3:

    QWindowSystemInterface::handleMouseEvent(targetView->m_platformWindow->window(),
                                             timestamp, qtWindowPoint, qtScreenPoint,
                                             buttons, button, eventType, modifiers);

5.15.5:

    QWindowSystemInterface::handleMouseEvent(targetView->m_platformWindow->window(),
                                             timestamp, qtWindowPoint, qtScreenPoint,
                                             m_buttons, button, eventType, modifiers);

Changing buttons to m_buttons in Qt 5.12.3 qtbase/src/plugins/platforms/cocoa/qnsview_mouse.mm:268 fixes the issue with the lost simulated right click. It would be great if the vcpkg could be updated with a Qt 5.12.3 build with this fix applied.

[ronso0]

@m0dB In case you can reproduce #10847 without the fix, could you check if that is fixed the buttons change, too?
Note: #10847 is about Mixxx 2.4-alpha which is built with Qt 5.12.4 it seems (I'm absolutely not into vcpkg)

[ronso0]

superseded by #10869