This is an automatically generated e-mail. To reply, visit: http://svn.reviewb= oard.kde.org/r/5237/

On September 3rd, 2010, 1:30 a.m., Christop= h Feck wrote:

Some ques=
tions from my side:

* A tool button would need to make sure that the widest of the text labels =
is used as the size hint, so that the tool bar does not need to re-layout o=
n state changes. Does your code allow that?

* Does it make sense to hard-code the limit to two states? What about maybe=
 "KMultiAction", which also allows a third or fourth action if th=
e programmer has a use case? Or allow something which uses QState? KStateAc=
tion?

* Do we need the "First" and "Second" enums? I would ju=
st use an "int" for the state. The programmer probably wants his =
own descriptive names, such as

    enum State {
        Stopped =3D 0,
        Playing =3D 1
    };

and I think numbers are just more readable here.

Other than that, an action which has "more" state than just "=
;On/Off" would make a useful addition to kdeui.

> A tool=
 button would need to make sure that the widest of the text labels is used =
as the size hint, so that the tool bar does not need to re-layout on state =
changes. Does your code allow that?

No. As far as I know there is no way for a QAction to influence the way it =
is rendered by widgets. Note that using KToggleAction + KToggleAction::setC=
heckedState() suffers from the same problem.

> Does it make sense to hard-code the limit to two states? What about ma=
ybe "KMultiAction", which also allows a third or fourth action if=
 the programmer has a use case? Or allow something which uses QState? KStat=
eAction?

I am a bit reluctant to go this way because I don't think it would be g=
ood for usability: having to click repeatedly on a button to reach the want=
ed state sounds painful, better use a combobox or a set of toggle buttons i=
n this case.

I had a discussion with a developer of Rekonq (hi Lionel!) and he made me r=
ealize KDualAction should use real QAction instead of KGuiItem, reasons beh=
ind this was the ability to have separate shortcuts for each action, and en=
suring that KDualAction can notice when one of the actions is triggered so =
that it updates itself (example: user press F5 in Rekonq, this triggers the=
 "reload" action, the "reload/stop" KDualAction should =
change to show the "stop" action). Therefore I am going to rework=
 the class this way. This should remove the need for enums. I may discard t=
his request and reopen a new one if it takes too much time.

- Aur=C3=A9lien

On September 2nd, 2010, 10:17 p.m., Aur=C3=A9lien G=C3=A2teau wrote:

Review request for kdelibs.

By Aur=C3=A9lien G=C3=A2teau.

Updated 2010-09-02 22:17:33

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)

- <insert-your-app-here>

I already have a patch to make Dragon use KDualAction (I used Dragon as a t=
est application) and I plan to port other applications if the class is acce=
pted in kdelibs (I also plan to fix some applications which mis-use KToggle=
Action so that they don't use setCheckedState() when it is not appropri=
ate).

Testing <= /h1>

The class comes with unit-tests. I tested the API made sense=
 while porting Dragon to it.

Diffs=

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)
trunk/KDE/kdelibs/kdeui/actions/kdualaction.cpp (PRE-CREATION)
trunk/KDE/kdelibs/kdeui/CMakeLists.txt (11= 71068)
trunk/KDE/kdelibs/kdeui/actions/kdualaction.h (PRE-CREATION)

View Diff

Descripti= on

Testing <= /h1> The class comes with unit-tests. I tested the API made sense= while porting Dragon to it.

Diffs=

Testing <= /h1>

The class comes with unit-tests. I tested the API made sense= while porting Dragon to it.