Review Request: New action class proposal: KDualAction

David Jarvie djarvie at kde.org
Fri Sep 24 13:05:44 BST 2010



> 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.
> 
> Aurélien Gâteau wrote:
>     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!

I can understand your comments about using a bool parameter - the problem with a bool is that it's not really able to intuitively represent two equal status items. But I don't think the argument about needing to connect to existing slots which take a bool parameter is a good one. IMO, it's better to design a good API and then change existing code to fit with it, rather than to make the API of a new class adhere to existing code when there's no need for source or binary compatibility. The main idea of this class is to provide two alternative but otherwise equal actions, so I don't think imposing a precedence on them is really appropriate. Perhaps using an enum would be better than a bool or int.

Another idea for the two action names might be 'default' and 'alternative' (apart from 'default' being a keyword, of course). That might get round the equal status issue, but still allow bool to be used.


- David


-----------------------------------------------------------
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/20100924/928e610a/attachment.htm>


More information about the kde-core-devel mailing list