--===============2625670205355741634== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > 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 oth= er whenever it is triggered". In other words, both actions/states have an e= qual status, so neither is necessarily "active" as compared to the other on= e? I suggest renaming the states (in the apidox and code) - perhaps simply = use 'state1' and 'state2' instead. > = > Aur=C3=A9lien G=C3=A2teau 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 possibl= e to use bool in signals. If we go for state1 and state2, then how do we na= me the signals? stateChanged(bool) would be weird, and stateChanged(int) wo= uld be error prone and not handy because it would not be possible to connec= t 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 st= ate1 and state2, I am all ears! > = > David Jarvie wrote: > I can understand your comments about using a bool parameter - the pro= blem 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 bet= ter to design a good API and then change existing code to fit with it, rath= er 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 i= mposing a precedence on them is really appropriate. Perhaps using an enum w= ould be better than a bool or int. > = > Another idea for the two action names might be 'default' and 'alterna= tive' (apart from 'default' being a keyword, of course). That might get rou= nd the equal status issue, but still allow bool to be used. > = > Aur=C3=A9lien G=C3=A2teau wrote: > I have to disagree with not using a bool: developers used KToggleActi= on with setCheckedState() despite the fact that it looked wrong because it = was convenient. If KDualAction is not at least as easy to use as KToggleAct= ion 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 na= turally 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=C3=A9lien G=C3=A2teau 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 sol= utions: > = > 1. Introduce an enum State { Inactive, Active } in the class and chan= ge the setters and getters to use it instead of bool. setTextForState() sig= nature would for example be turned into setTextForState(KDualAction::State,= const QString&). > = > 2. Rework the setters and getters to be more explicit: replace setTex= tForState() with setTextForActiveState() and setTextForInactiveState(). Sam= e 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 Acti= ve 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, ther= e 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 necess= arily one being subordinate to the other. I would not recommend developers to use this class for unrelated actions, t= he actions represented by this class should oppose in some way. I did some research on existing code based on the usage of KToggleAction::s= etCheckedState() ( http://lxr.kde.org/ident?i=3DsetCheckedState ). I went t= hrough all the references listed and came up with the following use cases: - Show/hide UI elements kmag, konqueror, kaddressbook, knode, umbrello, kiconedit, kuickshow, kar= bon, 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 =3D=3D overwrite, inactive =3D=3D 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 va= lue set by the constructor for all instances of KDualAction or as the defau= lt value set by the application for this particular instance of KDualAction. - Aur=C3=A9lien ----------------------------------------------------------- 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=C3=A9lien G=C3=A2teau 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 whi= ch represents two actions, switching from one to the other whenever it is t= riggered. KDualAction can be used to implement actions such as the Play/Pau= se 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::setCheck= edState(). 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 add= ition 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 th= is 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 (althoug= h it's a bit more involved in this case because the close action changes de= pending 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 m= isuse: http://lxr.kde.org/ident?i=3DsetCheckedState ) > = > = > 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 D= ragon, Konqueror and creating a showHideMenubar action in KStandardAction (= review requests to come if the class is accepted). > = > = > Thanks, > = > Aur=C3=A9lien > = > --===============2625670205355741634== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://svn.reviewb= oard.kde.org/r/5396/

On September 23rd, 2010, 4:11 p.m., David J= arvie wrote:

Labelling=
 the states "active" and "inactive" seems misleading, a=
nd not necessarily intuitive to use. The summary of the purpose of the clas=
s says it's "an action which represents two actions, switching fro=
m one to the other whenever it is triggered". In other words, both act=
ions/states have an equal status, so neither is necessarily "active&qu=
ot; as compared to the other one? I suggest renaming the states (in the api=
dox and code) - perhaps simply use 'state1' and 'state2' in=
stead.

On September 23rd, 2010, 8:07 p.m., Aur=C3=A9lien G=C3=A2teau wr= ote:

What I li=
ke about the "active" and "inactive" terms is that they=
 map quite well to boolean values. This is important because it makes it po=
ssible 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(in=
t) would be error prone and not handy because it would not be possible to c=
onnect to existing slots which accept a bool parameter.

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

On September 24th, 2010, 12:05 p.m., David Jarvie wrote:

I can und=
erstand your comments about using a bool parameter - the problem with a boo=
l is that it's not really able to intuitively represent two equal statu=
s items. But I don't think the argument about needing to connect to exi=
sting 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 i=
s to provide two alternative but otherwise equal actions, so I don't th=
ink imposing a precedence on them is really appropriate. Perhaps using an e=
num would be better than a bool or int.

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

On September 24th, 2010, 9:08 p.m., Aur=C3=A9lien G=C3=A2teau wr= ote:

I have to=
 disagree with not using a bool: developers used KToggleAction with setChec=
kedState() 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 actio=
n 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.

On September 24th, 2010, 10:26 p.m., David Jarvie wrote:

I don'=
;t object to using a bool if it has a reasonably natural meaning in the con=
text where it's used.

When I suggested the possible use of "default"/"alternative&=
quot;, I meant that the "default" action would be the one which w=
as initially set as the selected action in the constructor, or if the defau=
lt constructor was used, the action which was set later corresponding to tr=
ue.

On September 26th, 2010, 7:16 a.m., Aur=C3=A9lien G=C3=A2teau wr= ote:

I think t=
he 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 setTextForSt=
ate() with setTextForActiveState() and setTextForInactiveState(). Same thin=
g 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?

On September 26th, 2010, 12:22 p.m., David Jarvie wrote:

I would f=
avour the first option, since it allows more flexibility in use of the clas=
s. 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) on=
e 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 o=
ne being subordinate to the other.
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::s=
etCheckedState() ( http://lxr.kde.org/ident?i=3DsetCheckedState ). I went t=
hrough all the references listed and came up with the following use cases:

- Show/hide UI elements
  kmag, konqueror, kaddressbook, knode, umbrello, kiconedit, kuickshow, kar=
bon, 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 =3D=3D overwrite, inactive =3D=3D 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 K=
DualAction.

- Aur=C3=A9lien


On September 23rd, 2010, 2:29 p.m., Aur=C3=A9lien G=C3=A2teau wrote:

Review request for kdelibs.
By Aur=C3=A9lien G=C3=A2teau.

Updated 2010-09-23 14:29:29

Descripti= on 

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 whe=
never 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-st=
ate actions: They set the first action as the unchecked state and provide a=
n alternative KGuiItem for the checked state with KToggleAction::setChecked=
State(). This is wrong because when the user clicks a button representing t=
he action in a toolbar, the button stays down.  The appropriate use cases f=
or toggle buttons (and thus KToggleAction) are documented in a recent addit=
ion 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 mis=
use: http://lxr.kde.org/ident?i=3DsetCheckedState )

Testing <= /h1> 
The class comes with unit-tests. I tested the API made sense=
 by porting Dragon, Konqueror and creating a showHideMenubar action in KSta=
ndardAction (review requests to come if the class is accepted).

Diffs=

  • trunk/KDE/kdelibs/kdeui/CMakeLists.txt (11= 71068)
  • 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)

View Diff
--===============2625670205355741634==--