plugin-volume: add support for hires wheel

[kreijack]

Problem

The user can't control with the mouse wheel the volume level in the Volume Control plugin.

Analysis

Some mouses send a lot of events with "small" movement of the wheel. This invalidate some evaluation of the total stroke of the wheel.

In the Volume Control plugin, the volume was computed on the basis of the following formula:

   new_volume = old_volume +
                event->angleDelta().y() /
                QWheelEvent::DefaultDeltasPerStep *
		volumeSlider->singleStep()

However if event->angleDelta().y() is smaller than QWheelEvent::DefaultDeltasPerStep, their ratio is a value less than 1, which is zero when the math is integer based.

If the math was floating point based, the problem would not happened.

Solution

To avoid this issue the volumeSlider->singleStep() is increased by a factor of 1000 and the multiplication is performed before the division.

Of course all others computation will be scaled according:

• the range of the volume slider will be 0..100*1000 (where previous was 0..100)
• the volume will be computed as: volume = volumeSlider->value() / 1000
• volumeSlider->singleStep will be increased by a factor of 1000

[tsujan]

The user can't control with the mouse wheel the volume level in the Volume Control plugin.

Not reproducible.

Some mouses send a lot of events with "small" movement of the wheel.

You'd have problem with that mouse everywhere; it should be broken.

[kreijack]

You'd have problem with that mouse everywhere; it should be broken.

Why ? if the total (sum of the movement) is equal I should not have any problem.

Anyway, which is the value returned by
event->angleDelta().y() ?
for me is 15, of course I am having more events than a mouse with lower precision.

What is reporting libinput record /dev/input/event<NN_of_mouse> ? Do you have "REL_WHEEL_HI_RES"

[...]
   #   Event code 580 (KEY_APPSELECT)
    #   Event code 581 (KEY_SCREENSAVER)
    #   Event code 582 (KEY_VOICECOMMAND)
    #   Event code 583 (KEY_ASSISTANT)
    #   Event code 584 (KEY_KBD_LAYOUT_NEXT)
    #   Event code 585 (KEY_EMOJI_PICKER)
    #   Event code 586 (KEY_DICTATE)
    #   Event code 592 (KEY_BRIGHTNESS_MIN)
    #   Event code 593 (KEY_BRIGHTNESS_MAX)
    #   Event code 608 (KEY_KBDINPUTASSIST_PREV)
    #   Event code 609 (KEY_KBDINPUTASSIST_NEXT)
    #   Event code 610 (KEY_KBDINPUTASSIST_PREVGROUP)
    #   Event code 611 (KEY_KBDINPUTASSIST_NEXTGROUP)
    #   Event code 612 (KEY_KBDINPUTASSIST_ACCEPT)
    #   Event code 613 (KEY_KBDINPUTASSIST_CANCEL)
    # Event type 2 (EV_REL)
    #   Event code 0 (REL_X)
    #   Event code 1 (REL_Y)
    #   Event code 6 (REL_HWHEEL)
    #   Event code 8 (REL_WHEEL)
    #   Event code 11 (REL_WHEEL_HI_RES)                            <----------------
    #   Event code 12 (REL_HWHEEL_HI_RES)                           <----------------
    # Event type 3 (EV_ABS)
    #   Event code 32 (ABS_VOLUME)
    #       Value           0
    #       Min             1
    #       Max           767
    #   
   #   Event code 11 (REL_WHEEL_HI_RES)
    #   Event code 12 (REL_HWHEEL_HI_RES)
[...]

I can ask which kind of mouse do you have ? I have a "Logitech MX 2 Anywhere", and this show this behavior.

Here http://who-t.blogspot.com/2021/08/libinput-and-high-resolution-wheel.html there are some explaination.

[tsujan]

OK, you meant that your mouse has a high resolution (rotates by less than 15° each time == its angle delta is less than 120). The sentences "The user can't control…" and "Some mouses send a lot of events…" made me think it was broken.

