Review Request: New action class proposal: KDualAction

[David Jarvie]

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


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.


trunk/KDE/kdelibs/kdeui/actions/kdualaction.h
<http://svn.reviewboard.kde.org/r/5396/#comment7852>

    Say which state is initially set by the constructor



trunk/KDE/kdelibs/kdeui/actions/kdualaction.h
<http://svn.reviewboard.kde.org/r/5396/#comment7853>

    Say which state is initially set by the constructor



trunk/KDE/kdelibs/kdeui/actions/kdualaction.h
<http://svn.reviewboard.kde.org/r/5396/#comment7854>

    Say which state the two values of 'bool state' represent - presumably true = active, false = inactive:
    
        @param state true for the active state, false for the inactive state
    
    This comment applies to all methods taking a 'bool state' parameter.


- David


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/f40101a9/attachment.htm>