Bug in KAction::initPrivate

[Simon Hausmann]

On Tue, Sep 24, 2002 at 10:25:23PM +0200, Martijn Klingens wrote:
> On Tuesday 24 September 2002 19:59, Michael Brade wrote:
> > revision 1.278
> > date: 2002/07/01 21:30:38;  author: hausmann;  state: Exp;  lines: +0 -6
> > @@ -1923,8 +1923,6 @@ KListAction::KListAction( const QString&
> >
> >  : KSelectAction( text, cut, parent, name )
> >
> >  {
> >    d = new KListActionPrivate;
> > -  if ( receiver )
> > -    connect( this, SIGNAL(activated(int)), receiver, slot );
> > ----
> >
> > So it wasn't your change, Ellis, but Simons. Simon, any explanation for
> > breaking my app? ;-))
> 
> Yeah... Further up the inheritance chain KAction also connects to receiver. 
> The result is that the selected signal is fired off twice, resulting in very 
> weird reactions of the code.

I second that :) . The old behaviour of double-connecting under
certain (for the developer not obvious) circumstances was clearly a
bug IMHO.

> One way to solve this is to connect to receiver if and only if receiver takes 
> an int, and in that case NOT pas receiver to parent's ctor.
> 
> More clean to me seems to not pass the receiver to the parent anyway and do 
> all the connecting in KListAction's ctor. Even then we need to detect whether 
> the passed receiver takes a parameter or not though.

That sounds like code duplication involved..

IMHO this is a case of a too-magic API. The only similar case that I
can think of is QSignal, which does a similar matching for an
integer signature in the slot, but it is not bound to another
completely unrelated property, like KAction is with the object name.

(ah, I just see that this is apparently a change that is new to KDE
3.1, so we could still revert it if we find a better solution, or?)

Looking at the code I actually don't see a reason for it. I might be
missing something (please correct me if wrong), but isn't the
functionality that KAction tries to do in magic (and apparently
fails under certain conditions) exactly what QSignalMapper solves as
separate class, even more flexible (allows mapping using a string in
addition to the int mapping) and well-documented?

I mean, just from reading code it's more obvious to me what

QSignalMapper *desktopNumberMapper = new QSignalMapper( this );

connect( desktopNumberMapper, SIGNAL( mapped( int ) ),
         this, SLOT( moveToDesktop( int ) ) );

moveToDesktopAction = new KAction( .., desktopNumberMapper, SLOT( map() ),
                                   actionCollection() );

desktopNumberMapper->setMapping( moveToDesktopAction, 1 );

does, than

foob = new KAction( ..., this, SLOT( sendToDesktop( int ),
                    actionCollection(), "sendToDesktop{1}" ) 

Yes, the first example is more lines of code, but it's more readable
IMHO and it's easier to analyze if you see the code for the first
time. Especially as QSignalMapper has been introduced to exactly to
solve problems like these, so the developer might already know about
it. Whereas with the second example one has to read in the
kaction.cpp sources that a special signature with an encoded number
does the magic thing if provided in the Qt object name (something
that is completely unrelated to the activation of actions) .

Unless I completely miss the point I vote for reverting the change
to the KDE 3.0 API. I'd be happy to take those steps and fix the
applications relying on the magic object-name{42} behaviour, in case
there is an agreement.


Simon