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

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

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

-----------------------------------------------------------
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
>
>

On Sunday 08 November 2009, Frank Reininghaus wrote:

> -----------------------------------------------------------
> 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.

Maybe a static variable would be enough? AFAIU it's about storing
something between press and release, not storing something for
the longer term.

--
David Faure, faure@..., http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. Konqueror
(http://www.konqueror.org).