Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut

Gregor Mi codestruct at posteo.org
Fri Mar 13 22:09:14 GMT 2015



> On March 2, 2015, 7:47 a.m., Martin Gräßlin wrote:
> > processui/keyboardshortcututil.cpp, line 46
> > <https://git.reviewboard.kde.org/r/122249/diff/6/?file=351945#file351945line46>
> >
> >     This looks to complicated. It should be much easier to do with the KGlobalAccel API:
> >     * create a KActionCollection for component "kwin"
> >     * add a QAction with the shortcut name you want
> >     * ask KGlobalAccel to load the shortcut for it.
> 
> Gregor Mi wrote:
>     Thanks for the hint but I am not sure of how to use the API in such a way. I tried two things:
>     
>     1)
>         org::kde::KGlobalAccel kglobalaccel("org.kde.kglobalaccel", "/kglobalaccel", bus);
>     
>         auto kwinActions = kglobalaccel.allActionsForComponent(QStringList("kwin"));
>         Q_FOREACH(auto aaa, kwinActions.value()) {
>             //qDebug() << aaa; // ("kwin", "Kill Window", "KWin", "Kill Window")
>         }
>     Then I wonder how to feed the QStringList to a KActionCollection.
>     
>     2)
>         KActionCollection ac(nullptr, QString());
>         ac.setComponentName("kwin");
>         // ac.importGlobalShortcuts();
>         Q_FOREACH(auto bbb, ac.actions()) {
>             if (bbb->text() == "Kill Window") {
>                 qDebug() << bbb;
>             }
>         }
>     But the ac.actions() list is empty.
>     
>     Which of the two ways should be used?
> 
> Thomas Lübking wrote:
>     tried this?
>     -----------
>     KActionCollection ac(this, "kwin");
>     ac.setConfigGlobal(true);
>     QAction *act  = ac.action("Kill Window");
> 
> Gregor Mi wrote:
>     I have no QObject* as parent. Can this be the cause?
>     
>     KActionCollection ac(nullptr, "kwin");
>     ac.setConfigGlobal(true);
>     auto killWindowAction  = ac.action("Kill Window");
>     qDebug() << ac.actions().count();
>     qDebug() << killWindowAction;
>     /*
>     RESULT:
>     0
>     QObject(0x0)
>     */
> 
> Thomas Lübking wrote:
>     You'd have "qApp", but I don't actually think so.
> 
> Martin Gräßlin wrote:
>     try: ac.addAction instead of ac.action
>     
>     For reference code look at e.g. kwin/effects/desktopgrid/desktopgrid_config.cpp
> 
> Gregor Mi wrote:
>     I tried this:
>     
>     KActionCollection ac(nullptr, "kwin");
>     ac.setConfigGroup("default"); // needed?
>     ac.setConfigGlobal(true);
>     auto killWindowAction  = ac.addAction("Kill Window");
>     killWindowAction->setProperty("isConfigurationAction", true); // neded?
>     qDebug() << ac.actions().count();
>     qDebug() << KGlobalAccel::self()->shortcut(killWindowAction).count();
>     /*
>     CONSOLE OUTPUT:
>     1
>     0
>     */
> 
> Martin Gräßlin wrote:
>     I think you need to register killWindowAction with KGlobalAccel in some way so that it loads the configured shortcut.
> 
> Gregor Mi wrote:
>     I found out how to prepare the QAction without KActionCollection.
>     
>     auto killWindowAction  = new QAction(nullptr);
>     killWindowAction->setProperty("componentName", "kwin"); // see impl of KActionCollection
>     killWindowAction->setObjectName("Kill Window"); // see impl of KActionCollection
>     // killWindowAction->setProperty("isConfigurationAction", true); // neded?
>     qDebug() << killWindowAction->objectName(); // --> "Kill Window"
>     // qDebug() << ac.actions().count(); // --> 1
>     qDebug() << KGlobalAccel::self()->isComponentActive("kwin"); // --> true
>     qDebug() << KGlobalAccel::self()->allActionsForComponent({ "kwin" }).count(); // (deprecated) --> 152
>     qDebug() << KGlobalAccel::self()->hasShortcut(killWindowAction); // --> false
>     qDebug() << KGlobalAccel::self()->shortcut(killWindowAction).count(); // --> 0
>     qDebug() << KGlobalAccel::self()->defaultShortcut(killWindowAction).count(); // --> 0
>     delete killWindowAction;
>     
>     Open issues:
>     1. Is it necessary to specify the shortcut context ("default") somewhere?
>     2. How to tell KGlobalAccel to load the shortcuts for the given action.
> 
> Gregor Mi wrote:
>     Issue 1.: *not* necessary, see kde/workspace/plasma-workspace/kglobalaccel/component.h, line 98
> 
> Martin Gräßlin wrote:
>     concerning 2: see http://api.kde.org/frameworks-api/frameworks5-apidocs/kglobalaccel/html/classKGlobalAccel.html#a66ce504227e7e563f24de4c6b26b0395
>     
>     shortcut context is (I think) not needed.
> 
> Gregor Mi wrote:
>     I read the API documentation about setShortcut and frankly I do not fully understand it. It seems counter-intuitive to me to call that method in order to *load* the shortcut.
> 
> Martin Gräßlin wrote:
>     yes the API is weird. But it's really like it: if one doesn't pass NoAutloading the shortcut gets loaded.
> 
> Gregor Mi wrote:
>     yes, you are right, it is really working like this:
>     
>         KActionCollection ac(nullptr, "kwin");
>         auto killWindowAction  = ac.addAction("Kill Window");
>         //killWindowAction->setProperty("componentName", "kwin"); // see impl of KActionCollection
>         //killWindowAction->setObjectName("Kill Window"); // see impl of KActionCollection
>         qDebug() << killWindowAction->objectName(); // --> "Kill Window"
>         qDebug() << KGlobalAccel::self()->hasShortcut(killWindowAction); // --> true
>         KGlobalAccel::self()->setShortcut(killWindowAction, { }); // KGlobalAccel::Autoloading is default
>         auto shortcut = KGlobalAccel::self()->shortcut(killWindowAction);
>         qDebug() << shortcut.count() ; // --> 1
>         if (shortcut.count() > 0) {
>             qDebug() << shortcut; // --> Ctrl+Alt+Esc
>         }
>     
>     Thanks!
>     
>     What do you think about adding an API function `loadShortcut(QString componentName, QString actionName)`? (Or just add this example to the documentation of the shortcut() method)
> 
> Gregor Mi wrote:
>     Don't count your chickens before they are hatched.
>     
>     With the following reworked method I now observe that the global shortcut stops working (when the actual keyboard shortcut is pressed) although the correct shortcut is returned. I have to relogin to make it work again.
>     
>     QList<QKeySequence> KeyboardShortcutUtil::findGlobalShortcutByComponentAndActionName(const QString& componentName, const QString& uniqueName)
>     {
>         KActionCollection ac(nullptr, componentName); // e.g. "kwin"
>         auto killWindowAction  = ac.addAction(uniqueName); // e.g. "Kill Window"
>         //interally the following will be done:
>         //  killWindowAction->setProperty("componentName", "kwin"); // see impl of KActionCollection
>         //  killWindowAction->setObjectName("Kill Window"); // see impl of KActionCollection
>     
>         // we need to call this to load the settings in order to make the shortcut avaible
>         KGlobalAccel::self()->setShortcut(killWindowAction, { }); // KGlobalAccel::Autoloading is default
>         // TODO: this will disabled the global shortcut (though it is still shown in KDE Global Keyboard Shortcuts dialog)
>     
>         // retrieve shortcut list
>         auto shortcutList = KGlobalAccel::self()->shortcut(killWindowAction);
>     
>         // TODO: set the shortcut list to reenable it (does not work; you have to relogin)
>         KGlobalAccel::self()->setShortcut(killWindowAction, shortcutList); // also does not work
>     
>         return shortcutList;
>     }
> 
> Thomas Lübking wrote:
>     looking at kglobalaccl sources, this:
>     > KGlobalAccel::self()->setShortcut(killWindowAction, { });
>     
>     should cause this
>     > auto shortcutList = KGlobalAccel::self()->shortcut(killWindowAction);
>     
>     to be "{}" ?
>     
>     Instead query bool KGlobalAccel::isComponentActive("kwin"), cause otherwise the proper shortcut is "none" anyway ;-)
> 
> Gregor Mi wrote:
>     > looking at kglobalaccl sources, this:
>     > >   KGlobalAccel::self()->setShortcut(killWindowAction, { });
>     
>     > should cause this
>     > >    auto shortcutList = KGlobalAccel::self()->shortcut(killWindowAction);
>     
>     > to be "{}" ?
>     
>     I thought so too when I looked at the code but it actually returns the correct shortcut (e.g. Ctrl+Alt+Esc). But it also causes that this shortcut stops working when it is pressed on the keyboard.
> 
> Gregor Mi wrote:
>     Trying to make it work with the KDE Global Keyboardshortcuts dialog has no effect. I have to relogin. Maybe some conflict between my KDE 4 and KF5?
> 
> Gregor Mi wrote:
>     KGlobalAccel::isComponentActive("kwin") is always true when I put it on different places in the above code listing.
> 
> Thomas Lübking wrote:
>     Yes, because kwin is running ;-)
>     The question is, whether this does also make KGlobalAccel::self()->shortcut(killWindowAction) return a usable list (and w/o the "need" to disable the shortcut first)
> 
> Gregor Mi wrote:
>     Ah. ok. I removed the setShortcut calls but the isComponentActive has no effect. The list is empty.
> 
> Gregor Mi wrote:
>     I now also tried to modify KGlobalAccel itself and ran into this build error:
>     
>     CMake Error at CMakeLists.txt:31 (kf5_add_kdeinit_executable)
>     
>     Is it supposed not to build separate from the plasma-workspace module?
> 
> Gregor Mi wrote:
>     Sorry for the noise: I took the wrong KGlobalAccel project (there is one in frameworks and one in plasma-workspace)

