Mouse wheel horizontal axis control

Added attachment:

patch-mouse_wheel_horizontal.diff

I can confirm this is still true in VLC 3.0.8, 21 months after the original report. I can confirm this is also true on Windows.

Sent patch to mailing list but it wasn't merged nor commented.

@gudvinr did we merge your patch in the meantime?

I don't think so. However, I did mention it couple of times on IRC. j-b asked to test this with VLC 4 and it works there too. Path to customwidgets.cpp is different but this part of the code same as in VLC 3.

Thanks a lot for the immediate follow-up!

Hi, in my option the patch is not correct, if you have mouse with a round scrollwheel (like old Mac mouses) you're going to emit erratic mouse wheel events. I think you need to compute the direction (in terms of angle).

This issue is almost three years old and no one except couple of folks noticed that.
I assume that not many people who use mouse with multiple wheels aware about this.
And probably there's a lot less people who use old mac mouses if any.

While it is a fair point that these kind of controllers exist, they will have same issues as they have now.

Right now e->delta() is either positive or negative. Which means if you have scroll surface with 360⁰ resoulution it will emit KEY_MOUSEWHEELDOWN when you scroll from top to bottom or from right to left (since it doesn't check for orientation) and KEY_MOUSEWHEELUP otherwise.

Since orientation() and delta() are deprecated I intentionally used angleDelta:

    QPoint p = e->angleDelta();
    if      (p.y() > 0) i_vlck |= KEY_MOUSEWHEELUP;
    else if (p.y() < 0) i_vlck |= KEY_MOUSEWHEELDOWN;

It is quite apparent that it will not emit erratic mouse wheel event. While you scroll across same surface, delta will have non-zero y() value and either of first two checks will cause this if-else statement to end. It might produce unpredictable outcome in some edge cases but movement still won't be chaotic.

For a proper solution, however, it is indeed would be better to introduce orientation vector of some sort with a narrow cone of allowed values to determine precise direction.
But is it really worth it if there's no feedback from these users with round scrollwheel? It will take a considerable amount of time that I don't want to spend on something that I can't use.

Although one thing that I didn't know about is QWheelEvent::phase(). It probably should not matter anyway but this might help:

+    QPoint p = e->angleDelta();
+    if (p.isNull())     { /* scrolling distance did not yet change */ }
+    else if (p.y() > 0) i_vlck |= KEY_MOUSEWHEELUP;
+    else if (p.y() < 0) i_vlck |= KEY_MOUSEWHEELDOWN;
+    else if (p.x() > 0) i_vlck |= KEY_MOUSEWHEELLEFT;
+    else if (p.x() < 0) i_vlck |= KEY_MOUSEWHEELRIGHT;

It is quite apparent that it will not emit erratic mouse wheel event. While you scroll across same surface, delta will have non-zero y() value and either of first two checks will cause this if-else statement to end. It might produce unpredictable outcome in some edge cases but movement still won't be chaotic.

If you scroll along the X axis and have some minor movement along the Y axis you will emit wheel up/down events

While it is a fair point that these kind of controllers exist, they will have same issues as they have now.

Now they don't have horizontal scrolls, then they gain horizontal scrolls but sometimes have erratic scrolls.

But is it really worth it if there's no feedback from these users with round scrollwheel?

I guess we would if we introduce this kind of changes

For a proper solution, however, it is indeed would be better to introduce orientation vector of some sort with a narrow cone of allowed values to determine precise direction.

isn't that exactly the information provided by angle delta. I expect something like this to do the trick (untested)

if (abs(p.x()) > abs(p.y())) 
{
    if (p.x() > 0) i_vlck |= KEY_MOUSEWHEELLEFT;
    else if (p.x() < 0) i_vlck |= KEY_MOUSEWHEELRIGHT;
} 
else 
{
    if      (p.y() > 0) i_vlck |= KEY_MOUSEWHEELUP;
    else if (p.y() < 0) i_vlck |= KEY_MOUSEWHEELDOWN;
}

I have this kind of mouse somewhere at home, I'll give it a try.

If you scroll along the X axis and have some minor movement along the Y axis you will emit wheel up/down events

This is one of the edge cases that I did mention, yes.

isn't that exactly the information provided by angle delta. I expect something like this to do the trick (untested)

You will have same kind of erratic movements if you try to scroll along the diagonal where abs(x) almost same as abs(y). To avoid that you need some kind of dead zone between x and y axis which would be wide enough to completely ignore events from "probably diagonal movements".

If you calculate cosine for angle delta like abs(x)/sqrt(x²+y²) then values over 0.95 will represent angles inside ~15° cone around X axis and values below 0.26 represent angles inside ~75° cone around Y axis which gives about 30° of freedom for each direction.

Yes that would be better.

@chub can you check this one with your mouse?

First thanks for the patch.

So I did some tests with the apple mouse. As a disclaimer my mouse is very old and behave poorly, scrolling down scrolls mostly scrolls, and scrolling up scrolls, well ... never up, sometimes down , scrolling left & right seems OK.

from my observation, with this mouse on linux, mouse events are never reported with a proper orientation, I either got full up/down/left or right direction with a full step (+/- 120), so the implementation doesn't changes much in this case

Though a better test is 2 finger scrolls on touchpads, where it does provides proper 2D scroll event, I feel the 30° radius is a bit low for the Y axis, seemed OK for the X axis, but maybe that's just my hardware, or that I wouldn't notice if I wasn't paying attention to it. I think you can reproduce the same test on your side if you have a laptop.

Anyway, that's just nitpicking, the implementation looks fine to me (I'm okay with the current values, we'll see afterwards if we get any feedback on this)

I think we can make allowance zone for vertical axis wider.
I made up these numbers for abstract symmetrical scrollwheel. Since touchpad is not symmetrical it makes sense that it feels too narrow.

You can try to crank up v_cos_deadzone to 0.45 and see if that's better. That's roughly correspond to cosine of 15°×16/9 where I made an assumption that touchpad has same aspect ratio as popular display.

It won't make any difference for users with discrete scrollwheels but I guess touchpad will be a lot better playground than trackball.

look good to me

OK then. I synced my fork to latest upstream but I am unable to create merge requests to main repository so it is up to you now.

Keep in mind that file moved from modules/gui/qt/util/customwidgets.cpp in vlc-3 to modules/gui/qt/widgets/native/customwidgets.cpp in vlc-4.

There's also check in this method in vlc-4:

#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)

I don't know about supported Qt versions in vlc-4 but delta() deprecated and angleDelta present at least since Qt 5.5 so it may be safe to drop this check.

I am unable to create merge requests to main repository

yes we still uses mailing list for patches at the moment. You can send it as either a patch for 3.0 or 4.0, and I'll merge it in both versions.

deprecated and angleDelta present at least since Qt 5.5 so it may be safe to drop this check

It should be droppable for vlc 3 too, as 5.5 is the minimal required version

@chub since you pointed out that using raw y and x from angleDelta may end up badly for trackpads, I searched across videolan/vlc and found couple of places where it also might backfire:

qt/maininterface
qml/VolumeWidget
qml/EditorDNDView
qml/EditorDNDDelegate (uses method from previous view)

But these places only rely on vertical scroll so it might be better to check for abs(y) >= %single wheel step% and only then check for direction.
Otherwise trackpads with hires scroll feature will produce seemingly random events when you move fingers horizontally.

This is kind of related to what we got here but I can't test this theory.

changed milestone to %3.0.x maintenance

added Component::Interface: Qt Version::3.0.x labels

assigned to @chub

You can send it as either a patch for 3.0 or 4.0, and I'll merge it in both versions.

https://mailman.videolan.org/pipermail/vlc-devel/2021-April/142966.html

Hi @gudvinr, I saw your patch in the mailing list.

I added the following check to 4.0 in 91bce9ab

#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)

Just to suppress build warnings. And during checking I noticed some of the issues you mentioned but I intentionally tried to not alter the semantics of the code as the commit was only intended to be a replacement to fix build warnings.

I had to check Qt source code to see how angleDelta() and delta() differs and I concluded to use angleDelta().y() as a replacement to delta(). I first thought about if checking x() would make sense, but I did not do it because as I said I just wanted to fix the warnings but if you fixed the problem with this patch that's very good.

For why, I chose to check Qt 5.15 instead of Qt 5.5 or earlier, I use Qt 5.11.3 and Qt 5.15.2 to build the project and even though delta() is apparently deprecated starting with 5.0, usage of delta() does not emit build warnings on 5.11 because it is not marked deprecated in the 5.11 header files unlike 5.15.

And when I checked 5.15 header files, I saw this:

#if QT_DEPRECATED_SINCE(5, 15)
    // Actually deprecated since 5.0, in docs
    QT_DEPRECATED_VERSION_X_5_15("Use angleDelta()")
    inline int delta() const  { return qt4D; }
...

So I thought using 5.15 would make sense. angleDelta() was only tested by me so I wanted it restrict to only 5.15 to suppress the warnings. Maybe it is a better idea to obey the Qt docs, I don't know.

There's no need to check for Qt version with this patch since it only use angleDelta. I just pointed out that last time I checked it wasn't there but can be safely removed.

closed with commit e8d929f3

changed milestone to %3.0.13

marked #20444 (closed) as a duplicate of this issue

marked this issue as related to #20444 (closed)

Mouse wheel horizontal axis control

Child items ...

Activity