Bug 316546 - KStatusNotifierItem capture mouse wheel events three times. kmix and amarok get 3x volume change
Summary: KStatusNotifierItem capture mouse wheel events three times. kmix and amarok g...
Status: RESOLVED FIXED
Alias: None
Product: plasma4
Classification: Plasma
Component: notifications (show other bugs)
Version: 4.10.1
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords:
: 313579 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-03-11 15:42 UTC by Enrico Tagliavini
Modified: 2013-12-14 10:12 UTC (History)
17 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.11.4


Attachments
kDebug output for kmix for a single wheel movement (6.78 KB, text/plain)
2013-03-11 15:43 UTC, Enrico Tagliavini
Details
Suppressed redundant MouseEventListener signals (663 bytes, patch)
2013-06-11 23:52 UTC, naur
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Enrico Tagliavini 2013-03-11 15:42:07 UTC
When using the mouse wheel to change volume from a tray icon on kmix or amarok I get 3 times the volume change I expect. For kmix the default is 5%, I get 15%, for amarok I get 12%. I think the problem is KStatusNotifierItem capture 3 events and not just one. Example of amarok debu log (I just paste what happens when using the wheel on the icon, for a single movement of the wheel, a.k.a 15 degrees, if more is needed ask :) ).



    amarok: BEGIN: void Amarok::MediaPlayer2Player::volumeChanged(int)
    amarok:   MPRIS2: Queueing up a PropertiesChanged signal
    amarok: END__: void Amarok::MediaPlayer2Player::volumeChanged(int) [Took: 0s]
    amarok: BEGIN: void Amarok::MediaPlayer2Player::volumeChanged(int)
    amarok: END__: void Amarok::MediaPlayer2Player::volumeChanged(int) [Took: 0s]
    amarok: BEGIN: void Amarok::MediaPlayer2Player::volumeChanged(int)
    amarok: END__: void Amarok::MediaPlayer2Player::volumeChanged(int) [Took: 0s]
    amarok: BEGIN: void DBusAbstractAdaptor::_m_emitPropertiesChanged()
    amarok: END__: void DBusAbstractAdaptor::_m_emitPropertiesChanged() [Took: 0s]

or kmix enabling kDebug() (will attach it in a second, it is long)

The mouse wheel works as expected when used on kmix sliders or on the volume control inside amarok.


Reproducible: Always

Steps to Reproduce:
1. start amarok --debug 
2. use the mouse wheel over the amarok icon in the system tray and look at the output
In alternative
1. enabled kmix kDebug(): start kdebugdialog and check kmix, apply
2. quit kmix (eg killall kmix)
3. start kmix again from the command line
4. use the mouse wheel with mouse over the kmix icon, save the log and check the event is captured 3 times
Comment 1 Enrico Tagliavini 2013-03-11 15:43:03 UTC
Created attachment 77956 [details]
kDebug output for kmix for a single wheel movement
Comment 2 Enrico Tagliavini 2013-04-14 09:19:53 UTC
I just installed Fedora 18 on another PC. I updated to KDE 4.10.2 [which comes with normal fedora updates] and I have exactly the same problem.
Comment 3 Rex Dieter 2013-04-17 17:04:05 UTC