[SOLVED] I added this method to KGlobalAccel:

/**
 * See documentation of KGlobalAccel::Autoloading:
 * "Look up the action in global settings (using its main component's name and text())
 *  and set the shortcut as saved there."
 */
void KGlobalAccel::loadShortcutFromGlobalSettings(QAction *action)
{
    d->updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);
}

When I call this in KeyboardShortcutUtil::findGlobalShortcutByComponentAndActionName instead of setShortcut(...) everything works fine. Though, I am not sure if this fits with the current KGlobalAccel API concept.


- Gregor


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
-----------------------------------------------------------


On March 13, 2015, 10:08 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122249/
> -----------------------------------------------------------
> 
> (Updated March 13, 2015, 10:08 p.m.)
> 
> 
> Review request for KDE Base Apps, Martin Gräßlin and John Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> -------
> 
> Current situation:
> The "End Process..." button has a tooltip which says "To target a specific window to kill, press Ctrl+Alt+Esc at any time." The keyboard shortcut is hardcoded.
> 
> New:
> Replace the "End Process..." button with a drop-down button and a action named "Kill a specific window..."
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 5352e70f6f37daae76c1b3e2c1c9f149d235e3cd 
>   processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 
>   processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 
>   processui/keyboardshortcututil.h PRE-CREATION 
>   processui/keyboardshortcututil.cpp PRE-CREATION 
>   processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 
>   tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 
>   tests/keyboardshortcututiltest.h PRE-CREATION 
>   tests/keyboardshortcututiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122249/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> New End Process button with drop down arrow
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/16301e88-e21b-4358-9a63-a85dae5722bd__screenshot_default1.png
> Drop down shows Kill Window
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/01/28/58df12c5-7350-4bb0-b602-c5716caa9836__screenshot_default2.png
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150313/912f9bce/attachment.htm>


More information about the kde-core-devel mailing list