Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On Jan. 26, 2015, 7:05 vorm., Martin Gräßlin wrote:
My opinion is that this is a feature which should not be exposed in
libksysguard. It actually ties libksysguard to KWin, while libksysguard was
in the past also used in e.g. kdevelop.
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself.
Martin Gräßlin wrote:
In addition: KWin's global shortcut action names are not public API. We
do not guarantee that we don't change them, we do not guarantee that they are
exposed at all (KWin handling shortcuts internally without kglobalaccel on
Wayland?). I do not want to run into situations that we cannot change our
code because external usage makes it impossible.
Thomas Lübking wrote:
In case there was larger demand for invoking such action (taskbar,
dedicated plasmoid, ...) one could move the xkill functionality into
KWindowSystem (option for portage) - invoking a kwin shortcut through a
kglobalaccel dbus call is a hack. Maybe sufficient for any downstream
solution, but easily broken feature.
Gregor Mi wrote:
First of all, a clarification of this RR's intentions:
1) The original End Process... tooltip says you can always use
Ctrl+Alt+Esc... which is wrong as soon as someone changes the keyboard
shortcut exposed by KWin. So this should be fixed.
2) Make the Kill Window feature more discoverable. It is a seldom used
feature which makes it harder to remember.
About invoking Kill Window:
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself. [Martin]
...one could move the xkill functionality into KWindowSystem...
[Thomas]
Without knowing the amount of xkill code I suspect that having a dbus
call that loosly couples libksysguard to KWin is probably easier to maintain
than 2 times the xkill code.
Having said that, what about moving the xkill code to a common location
as Thomas suggested?
We do not guarantee that we don't change them, we do not guarantee that
they are exposed at all ... I do not want to run into situations that we
cannot change our code because external usage makes it impossible.
Understood. But would it then be possible at all to get the current
shortcut to display it to the user?
Martin Gräßlin wrote:
Ok, so this addresses two issues using one solution: exposing KWin's
internal shortcut. This is bad as outlined above.
I agree that 1) needs fixing. This can be done in the way as approached
in this review request: check whether kwin is registered on kglobalaccel and
get the key command. If it's done that way the fault is with libksysguard in
case KWin changes the shortcut name or doesn't use kglobalaccel any more.
Another fix is of course to just hide the shortcut.
2) is a different issue. Whether it's needed to expose the functionality
in addition from libksysguard is probably questionable. A better approach to
do this would be through a method in KWindowSystem. This does not need to
duplicate the code, it could also just send a client message to the window
manager to start the kill window process. Through KWindowSystem we can check
whether the feature is supported by the window manager and could exclude if
not supported. But and that's a big but: the feature would not be able to
work if it's triggered from a (context) menu or drop down list (it needs to
grab mouse). Given that I'm hesistant to say that it should be added to
kwindowsystem at all.
Thomas Lübking wrote:
ad 2)
I'd have said to rather *move* the code to KWindowSystem and use it from
there by any client (incl. kwin)
This allows porting the solution (in case such is possible on other
systems at all) as well as to invoke the feature unconditionally (ie. instead
of is this kwin? yes? tell kwin to trigger xkill. just trigger the xkill
functionality)
About the popupmenu:
The issue is global, ie. as long as a popup (or other grabber) is
around, the kwin shortcut neither works.
It's kind of the client codes problem to deal w/ a false return (eg.
invoke a timer and/or timered retries)
Gregor Mi wrote:
ad 1) (shortcut)
I could live with adapting (or remove) the shortcut retrieval as soon as
it will not be possible anymore. As long as it is, I would show it. (I
suspect as long as the shortcut is not hard-coded there will be a some way to
get it)
ad 2) (invoke window kill)
I looked a Kwin's source code. For reference, here are the two methods I
found to kill a window:
```
/*!
Kill Window feature, similar to xkill
*/
void Workspace::slotKillWindow()
{
if (m_windowKiller.isNull()) {
m_windowKiller.reset(new KillWindow());
}
m_windowKiller-start();
}
and
/**
* Kills
Moving KDiagram to extragear/graphics/libs (was: Re: KDiagram libs (KChart, KGantt) in KDE Review)
Hi, 2 weeks have passed, seems there are no stoppers for KDiagram to move on into extragear/graphics/libs :) So filing now a ticket to sysadmin to do the move. Thanks Albert, Aleix, Adriaan for having given KDiagram a check and (helping on) solving the issues you found (so KDiagram is Triple A rated? ;) ) Thanks also to whoever silently gave KDiagram some testing but did not report anything (hopefully a good sign). Status: * KDiagram builds on CI without warnings, all tests pass: http://build.kde.org/job/kdiagram_master_qt5/ * bugs.kde.org: added Product kdiagram, components KChart KGantt * reviewboard: kdiagram (first review request handled) * i18n.kde.org: integrated, first translations done: http://i18n.kde.org/stats/gui/trunk-kf5/po/kgantt_qt.po/ http://i18n.kde.org/stats/gui/trunk-kf5/po/kchart_qt.po/ * KMyMoney frameworks branch ported to KChart (and found first bugs :P) * Massif-Visualizer frameworks branch has review request for port to KChart (maintainer on vacation currently) * Calligra will start the Qt5/KF5 port the next days, so nothing yet to do Currently known unsolved issues (which I consider no showstoppers): * lupdate/Scripty fails on some files, but no deal right now, as no strings are in code affected. Will look into once I have more time. * code is build with different flag if unit tests are build, not perfect actually only enables unit tests embedded directly in the source files, so not changing actual library code (marked as TODO for now, no regression against currently used old snapshots of KDChart) First release: Other than initially planned I will not do an immediate release of KDiagram now, but first collect some more feedback by the projects porting to it, especially Calligra. First release will still happen before or at time the first release of any project known that ported to it of course (so tell if yours does :) ). Cheers Friedrich
Re: Review request: QBluez
Still failing here Oops, sorry. It's because the problem is not that you are running Bluez 4, but because the method call is rejected (auth issue?). Anyway, I modified the tests again so that it now checks whether the Bluez 5 is running and is functional (it checks if the call to GetManagedObjects succeeds). It should catch your error and correctly skip the tests now. David On Fri, Feb 20, 2015 at 9:57 PM, Albert Astals Cid aa...@kde.org wrote: El Dimecres, 18 de febrer de 2015, a les 11:50:01, David Rosca va escriure: If it's an expected situation, handle the situation. I have modified the tests (only the ones who would fail) so they will be skipped if Bluez 4 is detected. Still failing here 4: Test command: /home/kdeunstable/qbluez/build/autotests/adaptertest 4: Test timeout computed to be: 9.99988e+06 4: * Start testing of AdapterTest * 4: Config: Using QtTest library 5.4.0, Qt 5.4.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 4.9.2) 4: QWARN : AdapterTest::initTestCase() BluezQt: GetManagerJob Error: Rejected send message, 2 matched rules; type=method_call, sender=:1.145 (uid=1003 pid=10596 comm=/home/kdeunstable/qbluez/build/autotests/adapterte) interface=org.freedesktop.DBus.ObjectManager member=GetManagedObjects error name=(unset) requested_reply=0 destination=org.bluez (uid=0 pid=556 comm=/usr/sbin/bluetoothd ) 4: FAIL! : AdapterTest::initTestCase() '!initJob-error()' returned FALSE. () 4:Loc: [/home/kdeunstable/qbluez/autotests/adaptertest.cpp(76)] 4: PASS : AdapterTest::cleanupTestCase() 4: Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted 4: * Finished testing of AdapterTest * 4/6 Test #4: bluezqt-adaptertest ..***Failed0.01 sec test 5 Start 5: bluezqt-devicetest 5: Test command: /home/kdeunstable/qbluez/build/autotests/devicetest 5: Test timeout computed to be: 9.99988e+06 5: * Start testing of DeviceTest * 5: Config: Using QtTest library 5.4.0, Qt 5.4.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 4.9.2) 5: QWARN : DeviceTest::initTestCase() BluezQt: GetManagerJob Error: Rejected send message, 2 matched rules; type=method_call, sender=:1.146 (uid=1003 pid=10597 comm=/home/kdeunstable/qbluez/build/autotests/devicetes) interface=org.freedesktop.DBus.ObjectManager member=GetManagedObjects error name=(unset) requested_reply=0 destination=org.bluez (uid=0 pid=556 comm=/usr/sbin/bluetoothd ) 5: FAIL! : DeviceTest::initTestCase() '!initJob-error()' returned FALSE. () 5:Loc: [/home/kdeunstable/qbluez/autotests/devicetest.cpp(96)] 5: PASS : DeviceTest::cleanupTestCase() 5: Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted 5: * Finished testing of DeviceTest * 5/6 Test #5: bluezqt-devicetest ...***Failed0.01 sec I have also renamed the library to BluezQt. Here is an updated documentation: http://david.rosca.cz/bluezqt-apidocs/html/ David On Wed, Feb 18, 2015 at 2:19 AM, Thiago Macieira thi...@kde.org wrote: On Tuesday 17 February 2015 23:00:15 Albert Astals Cid wrote: It doesn't have the DBus ObjectManager, so that call should fail like that. Well, then the test should try to detect it and QEXPECT_FAIL, havign tests fail means something is bad, and as you say it's not bad, just expected. QEXPECT_FAIL is when you know it's wrong but either can't solve it or can't do it now. If it's an expected situation, handle the situation. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Feb. 20, 2015, 11:35 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- - Use QProcess::startDetached(xkill); instead of using the KWin method. This is not as advanced as the KWin method (e.g. the action can only be aborted with right-click instead of also Esc) but this way there is no coupling to KWin - Add a Don't ask again message box that warns the user about what will happen. 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. RR: Add a drop down menu to the End Process... button with one action: i18n(Kill a specific window... (Global shortcut: %1), killWindowShortcut) Diffs (updated) - 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
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- can you check what needs to be adjusted in plasma-workspace? it fails to build with your change: [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(unresolved overloaded function type, QString, QString)' [ 451s] services QExplicitlySharedDataPointerKService(new KService(proc-name, cmdline, QString())); [ 451s] ^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are: [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream, int) [ 451s] KService(QDataStream str, int offset); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString) [ 451s] explicit KService(const KDesktopFile *config, const QString entryPath = QString()); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString) [ 451s] explicit KService(const QString fullpath); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString, const QString, const QString) [ 451s] KService(const QString name, const QString exec, const QString icon); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from 'unresolved overloaded function type' to 'const QString' [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString, const QStringList)': [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ - Hrvoje Senjan On Feb. 20, 2015, 10:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 10:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: can you check what needs to be adjusted in plasma-workspace? it fails to build with your change: [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(unresolved overloaded function type, QString, QString)' [ 451s] services QExplicitlySharedDataPointerKService(new KService(proc-name, cmdline, QString())); [ 451s] ^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are: [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream, int) [ 451s] KService(QDataStream str, int offset); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString) [ 451s] explicit KService(const KDesktopFile *config, const QString entryPath = QString()); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString) [ 451s] explicit KService(const QString fullpath); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString, const QString, const QString) [ 451s] KService(const QString name, const QString exec, const QString icon); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from 'unresolved overloaded function type' to 'const QString' [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString, const QStringList)': [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ Probably `proc-command` must be replace with `proc-command()`. I'll check that. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On Jan. 26, 2015, 7:05 vorm., Martin Gräßlin wrote:
My opinion is that this is a feature which should not be exposed in
libksysguard. It actually ties libksysguard to KWin, while libksysguard was
in the past also used in e.g. kdevelop.
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself.
Martin Gräßlin wrote:
In addition: KWin's global shortcut action names are not public API. We
do not guarantee that we don't change them, we do not guarantee that they are
exposed at all (KWin handling shortcuts internally without kglobalaccel on
Wayland?). I do not want to run into situations that we cannot change our
code because external usage makes it impossible.
Thomas Lübking wrote:
In case there was larger demand for invoking such action (taskbar,
dedicated plasmoid, ...) one could move the xkill functionality into
KWindowSystem (option for portage) - invoking a kwin shortcut through a
kglobalaccel dbus call is a hack. Maybe sufficient for any downstream
solution, but easily broken feature.
Gregor Mi wrote:
First of all, a clarification of this RR's intentions:
1) The original End Process... tooltip says you can always use
Ctrl+Alt+Esc... which is wrong as soon as someone changes the keyboard
shortcut exposed by KWin. So this should be fixed.
2) Make the Kill Window feature more discoverable. It is a seldom used
feature which makes it harder to remember.
About invoking Kill Window:
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself. [Martin]
...one could move the xkill functionality into KWindowSystem...
[Thomas]
Without knowing the amount of xkill code I suspect that having a dbus
call that loosly couples libksysguard to KWin is probably easier to maintain
than 2 times the xkill code.
Having said that, what about moving the xkill code to a common location
as Thomas suggested?
We do not guarantee that we don't change them, we do not guarantee that
they are exposed at all ... I do not want to run into situations that we
cannot change our code because external usage makes it impossible.
Understood. But would it then be possible at all to get the current
shortcut to display it to the user?
Martin Gräßlin wrote:
Ok, so this addresses two issues using one solution: exposing KWin's
internal shortcut. This is bad as outlined above.
I agree that 1) needs fixing. This can be done in the way as approached
in this review request: check whether kwin is registered on kglobalaccel and
get the key command. If it's done that way the fault is with libksysguard in
case KWin changes the shortcut name or doesn't use kglobalaccel any more.
Another fix is of course to just hide the shortcut.
2) is a different issue. Whether it's needed to expose the functionality
in addition from libksysguard is probably questionable. A better approach to
do this would be through a method in KWindowSystem. This does not need to
duplicate the code, it could also just send a client message to the window
manager to start the kill window process. Through KWindowSystem we can check
whether the feature is supported by the window manager and could exclude if
not supported. But and that's a big but: the feature would not be able to
work if it's triggered from a (context) menu or drop down list (it needs to
grab mouse). Given that I'm hesistant to say that it should be added to
kwindowsystem at all.
Thomas Lübking wrote:
ad 2)
I'd have said to rather *move* the code to KWindowSystem and use it from
there by any client (incl. kwin)
This allows porting the solution (in case such is possible on other
systems at all) as well as to invoke the feature unconditionally (ie. instead
of is this kwin? yes? tell kwin to trigger xkill. just trigger the xkill
functionality)
About the popupmenu:
The issue is global, ie. as long as a popup (or other grabber) is
around, the kwin shortcut neither works.
It's kind of the client codes problem to deal w/ a false return (eg.
invoke a timer and/or timered retries)
Gregor Mi wrote:
ad 1) (shortcut)
I could live with adapting (or remove) the shortcut retrieval as soon as
it will not be possible anymore. As long as it is, I would show it. (I
suspect as long as the shortcut is not hard-coded there will be a some way to
get it)
ad 2) (invoke window kill)
I looked a Kwin's source code. For reference, here are the two methods I
found to kill a window:
```
/*!
Kill Window feature, similar to xkill
*/
void Workspace::slotKillWindow()
{
if (m_windowKiller.isNull()) {
m_windowKiller.reset(new KillWindow());
}
m_windowKiller-start();
}
and
/**
* Kills
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review76389 --- processui/ksysguardprocesslist.cpp https://git.reviewboard.kde.org/r/122249/#comment52615 leaving aside that the patch is not clean (still contains kglobalaccel stuff, ie. is probably just a variant proposal): I don't have xkill installed. It's not a dependency of anything in KDE - you'd have to add it (for package maintainers, no idea how to do that, though) (ftr: the popup grabs mouse limitations mostly hold for invoking xkill as well - just that there it would become harder to check for successful invocation) - Thomas Lübking On Feb. 20, 2015, 11:35 nachm., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Feb. 20, 2015, 11:35 nachm.) 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. RR: Add a drop down menu to the End Process... button with one action: i18n(Kill a specific window... (Global shortcut: %1), killWindowShortcut) Diffs - 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
Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
Hi all, As you may have noticed, right now starting plasma is a big spam of the following error: Calling KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated, use KPluginInfo::pluginName() in /whatever/plugin.so instead. i tried to see where it happens, and seems it's in ktradeparsetree.cpp , line 30 QVariant ParseContext::property(const QString _key) const I don't think this properly fixable, since from the stack trace it seems an appropriate use.. i see two ways to fix it: 1) in ParseContext::property stuf a very long if.. else.. that makes it call the proper KPluginInfo::correctAccessor() .. but is ugly and slows it down 2) since this is an appropriate use, consider it not wrong anymore, and just get rid of the warning. Opinions? ideas? -- Marco Martin
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
On Saturday, February 21, 2015 11:02:48 Marco Martin wrote: As you may have noticed, right now starting plasma is a big spam of the following error: Calling KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated, use KPluginInfo::pluginName() in /whatever/plugin.so instead. i tried to see where it happens, and seems it's in ktradeparsetree.cpp , line 30 QVariant ParseContext::property(const QString _key) const I don't think this properly fixable, since from the stack trace it seems an appropriate use.. i see two ways to fix it: 1) in ParseContext::property stuf a very long if.. else.. that makes it call the proper KPluginInfo::correctAccessor() .. but is ugly and slows it down 2) since this is an appropriate use, consider it not wrong anymore, and just get rid of the warning. Opinions? ideas? I've looked at it, too, since the warnings shown are just burying any useful information during the startup of Plasmashell. I think the warning is rather useless, and I'd just remove it, so 2). Cheers, -- sebas Sebastian Kügler|http://vizZzion.org| http://kde.org
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: can you check what needs to be adjusted in plasma-workspace? it fails to build with your change: [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(unresolved overloaded function type, QString, QString)' [ 451s] services QExplicitlySharedDataPointerKService(new KService(proc-name, cmdline, QString())); [ 451s] ^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are: [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream, int) [ 451s] KService(QDataStream str, int offset); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString) [ 451s] explicit KService(const KDesktopFile *config, const QString entryPath = QString()); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString) [ 451s] explicit KService(const QString fullpath); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString, const QString, const QString) [ 451s] KService(const QString name, const QString exec, const QString icon); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from 'unresolved overloaded function type' to 'const QString' [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString, const QStringList)': [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. How can I find out if which branch was compiled? I assume it is the master branch. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On Jan. 26, 2015, 7:05 a.m., Martin Gräßlin wrote:
My opinion is that this is a feature which should not be exposed in
libksysguard. It actually ties libksysguard to KWin, while libksysguard was
in the past also used in e.g. kdevelop.
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself.
Martin Gräßlin wrote:
In addition: KWin's global shortcut action names are not public API. We
do not guarantee that we don't change them, we do not guarantee that they are
exposed at all (KWin handling shortcuts internally without kglobalaccel on
Wayland?). I do not want to run into situations that we cannot change our
code because external usage makes it impossible.
Thomas Lübking wrote:
In case there was larger demand for invoking such action (taskbar,
dedicated plasmoid, ...) one could move the xkill functionality into
KWindowSystem (option for portage) - invoking a kwin shortcut through a
kglobalaccel dbus call is a hack. Maybe sufficient for any downstream
solution, but easily broken feature.
Gregor Mi wrote:
First of all, a clarification of this RR's intentions:
1) The original End Process... tooltip says you can always use
Ctrl+Alt+Esc... which is wrong as soon as someone changes the keyboard
shortcut exposed by KWin. So this should be fixed.
2) Make the Kill Window feature more discoverable. It is a seldom used
feature which makes it harder to remember.
About invoking Kill Window:
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself. [Martin]
...one could move the xkill functionality into KWindowSystem...
[Thomas]
Without knowing the amount of xkill code I suspect that having a dbus
call that loosly couples libksysguard to KWin is probably easier to maintain
than 2 times the xkill code.
Having said that, what about moving the xkill code to a common location
as Thomas suggested?
We do not guarantee that we don't change them, we do not guarantee that
they are exposed at all ... I do not want to run into situations that we
cannot change our code because external usage makes it impossible.
Understood. But would it then be possible at all to get the current
shortcut to display it to the user?
Martin Gräßlin wrote:
Ok, so this addresses two issues using one solution: exposing KWin's
internal shortcut. This is bad as outlined above.
I agree that 1) needs fixing. This can be done in the way as approached
in this review request: check whether kwin is registered on kglobalaccel and
get the key command. If it's done that way the fault is with libksysguard in
case KWin changes the shortcut name or doesn't use kglobalaccel any more.
Another fix is of course to just hide the shortcut.
2) is a different issue. Whether it's needed to expose the functionality
in addition from libksysguard is probably questionable. A better approach to
do this would be through a method in KWindowSystem. This does not need to
duplicate the code, it could also just send a client message to the window
manager to start the kill window process. Through KWindowSystem we can check
whether the feature is supported by the window manager and could exclude if
not supported. But and that's a big but: the feature would not be able to
work if it's triggered from a (context) menu or drop down list (it needs to
grab mouse). Given that I'm hesistant to say that it should be added to
kwindowsystem at all.
Thomas Lübking wrote:
ad 2)
I'd have said to rather *move* the code to KWindowSystem and use it from
there by any client (incl. kwin)
This allows porting the solution (in case such is possible on other
systems at all) as well as to invoke the feature unconditionally (ie. instead
of is this kwin? yes? tell kwin to trigger xkill. just trigger the xkill
functionality)
About the popupmenu:
The issue is global, ie. as long as a popup (or other grabber) is
around, the kwin shortcut neither works.
It's kind of the client codes problem to deal w/ a false return (eg.
invoke a timer and/or timered retries)
Gregor Mi wrote:
ad 1) (shortcut)
I could live with adapting (or remove) the shortcut retrieval as soon as
it will not be possible anymore. As long as it is, I would show it. (I
suspect as long as the shortcut is not hard-coded there will be a some way to
get it)
ad 2) (invoke window kill)
I looked a Kwin's source code. For reference, here are the two methods I
found to kill a window:
```
/*!
Kill Window feature, similar to xkill
*/
void Workspace::slotKillWindow()
{
if (m_windowKiller.isNull()) {
m_windowKiller.reset(new KillWindow());
}
m_windowKiller-start();
}
and
/**
* Kills
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown; no visible error. Unit tests succeed. Thanks, Gregor Mi
Review Request 122653: Set permissions for links in remote:, necessary for correct visualization
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122653/ --- Review request for kdelibs. Bugs: 339193 http://bugs.kde.org/show_bug.cgi?id=339193 Repository: kde-runtime Description --- KFileItem uses UDS_ACCESS to determine permissions. Readability is subsequently used to create the list of overlay icons. CCBUG: 339193 Diffs - kioslave/remote/remoteimpl.cpp 5d973c6c1b6c31b7f3107d0d15805ef04bfdd661 Diff: https://git.reviewboard.kde.org/r/122653/diff/ Testing --- dolphin remote: - no lock icon on smb:, mtp:, ... links Thanks, Stefan Brüns
Review Request 122652: Use correct default value when UDS_ACCESS/UDS_FILE_TYPE is not set
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122652/ --- Review request for kdelibs. Bugs: 339193 http://bugs.kde.org/show_bug.cgi?id=339193 Repository: kdelibs Description --- The default value for UDSEntry::numberValue(...) is 0, whereas KFileItem uses the special value KFileItem::Unknown == (mode_t) -1. CCBUG: 339193 Diffs - kio/kio/kfileitem.cpp f431d3608cfe646fb882365921e694af8ff8838f Diff: https://git.reviewboard.kde.org/r/122652/diff/ Testing --- dolphin remote: - no lock icon on smb:, mtp:, ... links Thanks, Stefan Brüns
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 17, 2015, 10:22 p.m., David Edmundson wrote: Thx for looking into it. Since it is quite a large change I'll keep my intermediate commits and push them now. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76201 --- On Feb. 15, 2015, 4:35 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 15, 2015, 4:35 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown; no visible error. Unit tests succeed. Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 8:16 a.m., Hrvoje Senjan wrote: can you check what needs to be adjusted in plasma-workspace? it fails to build with your change: [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(unresolved overloaded function type, QString, QString)' [ 451s] services QExplicitlySharedDataPointerKService(new KService(proc-name, cmdline, QString())); [ 451s] ^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are: [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream, int) [ 451s] KService(QDataStream str, int offset); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString) [ 451s] explicit KService(const KDesktopFile *config, const QString entryPath = QString()); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString) [ 451s] explicit KService(const QString fullpath); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString, const QString, const QString) [ 451s] KService(const QString name, const QString exec, const QString icon); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from 'unresolved overloaded function type' to 'const QString' [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString, const QStringList)': [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. Gregor Mi wrote: How can I find out if which branch was compiled? I assume it is the master branch. How can I find out if which branch was compiled? I assume it is the master branch. Yes its master branch - Bhushan --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 21, 2015, 3:16 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 21, 2015, 3:16 a.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
On Sat, Feb 21, 2015 at 1:34 PM, Alexander Richardson
arichardson@gmail.com wrote:
and then we could also have something like
KServiceTypeTrader::findPlugin(serviceType, name) that expands to
KServiceTypeTrader::self()-query(serviceType, [](const KPluginInfo info) {
return info.pluginName() == name;
}
I have been meaning to add this for quite a while, but I am really
busy at the moment.
So we would maintain such a warning, but start to port users to that
new api instead?
--
Marco Martin
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Feb. 21, 2015, 2:46 a.m., Hrvoje Senjan wrote: can you check what needs to be adjusted in plasma-workspace? it fails to build with your change: [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:471:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: error: no matching function for call to 'KService::KService(unresolved overloaded function type, QString, QString)' [ 451s] services QExplicitlySharedDataPointerKService(new KService(proc-name, cmdline, QString())); [ 451s] ^ [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:501:103: note: candidates are: [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: KService::KService(QDataStream, int) [ 451s] KService(QDataStream str, int offset); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:580:5: note: candidate expects 2 arguments, 3 provided [ 451s] In file included from /usr/include/KF5/KService/KService:1:0, [ 451s] from /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:32: [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: KService::KService(const KDesktopFile*, const QString) [ 451s] explicit KService(const KDesktopFile *config, const QString entryPath = QString()); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:82:14: note: candidate expects 2 arguments, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: KService::KService(const QString) [ 451s] explicit KService(const QString fullpath); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:75:14: note: candidate expects 1 argument, 3 provided [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: KService::KService(const QString, const QString, const QString) [ 451s] KService(const QString name, const QString exec, const QString icon); [ 451s] ^ [ 451s] /usr/include/KF5/KService/kservice.h:68:5: note: no known conversion for argument 1 from 'unresolved overloaded function type' to 'const QString' [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp: In function 'QUrl TaskManager::getServiceLauncherUrl(int, const QString, const QStringList)': [ 451s] /home/abuild/rpmbuild/BUILD/plasma-workspace-5.2.90git/libtaskmanager/taskitem.cpp:516:43: error: 'proc-KSysGuard::Process::command' does not have class type [ 451s] QString cmdline = proc ? proc-command.simplified() : QString(); // proc-command has a trailing space??? [ 451s]^ Gregor Mi wrote: Probably `proc-command` must be replace with `proc-command()`. I'll check that. Gregor Mi wrote: How can I find out if which branch was compiled? I assume it is the master branch. Bhushan Shah wrote: How can I find out if which branch was compiled? I assume it is the master branch. Yes its master branch Aleix and me have fixed plasma-workspace's build. Update the master branch. - Sebastian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review76358 --- On Feb. 20, 2015, 9:46 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Feb. 20, 2015, 9:46 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- This is a follow-up to https://git.reviewboard.kde.org/r/121717/: In process.h there are several public fields which easily trigger BIC changes. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) Diffs - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 processcore/processes_solaris_p.cpp
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
On Sat, Feb 21, 2015 at 3:43 PM, Marco Martin notm...@gmail.com wrote:
On Sat, Feb 21, 2015 at 1:34 PM, Alexander Richardson
arichardson@gmail.com wrote:
and then we could also have something like
KServiceTypeTrader::findPlugin(serviceType, name) that expands to
KServiceTypeTrader::self()-query(serviceType, [](const KPluginInfo info) {
return info.pluginName() == name;
}
anyways, absolutely +1 on this new function, I like the idea a lot :)
--
Marco Martin
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
On Saturday, February 21, 2015 17:19:02 Alexander Richardson wrote:
2015-02-21 14:43 GMT+00:00 Marco Martin notm...@gmail.com:
On Sat, Feb 21, 2015 at 1:34 PM, Alexander Richardson
arichardson@gmail.com wrote:
and then we could also have something like
KServiceTypeTrader::findPlugin(serviceType, name) that expands to
KServiceTypeTrader::self()-query(serviceType, [](const KPluginInfo
info) {
return info.pluginName() == name;
}
I have been meaning to add this for quite a while, but I am really
busy at the moment.
So we would maintain such a warning, but start to port users to that
new api instead?
Yes, that would be my suggestion. I won't be able to work on it before
Thursday though. Ideally there shouldn't be many changes required to
make it work for KMimeTypeTrader aswell, so then in KF5 we can get rid
of all the generated parsing code.
That's completely fine. We're not too much in a hurry, it's not a showstopper
by any means, just some janitorial work to make Plasma a bit easier to debug.
Currently, about 2/3 of its console output consists of these messages.
Thanks, Alex!
--
sebas
Sebastian Kügler|http://vizZzion.org| http://kde.org
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On Feb. 21, 2015, 4:08 p.m., Thomas Lübking wrote: processui/ksysguardprocesslist.cpp, line 367 https://git.reviewboard.kde.org/r/122249/diff/4/?file=350459#file350459line367 leaving aside that the patch is not clean (still contains kglobalaccel stuff, ie. is probably just a variant proposal): I don't have xkill installed. It's not a dependency of anything in KDE - you'd have to add it (for package maintainers, no idea how to do that, though) (ftr: the popup grabs mouse limitations mostly hold for invoking xkill as well - just that there it would become harder to check for successful invocation) kglobalaccel: I thought this would be ok to get the global shortcut for the killing action. xkill: ah, ok. I thought it comes with X. I'll remove it. popup grabs mouse limitation: I am not that familiar with that. How would that affect an xill invocation? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review76389 --- On Feb. 20, 2015, 11:35 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Feb. 20, 2015, 11:35 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. RR: Add a drop down menu to the End Process... button with one action: i18n(Kill a specific window... (Global shortcut: %1), killWindowShortcut) Diffs - 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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On Jan. 26, 2015, 7:05 a.m., Martin Gräßlin wrote:
My opinion is that this is a feature which should not be exposed in
libksysguard. It actually ties libksysguard to KWin, while libksysguard was
in the past also used in e.g. kdevelop.
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself.
Martin Gräßlin wrote:
In addition: KWin's global shortcut action names are not public API. We
do not guarantee that we don't change them, we do not guarantee that they are
exposed at all (KWin handling shortcuts internally without kglobalaccel on
Wayland?). I do not want to run into situations that we cannot change our
code because external usage makes it impossible.
Thomas Lübking wrote:
In case there was larger demand for invoking such action (taskbar,
dedicated plasmoid, ...) one could move the xkill functionality into
KWindowSystem (option for portage) - invoking a kwin shortcut through a
kglobalaccel dbus call is a hack. Maybe sufficient for any downstream
solution, but easily broken feature.
Gregor Mi wrote:
First of all, a clarification of this RR's intentions:
1) The original End Process... tooltip says you can always use
Ctrl+Alt+Esc... which is wrong as soon as someone changes the keyboard
shortcut exposed by KWin. So this should be fixed.
2) Make the Kill Window feature more discoverable. It is a seldom used
feature which makes it harder to remember.
About invoking Kill Window:
If libksysguard wants to offer the functionality to kill a window, it
should implement it itself. [Martin]
...one could move the xkill functionality into KWindowSystem...
[Thomas]
Without knowing the amount of xkill code I suspect that having a dbus
call that loosly couples libksysguard to KWin is probably easier to maintain
than 2 times the xkill code.
Having said that, what about moving the xkill code to a common location
as Thomas suggested?
We do not guarantee that we don't change them, we do not guarantee that
they are exposed at all ... I do not want to run into situations that we
cannot change our code because external usage makes it impossible.
Understood. But would it then be possible at all to get the current
shortcut to display it to the user?
Martin Gräßlin wrote:
Ok, so this addresses two issues using one solution: exposing KWin's
internal shortcut. This is bad as outlined above.
I agree that 1) needs fixing. This can be done in the way as approached
in this review request: check whether kwin is registered on kglobalaccel and
get the key command. If it's done that way the fault is with libksysguard in
case KWin changes the shortcut name or doesn't use kglobalaccel any more.
Another fix is of course to just hide the shortcut.
2) is a different issue. Whether it's needed to expose the functionality
in addition from libksysguard is probably questionable. A better approach to
do this would be through a method in KWindowSystem. This does not need to
duplicate the code, it could also just send a client message to the window
manager to start the kill window process. Through KWindowSystem we can check
whether the feature is supported by the window manager and could exclude if
not supported. But and that's a big but: the feature would not be able to
work if it's triggered from a (context) menu or drop down list (it needs to
grab mouse). Given that I'm hesistant to say that it should be added to
kwindowsystem at all.
Thomas Lübking wrote:
ad 2)
I'd have said to rather *move* the code to KWindowSystem and use it from
there by any client (incl. kwin)
This allows porting the solution (in case such is possible on other
systems at all) as well as to invoke the feature unconditionally (ie. instead
of is this kwin? yes? tell kwin to trigger xkill. just trigger the xkill
functionality)
About the popupmenu:
The issue is global, ie. as long as a popup (or other grabber) is
around, the kwin shortcut neither works.
It's kind of the client codes problem to deal w/ a false return (eg.
invoke a timer and/or timered retries)
Gregor Mi wrote:
ad 1) (shortcut)
I could live with adapting (or remove) the shortcut retrieval as soon as
it will not be possible anymore. As long as it is, I would show it. (I
suspect as long as the shortcut is not hard-coded there will be a some way to
get it)
ad 2) (invoke window kill)
I looked a Kwin's source code. For reference, here are the two methods I
found to kill a window:
```
/*!
Kill Window feature, similar to xkill
*/
void Workspace::slotKillWindow()
{
if (m_windowKiller.isNull()) {
m_windowKiller.reset(new KillWindow());
}
m_windowKiller-start();
}
and
/**
* Kills
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
2015-02-21 10:02 GMT+00:00 Marco Martin notm...@gmail.com:
Hi all,
As you may have noticed, right now starting plasma is a big spam of
the following error:
Calling KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated,
use KPluginInfo::pluginName() in /whatever/plugin.so instead.
i tried to see where it happens, and seems it's in
ktradeparsetree.cpp , line 30
QVariant ParseContext::property(const QString _key) const
I don't think this properly fixable, since from the stack trace it
seems an appropriate use.. i see two ways to fix it:
1) in ParseContext::property stuf a very long if.. else.. that makes
it call the proper KPluginInfo::correctAccessor() .. but is ugly and
slows it down
2) since this is an appropriate use, consider it not wrong anymore,
and just get rid of the warning.
Opinions? ideas?
I guess most of these would result from a call to
KServiceTypeTrader::self()-query(KMyApp/Plugin,
[X-KDE-PluginInfo-Name] = Foo).
My suggestion would be to add an overload to
KServiceTypeTrader::query() that takes a std::functionbool(const
KPluginInfo) instead of the constraints string.
Ideally we could then deprecate the string based version and use the
std::function version everywhere since that should be safer (and
faster).
KServiceTypeTrader::self()-query(KMyApp/Plugin, [](const
KPluginInfo info) {
return info.property(X-KMyApp-InterfaceVersion).toInt() 15;
}
instead of
KServiceTypeTrader::self()-query(KMyApp/Plugin,
[X-KMyApp-InterfaceVersion] 15);
and then we could also have something like
KServiceTypeTrader::findPlugin(serviceType, name) that expands to
KServiceTypeTrader::self()-query(serviceType, [](const KPluginInfo info) {
return info.pluginName() == name;
}
I have been meaning to add this for quite a while, but I am really
busy at the moment.
Alex
Re: Warning: KPluginInfo::property(X-KDE-PluginInfo-Name) is deprecated
2015-02-21 14:43 GMT+00:00 Marco Martin notm...@gmail.com:
On Sat, Feb 21, 2015 at 1:34 PM, Alexander Richardson
arichardson@gmail.com wrote:
and then we could also have something like
KServiceTypeTrader::findPlugin(serviceType, name) that expands to
KServiceTypeTrader::self()-query(serviceType, [](const KPluginInfo info) {
return info.pluginName() == name;
}
I have been meaning to add this for quite a while, but I am really
busy at the moment.
So we would maintain such a warning, but start to port users to that
new api instead?
Yes, that would be my suggestion. I won't be able to work on it before
Thursday though. Ideally there shouldn't be many changes required to
make it work for KMimeTypeTrader aswell, so then in KF5 we can get rid
of all the generated parsing code.
Alex
