[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: KActionCollection setAssociatedWidget problems
From:       Hamish Rodda <rodda () kde ! org>
Date:       2007-10-22 11:04:34
Message-ID: 200710222104.37398.rodda () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


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

["kdelibs-associatewidget.patch" (text/x-diff)]

Index: kate/view/kateview.cpp
===================================================================
--- kate/view/kateview.cpp	(revision 728024)
+++ kate/view/kateview.cpp	(working copy)
@@ -259,7 +259,6 @@
 void KateView::setupActions()
 {
   KActionCollection *ac = this->actionCollection ();
-  ac->setAssociatedWidget(m_viewInternal);
   QAction *a;
 
   m_toggleWriteLock = 0;
@@ -561,6 +560,8 @@
 
   slotSelectionChanged ();
 
+  ac->associateWidget(m_viewInternal);
+
   connect (this, SIGNAL(selectionChanged(KTextEditor::View*)), this, SLOT(slotSelectionChanged()));
 }
 
@@ -573,7 +574,6 @@
 {
   m_editActions = new KActionCollection( static_cast<QObject*>(this) );
   m_editActions->setObjectName( "edit_actions" );
-  m_editActions->setAssociatedWidget(m_viewInternal);
   KActionCollection* ac = m_editActions;
 
   QAction* a = ac->addAction("word_left");
@@ -803,6 +803,8 @@
     slotGotFocus();
   else
     slotLostFocus();
+
+  m_editActions->associateWidget(m_viewInternal);
 }
 
 void KateView::setupCodeFolding()
Index: kfile/kdiroperator.cpp
===================================================================
--- kfile/kdiroperator.cpp	(revision 728024)
+++ kfile/kdiroperator.cpp	(working copy)
@@ -1456,7 +1456,6 @@
 {
     d->actionCollection = new KActionCollection(this);
     d->actionCollection->setObjectName("KDirOperator::actionCollection");
-    d->actionCollection->setAssociatedWidget(topLevelWidget());
 
     d->actionMenu = new KActionMenu(i18n("Menu"), this);
     d->actionCollection->addAction("popupMenu", d->actionMenu);
@@ -1558,6 +1557,8 @@
     viewMenu->addAction(detailedAction);
     // TODO: QAbstractItemView does not offer an action collection. Provide
     // an interface to add a custom action collection.
+
+    d->actionCollection->associateWidget(topLevelWidget());
 }
 
 void KDirOperator::setupMenu()
Index: kdeui/dialogs/kedittoolbar.cpp
===================================================================
--- kdeui/dialogs/kedittoolbar.cpp	(revision 728024)
+++ kdeui/dialogs/kedittoolbar.cpp	(working copy)
@@ -671,7 +671,6 @@
   QDomElement elem;
 
   m_widget->setFactory( factory );
-  m_widget->actionCollection()->setAssociatedWidget( m_widget );
 
   // add all of the client data
   bool first = true;
@@ -705,6 +704,8 @@
   loadToolBarCombo( defaultToolBar );
   m_widget->adjustSize();
   m_widget->setMinimumSize( m_widget->sizeHint() );
+
+  m_widget->actionCollection()->associateWidget( m_widget );
 }
 
 bool KEditToolBarWidget::save()
Index: kdeui/actions/kactioncollection.h
===================================================================
--- kdeui/actions/kactioncollection.h	(revision 728024)
+++ kdeui/actions/kactioncollection.h	(working copy)
@@ -125,6 +125,18 @@
   QList<QWidget*> associatedWidgets() const;
 
   /**
+   * Associate all actions in this collection to the given \a widget, and set all
+   * shortcutContexts to Qt::WidgetShortcut.  This has the effect of making the actions
+   * only trigger when:
+   * 1) the widget (or its children, it Qt 4.4 and above) has focus
+   * 2) the shortcut is pressed.
+   *
+   * This is important particularly when actions only apply to a specific widget, and
+   * when the application is a kpart and needs to respect other parts' shortcuts.
+   */
+  void associateWidget(QWidget* widget) const;
+
+  /**
    * Returns the KConfig group with which settings will be loaded and saved.
    */
   QString configGroup() const;
