Bug in KAction::initPrivate

[Simon Hausmann]

On Thu, Sep 26, 2002 at 09:41:13PM +0200, Andreas Beckermann wrote:
> On Thursday 26 September 2002 09:37, Ellis Whitehead wrote:
> > Hi Simon,
> >
> > On Wednesday 25 September 2002 00:47, Simon Hausmann wrote:
> > > ...
> > > 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?)
> 
> Well - *if* you do it *please* do it asap. I'll have to change boson's code, 
> too or I'll have to tell users to use KDE >= 3.0.1 but <= 3.1 for boson 0.7. 

Ok, I'll make the change this weekend. I can make a patch for boson
(you're talking about the code in the SF repository, right?) if you
want, so that no work is invovled from your side.

Do you know of any code in the KDE CVS repository using the feature?
A grep in kdegames didn't reveal anything to me -- but I might have
missed something.

> > > 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 you are right - once you have found this solution this is more obvious. 
> But I for example was aware of QSignalMapper and even used it once but this I 
> didn't get to this solution. For most other developers it'll be very hard to 
> find the QSignalMapper class as it's a rarely used and very small class. So 

Time to spread the word :)

> an aboslute minimum would be to mention this possible solution somewhere in 
> the docs.

Excellent idea, will do that.

> > > 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. 
> 
> btw imho this is more readable:
> 
>  for (int i = 0; i < 10; i++) {
>      (void)new KAction(i18n("Select Group %1").arg(i == 0 ? 10 : i),-
>                        Qt::Key_0 + i, this,
>                        SLOT(slotSelectGroup(int)), actionCollection(),
>                        QString("select_group {%1}").arg(i));
>         (void)new KAction(i18n("Create Group %1").arg(i == 0 ? 10 : i),
>                        Qt::CTRL + Qt::Key_0 + i, this,
>                        SLOT(slotCreateGroup(int)), actionCollection(),
>                        QString("create_group {%1}").arg(i));
>  }
> 
> than that:
> 
> QSignalMapper* selectMapper = new QSignalMapper(this);
> QSignalMapper* createMapper = new QSignalMapper(this);
> connect(selectMapper, SIGNAL(mapped(int)), this, SLOT(slotSelectGroup(int));
> connect(createMapper, SIGNAL(mapped(int)), this, SLOT(slotCreateGroup(int));
>  for (int i = 0; i < 10; i++) {
>      KAction* a = new KAction(i18n("Select Group %1").arg(i == 0 ? 10 : i),-
>                        Qt::Key_0 + i, selectMapper,
>                        SLOT(map()), actionCollection(),
>                        QString("select_group_%1").arg(i));
>      selectMapper->setMapping(a, i);
>      a = new KAction(i18n("Create Group %1").arg(i == 0 ? 10 : i),
>                        Qt::CTRL + Qt::Key_0 + i, createMapper,
>                        SLOT(map()), actionCollection(),
>                        QString("create_group_%1").arg(i));
>      createMapper->setMapping(a, i);
>  }

Opinions differ :) BTW, in the second case you don't need the
special action names anymore :)

What makes me think the second example is more readable is because I
for one associate the activated() signal with kaction (consistent
and exchangable with QAction, btw) , and seeing there is
QSignalMapper involved I immediately know that this is about mapping
signal emissions from multiple objects to one slot. Spotting the
setMapping calls the binding to the argument is IMHO more obvious
than finding it in a QString( "blah_{%1}" ).arg( i ) call, at a
place I seldom look at :)

(but is probably because I don't like constructor calls with tons of
arugments but prefer smaller ones followed by obvious setFoo calls,
where you immediately see what the code is about, instead of
requiring to look in the class doc to check what the 5th argument
was about again)

> more readable and is more comfortable to an application programmer. But I'm 
> fine with the QSignalMapper solution as long as
> a) it happens now (i.e. not in two months or so)

Yep.

> b) someone changes http://edu.kde.org/developer/kaction.phtml which covers 
> exactly those problems as they are very common to games (and obviously for 
> edu, too as the doc was catched by them)

ACK, good you mention that. Will adapt.

Simon