KActionCollection setAssociatedWidget problems

[David Faure]

On Sunday 21 October 2007, Andreas Hartmetz wrote:
> Am Sonntag, 21. Oktober 2007 17:31:43 schrieb Hamish Rodda:
> > On Mon, 22 Oct 2007 12:39:00 am Andreas Hartmetz wrote:
> > > Am Sonntag, 21. Oktober 2007 16:01:02 schrieb Hamish Rodda:
> > > > Hi,
> > > >
> > > > A while back there was a cleanup of KActionCollection which subtly
> > > > changed the behaviour of setAssociatedWidget(QWidget*).  This function
> > > > is intended to make the actions specific to that widget, and
> > > > accomplishes this by adding all actions to the widget and setting their
> > > > shortcut context to Qt::WidgetShortcut. (It replaces KAccel's widget
> > > > binding functionality)
> > > >
> > > > The problem is that in the cleanup, the mechanism to ensure any further
> > > > actions were added to the widget was kept, but the mechanism to ensure
> > > > the shortcut context got changed to Qt::WidgetShortcut was deleted. 
> > > > Now, the only shortcuts which have their context changed are those
> > > > which are already a part of the action collection when
> > > > setAssociatedWidget is called.
> > >
> > > See below...
> > >
> > > > This has created a set of serious bugs: when parts are merged together,
> > > > actions' shortcuts step on each other's toes if the setAssociatedWidget
> > > > call was prior to their addition to the action collection.  For
> > > > instance, KDirOperator's delete shortcut (a WindowShortcut) competes
> > > > with katepart's delete shortcut, thus delete doesn't work when you're
> > > > typing in kate.
> > > >
> > > > While I agree that the magic of changing the shortcut context is not
> > > > ideal, it is the intention (or at least I hope it is) of every use of
> > > > setAssociatedWidget to conveniently have this effect.
> > >
> > > There was actually a complaint from an application programmer about
> > > the "magic" behavior. So that's why I removed it, but apparently not in
> > > all places.
> > >
> > > > I believe the best course of action is to reintroduce this behaviour
> > > > (one line patch attached).  The alternative really is to remove
> > > > setAssociatedShortcut and let the apps do it themselves, but I think
> > > > this would lead to more errors.
> > >
> > > IMHO collections that change their contents are a dangerous concept.
> > > There may be cases where it works, but generally it'll just give you
> > > problems. Imagine you are debugging an application and objects change but
> > > you have no idea why - because it's implicit and you can't know without
> > > reading the documentation of all classes involved. Even a breakpoint at
> > > the setter won't necessarily work because not everything goes through the
> > > setter - example: destroying and recreating an object changes its
> > > properties without calling any setters.
> >
> > Ok. Should we remove *AssociatedWidget from the api and do:
> >
> > for (KAction* action, yourActionCollection) {
> >    action->setShortcutContext(Qt::WidgetShortcut);
> >    widget->addAction(action);
> > }
> >
> > for every use of it?  Perhaps replace it with
> > KActionCollection::associateActionsWithWidget(QWidget*), which basically
> > does the above on a once-off only...
> >
> I'd agree with both but I also don't feel like deciding about something I have 
> never used as an application programmer...
> Any further opinions? Sorry, I know we've had this topic several times 
> already :/

I know that I fixed a bug in konqpopupmenu where a call to setAssociatedWidget
made all actions be plugged twice into the menu. [and this was really not obvious from the code]

Somehow, the automatic setting of the shortcut context would be good to have,
but the automatic addAction seems very confusing; especially since for popup menus
you usually add actions yourself, and for the rest we use XMLGUI -- or what do I miss here?

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).