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

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