Yes, the code fails to cover this case. IMO, a clean solution requires adding the steps cumulatively, until they reach (or pass) 15°.

I can ask which kind of mouse do you have ?

More than 6 mice with 3 computers/laptops, whose steps were all 15°, but that's beside the point.

[kreijack]

Yes, the code fails to cover this case. IMO, a clean solution requires adding the steps cumulatively, until they reach (or pass) 15°.

I tried also this path; but this would require to add a further variable. I found faster to increase the "range" of the slider by a factor of 1000, and computing the volume as "slider->value() / 1000"

You can see this other option here
kreijack@a07bff3

[tsujan]

I haven't looked into it yet, but I think the cumulative method might be cleaner. I vaguely remember that it was used in LXQtTaskBar for moving windows to next/previous desktop by turning the mouse wheel. Your high-resolution mouse is a good opportunity for knowing whether that option works well with such mice. Would you please test it (check Task Manager's config dialog)?

[kreijack]

Your high-resolution mouse is a good opportunity for knowing whether that option works well with such mice. Would you please test it (check Task Manager's config dialog)?

It works, but it requires to turn the wheel by 3 "step" (or detent); is it the expected behavior ?

[kreijack]

I haven't looked into it yet, but I think the cumulative method might be cleaner.

This was my first thought; but then I realized that I would have to synchronize three things:

• volume (which can be adjusted by an external mixer), which in turn has to adjust wheel counter and slider
• slider (which can be adjusted by the mouse when mouse move the cursor), which in turn has to adjust the volume and the wheel counter
• the wheel counter (which can be adjusted by the wheel), which in turn has to adjust the volume and the slider

Anyway I will give another chance to having a counter in a separate variables

[tsujan]

It works, but it requires to turn the wheel by 3 "step" (or detent); is it the expected behavior ?

It has a configurable "Wheel Delta Threshold".

Anyway, I was thinking about a static local variable, like the one used in LXQtTaskBar::wheelEvent. Now I'm too drowsy to continue our discussion; maybe tomorrow, with some code.

[tsujan]

Before I go to sleep — I mean something like this:

void VolumePopup::handleWheelEvent(QWheelEvent *event)
{
    static int _delta = 0;
    _delta += event->angleDelta().y();
    if (qAbs(_delta) >= QWheelEvent::DefaultDeltasPerStep) {
        m_volumeSlider->setSliderPosition(m_volumeSlider->sliderPosition()
                + (_delta / QWheelEvent::DefaultDeltasPerStep * m_volumeSlider->singleStep()));
        _delta = 0;
    }
}

Does it feel natural with your mouse?

[palinek]

void VolumePopup::handleWheelEvent(QWheelEvent *event)
{
static int _delta = 0;

I'm afraid, this static is not a good solution as this implicitly requires the volume plugin to be singleton object. When there would be more volume widgets in lxqt-panel process, their usage of _delta would clash.

[tsujan]

... requires the volume plugin to be singleton object

You're right; I missed that last night. It should be replaced by a member variable.

But what was important to me was UX with a hi-res mouse (which I don't have). In theory, it seems OK: the volume will change if the user turns the wheel by ~15° — one step for most mice, more for hi-res mice.

Still waiting for @kreijack's reply....

[tsujan]

@palinek
On second thought, the static-ness can't have an impact on UX. Successive volume changes are still separated from each other by a 15-degree wheel turn.

[palinek]

Successive volume changes are still separated from each other by a 15-degree wheel turn.

Yes, but you can be taking "leftover" from one widget when processing the event on other widget. The same comes into play even with one widget -> scroll below threshold, leave widget, scroll on the widget again. I know all these are rather academical scenarios with close to no impact on UX, but pointing it out just for the sake of correctness.

[tsujan]

The user can be sure that the volume changes only once when he turns the wheel by 15°; practically, it makes no difference at which smaller degree it changes.

The main point of a cumulative approach is that, without it, a high sensitivity could be annoying with a hi-res mouse.

[kreijack]

What about declaring _delta as "member variable" ? Moreover the "_delta" variable handling has to consider that:

• event->angleDelta().y() may be > 0 or < 0
• we need to preserve the fraction part when we pass the value to setSliderPosition
• when the slider is moved via a mouse and/or due to a change of the volume by an external mixer, the fraction part has to be reset to 0

I reworked a bit the @tsujan suggestion

// file volumepopup.h

class VolumePopup {
[...]
    int fractionWheelValue = 0;
}

// file volumepopup.cpp
void VolumePopup::handleWheelEvent(QWheelEvent *event)
{
    fractionWheelValue += event->angleDelta().y();

    // check if fractionWheelValue is enough to change the volume
    // pay attention that fractionWheelValue may be > 0 or < 0
    if (fractionWheelValue / QWheelEvent::DefaultDeltasPerStep != 0) {
        m_volumeSlider->setSliderPosition(m_volumeSlider->sliderPosition()
                + (fractionWheelValue / QWheelEvent::DefaultDeltasPerStep * m_volumeSlider->singleStep()));

        // we need to preserve the fraction part of the wheel movement
        fractionWheelValue = fractionWheelValue % QWheelEvent::DefaultDeltasPerStep ;
    }
}

[...]
void VolumePopup::handleDeviceVolumeChanged(int volume)
{
    // we are updating the slider, reset the fractional part of the wheel movement
    fractionWheelValue = 0;
[...]

void VolumePopup::handleSliderValueChanged(int value)
{
    // we are updating the slider, reset the fractional part of the wheel movement
    fractionWheelValue = 0;
[...]

// 

[tsujan]

What about declaring _delta as "member variable" ?

Makes no difference. This way was more compact; hence suggesting it.

event->angleDelta().y() may be > 0 or < 0

My mistake. Use qAbs for comparison (corrected the snippet).

You didn't answer my question: After using qAbs, and apart from theoretical considerations, does it feel natural with your mouse? If not, would you explain why?

[kreijack]

What about declaring _delta as "member variable" ?

Makes no difference. This way was more compact; hence suggesting it.

event->angleDelta().y() may be > 0 or < 0

My mistake. Use qAbs for comparison (corrected the snippet).

You didn't answer my question: After using qAbs, and apart from theoretical considerations, does it feel natural with your mouse? If not, would you explain why?

Ah sorry, I forgot to test it. Anyway I don't see any reasons why it should not work.

I don't like the idea of having _delta static; because it is a value per instance. From a practical point of view this don't have any impact (we already have 100 levels of volume, the fractional part is negligible offset, we need only to guarantee the smoothness of an activation).
Only this is ugly, and my sense of aesthetic reject it :-)

However we need
``
`fractionWheelValue = fractionWheelValue % QWheelEvent::DefaultDeltasPerStep ;

to guarantee the smoothness (otherwise time to time we lost a fraction of tick)


I will try a test

[tsujan]

I don't see any reasons why it should not work.

That wasn't an answer to the question I asked ;) I wouldn't ask if I had a hi-res mouse.

my sense of aesthetic reject it

I don't want to start an aesthetic discussion. A simple code that works is preferable — the simpler, the better. And static is in no way a taboo.

[kreijack]

I don't see any reasons why it should not work.

That wasn't an answer to the question I asked ;) I wouldn't ask if I had a hi-res mouse.

(it was "trapped" in the quotation, but in the last part of my last messages there was a [...] I will try a test :-) )

Anyway the test was successful. It works. The only think is that the tooltip which show the percent of volume flickers when the wheel mouse rotates. Do you have the same problem ?

my sense of aesthetic reject it

I don't want to start an aesthetic discussion. A simple code that works is preferable — the simpler, the better. And static is in no way a taboo.

I agree with you that making _delta static is a very pragmatic approach: your proposal is a smaller changes, and this has its own payback; referring to my aesthetic sense was a joke ; I never want starting a flamewar about using static.

[tsujan]

The only think is that the tooltip which show the percent of volume flickers when the wheel mouse rotates

To show the new value, the instant tooltip is shown again. I don't see it as a problem.

[kreijack]

This is what I am seeing (ok the color are not the same, but the flickering effect is the same).
Only To be sure that we are seeing the same thing and that this is not a "side effect" of the high res wheel.

[kreijack]

Instead this is how the tooltip is showed when I use the mouse and not the wheel (and it is fine)

[tsujan]

Only To be sure that we are seeing the same thing...

Apparently, we don't:

tooltip.mp4

...and that this is not a "side effect" of the high res wheel.

I don't think so but can't tell for sure. The window manager and compositor have greater roles. I use KWin. If it still happens after changing your WM, you could open a report.

Anyway, thanks for the PR! I'll review it tomorrow.

[kreijack]

Yes there is something different. However I am out of topic. This will be another issue.

[tsujan]

I'm afraid there are 4 problems:

1. Consider a hi-res mouse whose step is 10°. If 0 means no volume change and 1 means a change, we'll get 011011011… by turning its wheel. In other words, the volume is changed inconsistently. The problem you saw in the tooltip was actually caused by this.
2. Hi-res mice shouldn't change the volume with greater angular speed compared to ordinary mice (= one change in ~15°).
3. There's an irregularity when the wheel direction is reversed (with my snippet too).
4. It can be very erratic with touchpad scrolling. A touchpad can create deltas as small as 1. (Touchpad scrolling isn't handled by the master branch either.)

The following PR doesn't have the above problems and guarantees that a tooltip is shown in the end: #1857

[palinek]

I don't like the idea of having _delta static; because it is a value per instance.

I, for one, completely agree here. The reasons are not aesthetic, but I already described the scenarios and you correctly addressed those.

[tsujan]

What was called "leftover" exists with a single instance too. When it can be ignored with a single instance, it can also be ignored with more — no practical impact (→ #1856 (comment) → first paragraph).

[palinek]

What was called "leftover" exists with a single instance too

No, it doesn't exist in single instance, since it was handled in the @kreijack's snippet

[tsujan]

That code is basically incorrect (→ #1856 (comment)). You can't get rid of the leftover in that way. You can only reset it with a timer (like the one in #1857), by assuming an end to a series of wheel rotations. But even that isn't worth cluttering the code.

[kreijack]

I'm afraid there are 4 problems:

1. Consider a hi-res mouse whose step is 10°. If `0` means no volume change and `1` means a change, we'll get `011011011…` by turning its wheel. In other words, the volume is changed inconsistently. 

The problem you saw in the tooltip was actually caused by this.

You assumption is that the "unit step" is a "non divisor" (i.e. x % y = 0) of the step (now 15°). The reality is that the hires step is a divisor of the step. This is true for my mice, and this should be true for all the hires wheel, and this to avoid you concern.
In fact my mouse returns +/-15, which is a divisor of DefaultDeltasPerStep (120/15 = 8, 120%15 = 0)

2. Hi-res mice shouldn't change the volume with greater angular speed compared to ordinary mice (= one change in ~15°).

Could you elaborate this sentence ? The speed is the same: the steps are smaller but more frequent.

3. There's an irregularity when the wheel direction is reversed (with my snippet too).

I am not saying that this is not true, but really it is not clear to me what is the irregularity

4. It can be very erratic with touchpad scrolling. A touchpad can create deltas as small as 1. (Touchpad scrolling isn't handled by the master branch either.)

May be I am missing something obvious, but please could you elaborate a bit ?

The following PR doesn't have the above problems and guarantees that a tooltip is shown in the end: #1857

I tested your code, and the tooltip problem is still here.

[tsujan]

I assumed that the above explanations were clear enough. I'm very sorry that I don't have time for more (hence, my short replies and my avoidance of repeating myself).

As for testing with a hi-res mouse, there was a simple way of simulating any kind of it in the code. Everything is fully tested, not just theorized. Moreover, touchpad serves as a super hi-res tool when it comes to scrolling.