Review Request: New action class proposal: KDualAction

[Aurélien Gâteau]

> On 2010-10-13 22:53:42, Christoph Feck wrote:
> > trunk/KDE/kdelibs/kdeui/actions/kdualaction.h, line 147
> > <http://svn.reviewboard.kde.org/r/5396/diff/3/?file=39188#file39188line147>
> >
> >     Shouldn't this be "setCurrentState(State state)" after your bool->enum conversion?

While I agree that setTextForState(true, i18n("Hide Sidebar")) is less intuitive than setTextForState(KDualAction::ActiveState, i18n("Hide Sidebar")), I think setActive(true) is as intuitive as setState(KDualAction::ActiveState). It also has the advantage of being easier to use as a slot, since it uses a bool instead of a class-specific enum.

For this reason I would like to keep setActive(bool). For completeness I could add setState(KDualAction::State), but it feels redundant. An alternative solution is to get rid of the enum and create explicit setter and getter methods. That is, replacing setTextForState(KDualAction::State,  QString) with setTextForInactiveState(QString) and setTextForActiveState(QString). 


On 2010-10-13 22:53:42, Aurélien Gâteau wrote:
> > Additionally, please add a KDualAction camelcased header. Thanks.

Just added it.


- Aurélien


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


On 2010-10-12 15:32:08, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5396/
> -----------------------------------------------------------
> 
> (Updated 2010-10-12 15:32:08)
> 
> 
> 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 1180870 
>   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 1180870 
>   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/20101014/40a76768/attachment.htm>