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

[Ingo Klöcker]

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.)

I'd revert the two texts, but that's probably just me. Since you use 
active everywhere else, I'd use inactiveText and activeText instead of 
on/offText.


> >>     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.


Regards,
Ingo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100918/f2e0ec60/attachment.sig>