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

List:       kde-core-devel
Subject:    Re: [PATCH] Unbreak global shortcuts configuration
From:       David Faure <faure () kde ! org>
Date:       2008-02-05 12:06:16
Message-ID: 200802051306.16845.faure () kde ! org
[Download RAW message or body]

On Wednesday 30 January 2008, Aurélien Gâteau wrote:
> KAction instances talk to KGlobalAccel singleton to make it aware of global
> shortcuts changes via KGlobalAccel::updateGlobalShortcuts() and
> KGlobalAccel::updateGlobalShortcutsAllowed() (i'm still not sure why there
> are two methods).

I just updated the README file to explain why there are two:

KGlobalAccel::updateGlobalShortcutsAllowed(true) sets the "SetPresent" flag when calling
kdedglobalaccel, which makes kdedglobalaccel actually grab the key shortcut
(so that the grab is done after the action has been defined, and only if it is enabled).
kdedglobalaccel must know about inactive global shortcuts too (e.g. those defined in
applications not running at the moment), for conflict resolution.

The code between the two method was duplicated though, so I factorized it,
see attached patch. The question is whether it makes the code easier to understand,
or worse :-) Shall I commit this refactor?

David (back to debugging why kglobalshortcuttest fails to set a global shortcut)

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

["kglobalaccel.diff" (text/x-diff)]

Index: kglobalaccel_p.h
===================================================================
--- kglobalaccel_p.h	(revision 768814)
+++ kglobalaccel_p.h	(working copy)
@@ -44,6 +44,9 @@ public:
     ///Register or unregister the action in this class, and notify the KDED module
     void updateGlobalShortcutAllowed(KAction *action, /*KAction::ShortcutTypes*/uint \
flags);  
+    /// Helper method for the above two, takes care of the actual call to kded
+    QList<int> updateGlobalShortcutInKded(KAction* action, const QStringList& \
actionId, uint flags, uint initialSetterFlags); +
     QList<int> intListFromShortcut(const KShortcut &cut);
     KShortcut shortcutFromIntList(const QList<int> &list);
 
Index: kglobalaccel.cpp
===================================================================
--- kglobalaccel.cpp	(revision 768814)
+++ kglobalaccel.cpp	(working copy)
@@ -132,6 +132,27 @@ KGlobalAccel *KGlobalAccel::self( )
     return s_instance;
 }
 
+QList<int> KGlobalAccelPrivate::updateGlobalShortcutInKded(KAction* action, const \
QStringList& actionId, uint flags, uint initialSetterFlags) +{
+    uint setterFlags = initialSetterFlags;
+    KShortcut defaultShortcut = action->globalShortcut(KAction::DefaultShortcut);
+    KShortcut activeShortcut = action->globalShortcut();
+    if (flags & KAction::NoAutoloading)
+        setterFlags |= KdedGlobalAccel::NoAutoloading;
+    if (defaultShortcut.isEmpty())
+        setterFlags |= KdedGlobalAccel::IsDefaultEmpty;
+    if (defaultShortcut == activeShortcut)
+        setterFlags |= KdedGlobalAccel::IsDefault;
+
+    const QList<int> result = iface.setShortcut(actionId,
+                                                \
intListFromShortcut(action->globalShortcut()), +                                      \
setterFlags); +    KShortcut scResult(shortcutFromIntList(result));
+
+    if (scResult != action->globalShortcut())
+        action->d->setActiveGlobalShortcutNoEnable(scResult);
+    return result;
+}
 
 void KGlobalAccelPrivate::updateGlobalShortcutAllowed(KAction *action, uint flags)
 {
@@ -151,28 +172,13 @@ void KGlobalAccelPrivate::updateGlobalSh
     //TODO: what about i18ned names?
 
     if (!oldEnabled && newEnabled) {
-        uint setterFlags = KdedGlobalAccel::SetPresent;
-        KShortcut defaultShortcut = \
                action->globalShortcut(KAction::DefaultShortcut);
-        KShortcut activeShortcut = action->globalShortcut();
-        if (flags & KAction::NoAutoloading)
-            setterFlags |= KdedGlobalAccel::NoAutoloading;
-        if (defaultShortcut.isEmpty())
-            setterFlags |= KdedGlobalAccel::IsDefaultEmpty;
-        if (defaultShortcut == activeShortcut)
-            setterFlags |= KdedGlobalAccel::IsDefault;
-
+        // action is now enabled
         nameToAction.insert(actionId.at(1), action);
         actionToName.insert(action, actionId.at(1));
-        QList<int> result = iface.setShortcut(actionId,
-                                              \
                intListFromShortcut(action->globalShortcut()),
-                                              setterFlags);
-        KShortcut scResult(shortcutFromIntList(result));
-
-        if (scResult != action->globalShortcut())
-            action->d->setActiveGlobalShortcutNoEnable(scResult);
+        updateGlobalShortcutInKded(action, actionId, flags, \
KdedGlobalAccel::SetPresent);  }
-
-    if (oldEnabled && !newEnabled) {
+    else if (oldEnabled && !newEnabled) {
+        // action is now disabled
         nameToAction.remove(actionToName.take(action));
         iface.setInactive(actionId);
     }
@@ -190,24 +196,7 @@ void KGlobalAccelPrivate::updateGlobalSh
     actionId.append(action->text());
     //TODO: what about i18ned names?
 
-    uint setterFlags = 0;
-    KShortcut defaultShortcut = action->globalShortcut(KAction::DefaultShortcut);
-    KShortcut activeShortcut = action->globalShortcut();
-    if (flags & KAction::NoAutoloading)
-        setterFlags |= KdedGlobalAccel::NoAutoloading;
-    if (defaultShortcut.isEmpty())
-        setterFlags |= KdedGlobalAccel::IsDefaultEmpty;
-    if (defaultShortcut == activeShortcut)
-        setterFlags |= KdedGlobalAccel::IsDefault;
-
-    QList<int> result = iface.setShortcut(actionId,
-                                          \
                intListFromShortcut(action->globalShortcut()),
-                                          setterFlags);
-    KShortcut scResult(shortcutFromIntList(result));
-
-    if (scResult != action->globalShortcut()) {
-        action->d->setActiveGlobalShortcutNoEnable(scResult);
-    }
+    const QList<int> result = updateGlobalShortcutInKded(action, actionId, flags, \
0);  
     //We might be able to avoid that call sometimes, but it's neither worth the \
effort nor  //the bytes to determine the cases where it's safe to avoid it.
@@ -242,6 +231,7 @@ KShortcut KGlobalAccelPrivate::shortcutF
 void KGlobalAccelPrivate::_k_invokeAction(const QStringList &actionId)
 {
     //TODO: can we make it so that we don't have to check the mainComponentName? \
(i.e. targeted signals) +    // Well, how about making the full QStringList the key \
                in nameToAction?
     if (actionId.at(0) != mainComponentName || isUsingForeignComponentName)
         return;
 



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