Review Request: New action class proposal: KDualAction

David Jarvie djarvie at kde.org
Tue Sep 28 12:19:27 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!
> 
> David Jarvie wrote:
>     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.
> 
> Aurélien Gâteau wrote:
>     I have to disagree with not using a bool: developers used KToggleAction with setCheckedState() despite the fact that it looked wrong because it was convenient. If KDualAction is not at least as easy to use as KToggleAction it won't be used.
>     
>     I am not sure about "default/alternative". When you use the action to show/hide a sidebar, which one is "default" and which one is "alternative"? and how does it map to true/false? I think "active/inactive" maps more naturally to true/false.
> 
> David Jarvie wrote:
>     I don't object to using a bool if it has a reasonably natural meaning in the context where it's used.
>     
>     When I suggested the possible use of "default"/"alternative", I meant that the "default" action would be the one which was initially set as the selected action in the constructor, or if the default constructor was used, the action which was set later corresponding to true.
> 
> Aurélien Gâteau wrote:
>     I think the part of the API where the bool is unclear is the setters and getters for the icon/label/tooltip. So I thought about two possible solutions:
>     
>     1. Introduce an enum State { Inactive, Active } in the class and change the setters and getters to use it instead of bool. setTextForState() signature would for example be turned into setTextForState(KDualAction::State, const QString&).
>     
>     2. Rework the setters and getters to be more explicit: replace setTextForState() with setTextForActiveState() and setTextForInactiveState(). Same thing for all setters and getters.
>     
>     In both solutions, I think the setActive() and isActive() methods can stay, as well as the activeChanged() and activeChangedByUser() signals.
>     
>     What do you think about this?
> 
> David Jarvie wrote:
>     I would favour the first option, since it allows more flexibility in use of the class. The only thing I still take issue with is the use of Active and Inactive to identify the two actions. I know that in some uses (e.g. Play/Pause) one action can be active and the other inactive. However, there will be other uses where that isn't true. The purpose of the new class is simply to provide two alternative actions, whose relationship isn't necessarily one being subordinate to the other.
> 
> Aurélien Gâteau wrote:
>     I would not recommend developers to use this class for unrelated actions, the actions represented by this class should oppose in some way.
>     
>     I did some research on existing code based on the usage of KToggleAction::setCheckedState() ( http://lxr.kde.org/ident?i=setCheckedState ). I went through all the references listed and came up with the following use cases:
>     
>     - Show/hide UI elements
>       kmag, konqueror, kaddressbook, knode, umbrello, kiconedit, kuickshow, karbon, krita, kword, libkoffice, klettres
>     
>     - Show/hide content
>       kuser, marble, knode, kate, kbugbuster, kpovmodeler, krita
>     
>     - Lock/unlock (or read-only/read-write)
>       knotes, okteta, superkaramba
>     
>     - Mark items as important/normal, mark items as action items/normal items
>       akregator, kmail
>     
>     - Enable/Disable remote control
>       krfb
>     
>     - Encrypt message
>       kmail
>     
>     - Insert/overwrite
>       okteta
>     
>     - Ignore/unignore
>       konversation
>     
>     Of all these, only Insert/overwrite is difficult to map to active/inactive. In this case, since the code is in a class named OverwriteModeController, I would say active == overwrite, inactive == insert.
>     
>     I think it is better to use Active/Inactive instead of Default/Alternative because "default" can be ambiguous: it can be interpreted as the default value set by the constructor for all instances of KDualAction or as the default value set by the application for this particular instance of KDualAction.

OK, you've convinced me.


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

More information about the kde-core-devel mailing list