RFC: An action class to ease implementation of show/hide-like actions

Aurélien Gâteau agateau at kde.org
Sun Sep 19 08:08:32 BST 2010


On 18/09/2010 12:33, Ingo Klöcker wrote:
> On Saturday 18 September 2010, Aurélien Gâteau wrote:
>> On 18/09/2010 00:06, Ingo Klöcker wrote:
>>> On Friday 17 September 2010, Aurélien Gâteau wrote:
>>>> On 10/09/2010 23:20, Aurélien Gâteau wrote:
>>>> class KDEUI_EXPORT KDualAction : public KAction
>>>> {
>>>>
>>>>     Q_OBJECT
>>>>
>>>> public:
>>>>     KDualAction(const QString &offActionText, QObject *parent);
>>>
>>> I don't understand why one can set one of the texts via the c'tor,
>>> but not the other one. Am I missing something or did you forget
>>> something?
>>>
>>> :-)
>>
>> I created this constructor to make it as easy as possible to replace
>> a KToggleAction with a KDualAction. Since KToggleAction has a
>> constructor which takes the offActionText, I created one as well.
>> Maybe it should be changed to KDualAction(offText, onText, parent).
>> What do you think of this?
> 
> Makes more sense since a dual action will probably always have two 
> different texts. (Otherwise, one wouldn't use it.)

Indeed.
> 
> I'd revert the two texts, but that's probably just me.

What do you mean?

> Since you use 
> active everywhere else, I'd use inactiveText and activeText instead of 
> on/offText.

Agreed, I missed those when I renamed on/off to active/inactive.
 
>>>>     bool isActive() const;
>>>>     void setActive(bool);
>>>>     void silentSetActive(bool);
>>>
>>> I suppose this is supposed to set the active state without emitting
>>> the corresponding signal, i.e. it is equivalent to
>>>
>>>   dualAction->blockSignals( true );
>>>   dualAction->setActive( true/false );
>>>   dualAction->blockSignals( false );
>>
>> It is roughly equivalent: it does not have to block signals, it just
>> does not emit them.
>>
>>> IMHO something like a silentSetActive() (or using blockSignals())
>>> is evil because the caller can never know whether there are other
>>> listeners which rely on the signal to be emitted. Therefore, I
>>> strongly suggest removing this method from the class's interface.
>>
>> I think it is useful to have this method for situations where the
>> state of the element represented by the action has changed and you
>> need to update the action to reflect the new state.
>>
>> A concrete example is the play/pause action of a media player: While
>> playing the action text is "Pause". When the played song stops, the
>> action text must be set to "Play".
>>
>> Having it as a single method is interesting because:
>> - It avoids the use of blockSignals(), which can be tricky, the
>> example you give is not completely correct, it should be something
>> like this:
>>
>>     bool blocked = dualAction->blockSignals( true );
>>     dualAction->setActive( true/false );
>>     dualAction->blockSignals( blocked );
>>
>> - The method can be turned into a slot and directly connected to the
>> object it represents.
>>
>> Still I agree it is mostly there for developer convenience, so if you
>> feel strongly against it, it can be removed.
> 
> I'd prefer it being removed.

Would you prefer Matthew solution (two different signals?)

Aurélien




More information about the kde-core-devel mailing list