LCD Widgets reduce value by 2 when finishing trackpad "wheel scroll" action

[follower]

Here is a demonstration using the tempo control:

The following events happen in the gif:

1. When two fingers are placed first on the trackpad (the beginning of the wheel scroll motion) the tempo drops from 140 to 138;
2. After a pause, the normal scrolling action changes the tempo to 128;
3. The fingers are then lifted off the trackpad and the tempo mistakenly changes to 126.

More details on cause and suggested solution to follow.

[Umcaruje]

@follower I'm assuming this is a mac issue, cause I can't reproduce on linux, neither with my touchpad not my mouse.

[follower]

Yes, it is Mac-only caused by the addition of Qt::ScrollPhase / QWheelEvent::phase() in Qt 5.2.

I've got more details and plan to put together a PR for this specific case but also want to get feedback on implementing a solution for all other occurrences.

(TL;DW: .delta() can now return 0 (when scroll phase is begin/end) in addition to positive or negative values which means that all ternary conditionals fail to work correctly.)

I recommend ignoring this ticket until I add the additional information. :)

[zonkmachine]

This sounds like a Qt bug.

@follower What version of macOS/Qt do you see this issue with?

[follower]

It's not specifically a Qt bug but a change in behaviour coupled with a coding idiom/incorrect coding assumption in LMMS. (It could be argued that the non-backward compatible behaviour is a Qt bug but...)

Issue: Delta events should be ignored when in phase start/end

Specifically, the idiom is the use of the ternary operator to evaluate mouse wheel delta changes (e.g. see):

( _we->delta() > 0 ) ? 1 : -1

Although there are also places where if occurrences use the same assumption.

The problem being that any action should only happen if the ScrollPhase value also has the correct phase value.

Number of occurrences

From memory there are about a dozen places where QWheelEvent/->delta() occurs--not all result in incorrect behaviour but a number of them do. (GitHub code search is "suboptimal" but see here & here.)

In most cases the workaround/fix (an early exit from the method) is relatively straight-forward but in some cases it is more complicated due to questions about correct event propagation behaviour.

Current fix status

Somewhere I have an old LMMS master with ~2/3rds of the occurrences fixed, I will work on trying to find it and flick a patch somewhere.

One of the primary reasons I stalled on the patch was because it seemed like a "code smell" that the idiom is used in so many places without a common implementation that is used (e.g. even the variable name given to the QWheelEvent instance chances between _me, event, we) and would only need to be changed once.

While the code duplication is a factor, in order to avoid a "perfect is the enemy of good" situation, I think patching the easily changed places would be worthwhile.

(In terms of versions affected, I think I've observed it in Qt 5.5 & 5.6 but from the documentation it seems it would be Qt 5.2+.)

[follower]

Found a couple of related open tabs...

• The additional check required is that Qt::ScrollPhase has the value Qt::ScrollUpdate (see).

• Another factor contributing to patch progress stalling was the discovery that .delta() has actually been deprecated and replaced by .pixelDelta() or .angleDelta() (see) so the code duplication becomes more of a hassle.