*** This bug has been marked as a duplicate of bug 313579 ***
Comment 4 Rex Dieter 2013-04-17 18:09:36 UTC
I take it back, even with the fix from bug #313579 , I still seem to be seeing 15% jumps.  odd.
Comment 5 Enrico Tagliavini 2013-04-17 18:56:47 UTC
(In reply to comment #4)
> I take it back, even with the fix from bug #313579 , I still seem to be
> seeing 15% jumps.  odd.

I've done some hack around kmix to get the inc value printet. It is correct, it correspond to an increment in the volume of 5%, as expected. But it is captured 3 times in a row for a single wheel step.
Comment 6 Rex Dieter 2013-04-17 19:17:57 UTC
So, clearly the title of this report (about 3X mouse events) is accurate.

Interestingly, I was hoping that setting systemsettings->Input devices->mouse->advanced(tab)
mouse wheel scrolls by :  ...
to values other than 3 may help here, but alas, it did not.
Comment 7 Jekyll Wu 2013-05-18 04:58:15 UTC
*** Bug 313579 has been marked as a duplicate of this bug. ***
Comment 8 Adam Porter 2013-05-29 11:39:40 UTC
Bug 313579 has the patch for this bug.  It's also older and has more votes.  This bug should be a duplicate of it, not vice versa.
Comment 9 Rex Dieter 2013-05-29 13:26:56 UTC
Adam, the patch in bug #313579 doesn't help (not for me anyway).  hard-coding a step of 5% + this bug (processed events 3 times) still means a 15% jump per mouse scroll.  :(
Comment 10 Adam Porter 2013-06-01 21:59:12 UTC
Rex, I see.  This is getting confusing.  Maybe the bug was in KStatusNotifierItem all along, not kmix.  I'm not sure that these are duplicate bugs, though.
Comment 11 naur 2013-06-11 23:52:20 UTC
Created attachment 80465 [details]
Suppressed redundant MouseEventListener signals

I made a patch for plasma runtime (4.10.3) which fixes this issue.

The problem is caused by MouseEventListener item emitting multiple wheelMoved() signals for the same wheel event (once for every child item).
Attached patch suppresses redundant mouse signals.

I don't know if the original behavior of MouseEventListener was required in other parts of plasma desktop, so I might have introduced some side effects. The kmix problem is definitely solved though.
Comment 12 Enrico Tagliavini 2013-06-12 11:02:46 UTC
(In reply to comment #11)
> I made a patch for plasma runtime (4.10.3) which fixes this issue.

Thank you very much for that. Not an easy spot at all. I think you can also submit the patch the the KDE reviewboard. This might be a way to have it merged quicker
Comment 13 Enrico Tagliavini 2013-06-29 18:37:24 UTC
(In reply to comment #11)
> Created attachment 80465 [details]
> Suppressed redundant MouseEventListener signals
> 
> I made a patch for plasma runtime (4.10.3) which fixes this issue.
> 
> The problem is caused by MouseEventListener item emitting multiple
> wheelMoved() signals for the same wheel event (once for every child item).
> Attached patch suppresses redundant mouse signals.
> 
> I don't know if the original behavior of MouseEventListener was required in
> other parts of plasma desktop, so I might have introduced some side effects.
> The kmix problem is definitely solved though.

I'm testing this under KDE 4.10.4 on gentoo. I applied your patch to plasma-runtime. Now both kmix and amarok are working as expected. I can see no side effects at first glance.

Please consider this patch to be integrated. The problem is not critical, but very annoying :).

Thank you again
Comment 14 Rex Dieter 2013-06-29 19:20:36 UTC
I can also vouche the proposed patch here works as advertised (thanks)
Comment 15 Christoph Feck 2013-07-20 12:34:07 UTC
Please add the patch to https://git.reviewboard.kde.org/ so that Plasma developers can review it for inclusion.
Comment 16 Andrew 2013-07-20 19:15:29 UTC
Seems fixed for me with KDE 4.10.5 at Fedora 19 x64.
Comment 17 Rex Dieter 2013-07-20 19:37:52 UTC
true, fedora's kde-runtime-4.10.5-3+ builds include the proposed patch from comment #11
Comment 18 Eike Hein 2013-09-03 19:08:55 UTC
naur, I've discussed and reviewed this patch together with Marco Martin as I was fixing other bugs in MouseEventListener, and we'd like to merge it upstream. Could you please re-submit it via ReviewBoard, so we get a proper real name for the submission?
Comment 19 Christoph Feck 2013-10-02 23:41:47 UTC
Could you please check comment #18? To get your commit into KDE repositories, we need proper attribution.
Comment 20 Eike Hein 2013-10-02 23:47:52 UTC
I've since explained the problem to Nicolás Alvarez and he has written a fix for this independently, without having seen the original patch, i.e. we should have a committable cleanroom implementation now.
Comment 21 Maximiliano Curia 2013-10-30 12:21:22 UTC
(In reply to comment #20)
> I've since explained the problem to Nicolás Alvarez and he has written a fix
> for this independently, without having seen the original patch, i.e. we
> should have a committable cleanroom implementation now.

So, is this patch commited already? Can we close this bug?
If not, could please add a link to the reviewboard?

Thanks,
Comment 22 Eike Hein 2013-10-30 16:15:40 UTC
Unfortunately that effort stalled because we realized the bug is actually "unfixable" in the sense that the fix avoids this bug, but causes others, because it's fundamentally based on comparing and discarding mouse events by their pointer values, which aren't good-enough unique ids because Qt can sometimes allocate new events on the stack in the exact same location, making them indistinguishable. QEvents sadly have no monotonic timestamp or similar. We could commit it as a lesser evil of sorts, but basically MouseEventListener is fundamentally broken.
Comment 23 Enrico Tagliavini 2013-10-31 09:25:23 UTC
Hi Eike. It is since that patch was attached to this bug that I apply it to my KDE build. In short it is about 6 month I'm using it. 2 days ago I've adapted the patch to KDE 4.11.2 and built it. I run this Desktop no less then 10 hours per day (weekend included) since it is both my work laptop and my personal laptop. In 6 months I never had a glitch with mouse events related to the system tray. While I understand when you say this fix introduces other problems due to address collision, but in practise this seems to be unlikely. This is just my personal experience so YMMV.

That said I think it is very worth including the fix to mitigate the issue for KDE 4.x and accept the idea it is broken. A proper solution can be developed for KDE 5. In the meantime users are not affected by the issue. Keep in mind, while this is not critical, it is extremely annoying and affects almost all mouse wheel + system tray users.

Thank you :)
Comment 24 Maximiliano Curia 2013-10-31 14:14:18 UTC
Well the patch compares the pointers received, which could be argued that are rarely the same between calls if it's not an error. Anyway, I don't get how is that the MouseEventListener is installed three times, is that a bug in plasma-widget or am I missing something else?
Comment 25 Nicolás Alvarez 2013-10-31 17:19:14 UTC
The event objects are usually created as local variables on the stack. They will have the same address whenever the same code path is followed. It's not rare or unlikely, in my tests every click had a mouse click event at the same address in memory.

MouseEventListener is not installed three times. It handles events for all its children, and the item hierarchy in the tray happens to have three items there.
Comment 26 Nicolás Alvarez 2013-11-18 23:39:43 UTC
Git commit d8089973bd83b7297ee67765b88ad56094a36adb by Nicolás Alvarez.
Committed on 06/10/2013 at 19:35.
Pushed by nalvarez into branch 'KDE/4.11'.

MouseEventListener: Don't process event from children multiple times.

If an event passed through multiple visually-overlapping children of
MouseEventListener, the event filter was processing the event every time.
For example, this caused problems for the KMix icon in the tray, which
would trigger volume changes three times for every mouse wheel event.

M  +5    -0    plasma/declarativeimports/qtextracomponents/mouseeventlistener.cpp

http://commits.kde.org/kde-runtime/d8089973bd83b7297ee67765b88ad56094a36adb
Comment 27 Enrico Tagliavini 2013-11-19 09:11:32 UTC
Thank you very very much!
Comment 28 Simonas 2013-12-14 10:12:52 UTC
The only regression i noticed is that now scrolling step in add widgets dialog is three times smaller, other than that, its fine