Review Request: New action class proposal: KDualAction

[Aurélien Gâteau]

> On 2010-09-23 16:11:37, David Jarvie wrote:
> > Labelling the states "active" and "inactive" seems misleading, and not necessarily intuitive to use. The summary of the purpose of the class says it's "an action which represents two actions, switching from one to the other whenever it is triggered". In other words, both actions/states have an equal status, so neither is necessarily "active" as compared to the other one? I suggest renaming the states (in the apidox and code) - perhaps simply use 'state1' and 'state2' instead.

What I like about the "active" and "inactive" terms is that they map quite well to boolean values. This is important because it makes it possible to use bool in signals. If we go for state1 and state2, then how do we name the signals? stateChanged(bool) would be weird, and stateChanged(int) would be error prone and not handy because it would not be possible to connect to existing slots which accept a bool parameter.

Having said that, I agree these terms do not feel like the best match: if you have an idea for a signal name with a bool parameter when using state1 and state2, I am all ears!


- Aurélien


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


On 2010-09-23 14:29:29, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5396/
> -----------------------------------------------------------
> 
> (Updated 2010-09-23 14:29:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> 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)
> 
> - Any application which incorrectly uses KToggleAction + setCheckedState() to show/hide a UI element (a search on lxr.kde.org shows quite a lot of misuse: http://lxr.kde.org/ident?i=setCheckedState )
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1171068 
>   trunk/KDE/kdelibs/kdeui/actions/kdualaction.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/actions/kdualaction.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/actions/kdualaction_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/tests/CMakeLists.txt 1171068 
>   trunk/KDE/kdelibs/kdeui/tests/kdualactiontest.cpp PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5396/diff
> 
> 
> Testing
> -------
> 
> The class comes with unit-tests. I tested the API made sense by porting Dragon, Konqueror and creating a showHideMenubar action in KStandardAction (review requests to come if the class is accepted).
> 
> 
> Thanks,
> 
> Aurélien
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100923/dc761f59/attachment.htm>