KActionCollection setAssociatedWidget problems

[Andreas Hartmetz]

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 :/

> Cheers,
> Hamish.