Review Request: KToolbar: Do not allow the user to middle-click disabled buttons

[Frank Reininghaus]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1914/#review2986
-----------------------------------------------------------


I have committed the changes to KToolBar and the unit test (slightly modified) to trunk in r1046431.

I left KActionPrivate::slotTriggered() as it is because my proposed change would make the "buttons" argument of the triggered(Qt::MouseButtons, Qt::KeyboardModifiers) signal of KAction incorrect if the action is triggered using the keyboard. Ensuring that "buttons" is always correct would probably require to add a new member variable to KActionPrivate which keeps track of the pressed buttons, and maybe it doesn't make much sense to add that overhead to every single action just to fix an issue that hasn't caused any trouble so far.

- Frank


On 2009-10-19 20:20:55, Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1914/
> -----------------------------------------------------------
> 
> (Updated 2009-10-19 20:20:55)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> 1. I've noticed that one can middle-click disabled buttons in a KToolbar and that even the triggered(Qt::MouseButtons, Qt::KeyboardModifiers) signal of the corresponding action gets emitted. To try that, open Konqueror in "/" and middle-click the "Up" button. My proposed change to ktoolbar.cpp fixes this. It could maybe be done easier by checking if the action is enabled only in the first "if" statement after the "Handle MMB on toolbar buttons" comment, but this would still be broken in the case that an action is disabled between the "mouse press" and "mouse release" events.
> 
> I wanted to add a new unit test and noticed that there were no tests at all about clicking toolbar buttons, so I thought I could add tests for clicks of all possible buttons and combinations of Shift/Ctrl/Alt. This revealed another strage thing:
> 
> 2. If a toolbar button is clicked with the left mouse button, the action's triggered(Qt::MouseButtons buttons, Qt::KeyboardModifiers modifiers) signal is emitted, but "buttons" is Qt::NoButton. The reason is that KActionPrivate::slotTriggered() uses QApplication::mouseButtons() to determine buttons, but at that time, the button is released already. To make my tests (and probably most applications of that signal) work, this has to be replaced by Qt::LeftButton. However, this will lead to incorrect results if an action is invoked using the keyboard. I can't see an easy way to get the right result in both cases :-(
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/actions/kaction.cpp 1037643 
>   /trunk/KDE/kdelibs/kdeui/tests/ktoolbar_unittest.cpp 1037643 
>   /trunk/KDE/kdelibs/kdeui/widgets/ktoolbar.cpp 1037643 
> 
> Diff: http://reviewboard.kde.org/r/1914/diff
> 
> 
> Testing
> -------
> 
> Fixes the issue. Old and new unit tests pass.
> 
> 
> Thanks,
> 
> Frank
> 
>