<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5237/">http://svn.reviewboard.kde.org/r/5237/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 3rd, 2010, 1:30 a.m., <b>Christoph Feck</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Some questions from my side:
* A tool button would need to make sure that the widest of the text labels is used as the size hint, so that the tool bar does not need to re-layout on state changes. Does your code allow that?
* Does it make sense to hard-code the limit to two states? What about maybe "KMultiAction", which also allows a third or fourth action if the programmer has a use case? Or allow something which uses QState? KStateAction?
* Do we need the "First" and "Second" enums? I would just use an "int" for the state. The programmer probably wants his own descriptive names, such as
enum State {
Stopped = 0,
Playing = 1
};
and I think numbers are just more readable here.
Other than that, an action which has "more" state than just "On/Off" would make a useful addition to kdeui.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> A tool button would need to make sure that the widest of the text labels is used as the size hint, so that the tool bar does not need to re-layout on state changes. Does your code allow that?
No. As far as I know there is no way for a QAction to influence the way it is rendered by widgets. Note that using KToggleAction + KToggleAction::setCheckedState() suffers from the same problem.
> Does it make sense to hard-code the limit to two states? What about maybe "KMultiAction", which also allows a third or fourth action if the programmer has a use case? Or allow something which uses QState? KStateAction?
I am a bit reluctant to go this way because I don't think it would be good for usability: having to click repeatedly on a button to reach the wanted state sounds painful, better use a combobox or a set of toggle buttons in this case.
I had a discussion with a developer of Rekonq (hi Lionel!) and he made me realize KDualAction should use real QAction instead of KGuiItem, reasons behind this was the ability to have separate shortcuts for each action, and ensuring that KDualAction can notice when one of the actions is triggered so that it updates itself (example: user press F5 in Rekonq, this triggers the "reload" action, the "reload/stop" KDualAction should change to show the "stop" action). Therefore I am going to rework the class this way. This should remove the need for enums. I may discard this request and reopen a new one if it takes too much time.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On September 2nd, 2010, 10:17 p.m., Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated 2010-09-02 22:17:33</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review-request introduces a new class named KDualAction. The goal of this class is to make it easy to create a dual-state action: an action which represents two actions, switching from one to the other whenever it is triggered. KDualAction can be used to implement actions such as the Play/Pause action of a media player or the Reload/Stop action of a web browser.
Right now some applications mis-use KToggleAction to implement such dual-state actions: They set the first action as the unchecked state and provide an alternative KGuiItem for the checked state with KToggleAction::setCheckedState(). This is wrong because when the user clicks a button representing the action in a toolbar, the button stays down. The appropriate use cases for toggle buttons (and thus KToggleAction) are documented in a recent addition to the HIG:
http://techbase.kde.org/Projects/Usability/HIG/Toggle_Buttons
Potential users for this class:
- Dragon, Juk, Amarok to implement their Play/Pause action.
- Rekonq to implement its Reload/Stop action. Konqueror could also use this but it does not feature a dual reload/stop action as far as I know.
- Dolphin could maybe use it to implement its Split/Close action (although it's a bit more involved in this case because the close action changes depending on which panel it is going to close)
- <insert-your-app-here>
I already have a patch to make Dragon use KDualAction (I used Dragon as a test application) and I plan to port other applications if the class is accepted in kdelibs (I also plan to fix some applications which mis-use KToggleAction so that they don't use setCheckedState() when it is not appropriate).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The class comes with unit-tests. I tested the API made sense while porting Dragon to it.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/KDE/kdelibs/kdeui/actions/kdualaction_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/tests/CMakeLists.txt <span style="color: grey">(1171068)</span></li>
<li>trunk/KDE/kdelibs/kdeui/tests/kdualactiontest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/actions/kdualaction.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/CMakeLists.txt <span style="color: grey">(1171068)</span></li>
<li>trunk/KDE/kdelibs/kdeui/actions/kdualaction.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5237/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>