KActionCollection setAssociatedWidget problems

[Hamish Rodda]

On Mon, 22 Oct 2007 06:59:06 pm David Faure wrote:
> 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?

This functionality is usually used when you've got a custom widget which wants 
to respond to shortcuts when it has focus, eg. katepart, kdiroperator.  These 
situations don't use XMLGUI for action adding.

I have thought about this more, and come up with an alternative which should 
make everyone happy.  We remove {set|add|remove|clear}AssociatedWidget(s) and 
associatedWidgets(), so no more magic.  Then we add void 
associateWidget(QWidget*), which is a simple convenience function which 
iterates the actions, sets the shortcut context and adds the actions to the 
widget.  This means you have to be more careful when you call it (need to 
have all of your actions set up, or any extras have to be dealt with 
manually).

It turned out to be a fairly simple patch; of course, removing those methods 
would be BIC.  The patch adding this new solution is attached (but not 
removing the old methods, yet).

Comments?  If ok, would it be possible to remove the old methods, and if so, 
when?

Cheers,
Hamish
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kdelibs-associatewidget.patch
Type: text/x-diff
Size: 5159 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20071022/fd60012a/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kdebase-associatewidget.patch
Type: text/x-diff
Size: 1626 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20071022/fd60012a/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20071022/fd60012a/attachment.sig>