[PATCH] Re: KDE/kdelibs/kdeui/xmlgui

[Michael Jansen]

On Saturday 03 January 2009 00:10:03 Michael Jansen wrote:
> > Please try the attached patch.
> > It tries to solve the problem by just passing 0 as parent to
> > KStandardAction::create(). As with all action classes which are created
> > by KStandardAction::create() the parent seems just to be used only for
> > Qt's garbage control. So this should be theoretically no problem. With 0
> > as parent KStandardAction::create() now does not add the new action to
> > the collection itself. And practically I found no problem, too, e.g. with
> > Okular, Konqueror and Konsole. And Okteta :P
> >
> > So if this patch works for you all, it should be committed and your
> > ui_standards.rc commit, Urs, can be reverted :)
>
> I have no objections to the patch if it solves the problem. My problem is
> WHY does it solve the problem?
>
> if you look at kactioncollection.cpp:264/287/283 you will find code trying
> to prevent the problem you described. In this case line 283 is relevant.
>
>     // Check if we have this action under a different name.
>     // Not using takeAction because we don't want to remove it from
> categories,
>     // and because it has the new name already.
>     const int oldIndex = d->actions.indexOf(action);
>     if (oldIndex != -1) {
>         d->actionByName.remove(objectName);
>         d->actions.removeAt(oldIndex);
>     }
>
> So the problem you guys seem to have with having the SAME action with two
> names in the action collection should not happen. Are you guys sure it's
> the SAME action?

OK. I got it. See http://websvn.kde.org/?view=rev&revision=904755 . I've added 
a unit test and changed your comments a bit.

The problem was introduced by me. It should be absolutely impossible to get 
the same action twice into a collection. I introduced the setObjectName() call  
in this KActionCollection::addAction(KStandardAction..) method which exposed a 
flaw in our logic trying to prevent double additions.

Mike


-- 
Michael Jansen

http://www.michael-jansen.biz