Index: kdeui/actions/kactioncollection.cpp
===================================================================
--- kdeui/actions/kactioncollection.cpp	(revision 728024)
+++ kdeui/actions/kactioncollection.cpp	(working copy)
@@ -338,6 +338,14 @@
   return d->associatedWidgets;
 }
 
+void KActionCollection::associateWidget(QWidget* widget) const
+{
+  foreach (QAction* action, actions()) {
+    action->setShortcutContext(Qt::WidgetShortcut);
+    widget->addAction(action);
+  }
+}
+
 QString KActionCollection::configGroup( ) const
 {
   return d->configGroup;
Index: kdeui/tests/ktoolbartest.cpp
===================================================================
--- kdeui/tests/ktoolbartest.cpp	(revision 728024)
+++ kdeui/tests/ktoolbartest.cpp	(working copy)
@@ -37,7 +37,7 @@
     action3->setSeparator(true);
     action7->setSeparator(true);
 
-    coll.setAssociatedWidget(tb);
+    coll.associateWidget(tb);
 
     mw->show();
 
Index: kdeui/widgets/kdatetable.cpp
===================================================================
--- kdeui/widgets/kdatetable.cpp	(revision 728024)
+++ kdeui/widgets/kdatetable.cpp	(working copy)
@@ -231,7 +231,6 @@
 void KDateTable::initAccels()
 {
     KActionCollection * localCollection = new KActionCollection( this );
-    localCollection->setAssociatedWidget( this );
 
     QAction* next = localCollection->addAction( QLatin1String( "next" ) );
     next->setShortcuts( KStandardShortcut::next() );
@@ -258,6 +257,7 @@
     connect( endWeek, SIGNAL( triggered( bool ) ), SLOT( endOfWeek() ) );
 
     localCollection->readSettings();
+    localCollection->associateWidget( this );
 }
 
 int KDateTable::posFromDate( const QDate &date_ )

["kdebase-associatewidget.patch" (text/x-diff)]

Index: apps/konsole/src/Part.cpp
===================================================================
--- apps/konsole/src/Part.cpp	(revision 728025)
+++ apps/konsole/src/Part.cpp	(working copy)
@@ -84,7 +84,7 @@
     _viewManager->widget()->setParent(parentWidget);
 
     setWidget(_viewManager->widget());
-    actionCollection()->setAssociatedWidget(_viewManager->widget());
+    actionCollection()->associateWidget(_viewManager->widget());
     
     // create basic session
     createSession(QString());
Index: apps/konsole/src/SessionController.cpp
===================================================================
--- apps/konsole/src/SessionController.cpp	(revision 728025)
+++ apps/konsole/src/SessionController.cpp	(working copy)
@@ -87,8 +87,8 @@
 
     // handle user interface related to session (menus etc.)
     setXMLFile("konsole/sessionui.rc");
-    actionCollection()->setAssociatedWidget(view);
     setupActions();
+    actionCollection()->associateWidget(view);
 
     setIdentifier(_session->sessionId());
     sessionTitleChanged();
Index: apps/konsole/src/MainWindow.cpp
===================================================================
--- apps/konsole/src/MainWindow.cpp	(revision 728025)
+++ apps/konsole/src/MainWindow.cpp	(working copy)
@@ -60,8 +60,8 @@
     // file can be found when this code is being used in the Konsole part.
     setXMLFile("konsole/konsoleui.rc");
 
-    actionCollection()->setAssociatedWidget(this);
     setupActions();
+    actionCollection()->associateWidget(this);
 
     // create view manager
         _viewManager = new ViewManager(this,actionCollection());

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]