Re: Review Request 128854: Add a Tools button above the process list of System Monitor
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/ --- (Updated Feb. 4, 2018, 3:14 nachm.) Status -- This change has been discarded. Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, Thomas Lübking, and Thomas Pfeiffer. Repository: libksysguard Description --- This adds a new "Tools" button to the ksysguard widget which contains entries to related tools: - Run Command --> starts KRunner to execute a command - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts the lightweight System Monitor which has less features) - Info Center --> starts the Info Center which shows additional system information - Htop --> starts htop - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. The displayed shortcut is the one set in Global Keyboard Shortcuts. Currently, this shortcut is shown in the End Process... button tooltip but there it is hard-coded. This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ Diffs - CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 Diff: https://git.reviewboard.kde.org/r/128854/diff/5/ Testing --- - test if all tools start correctly - check if a changed shortcut for Kill Window is shown in the tools menu (see screenshot) - check if for example htop is not installed that there is no menu item for it File Attachments screenshot of the new Tools button https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png Thanks, Gregor Mi
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/#review103862 --- Migrated to Phabricator: https://phabricator.kde.org/D10297 - Gregor Mi On Aug. 3, 2017, 1:25 nachm., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128854/ > --- > > (Updated Aug. 3, 2017, 1:25 nachm.) > > > Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, > Thomas Lübking, and Thomas Pfeiffer. > > > Repository: libksysguard > > > Description > --- > > This adds a new "Tools" button to the ksysguard widget which contains entries > to related tools: > > - Run Command --> starts KRunner to execute a command > - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts > the lightweight System Monitor which has less features) > - Info Center --> starts the Info Center which shows additional system > information > - Htop --> starts htop > - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. > The displayed shortcut is the one set in Global Keyboard Shortcuts. > Currently, this shortcut is shown in the End Process... button tooltip but > there it is hard-coded. > > This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ > > > Diffs > - > > CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 > processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 > > > Diff: https://git.reviewboard.kde.org/r/128854/diff/5/ > > > Testing > --- > > - test if all tools start correctly > - check if a changed shortcut for Kill Window is shown in the tools menu (see > screenshot) > - check if for example htop is not installed that there is no menu item for it > > > File Attachments > > > screenshot of the new Tools button > > https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png > > > Thanks, > > Gregor Mi > >
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/ --- (Updated Aug. 3, 2017, 1:25 p.m.) Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, Thomas Lübking, and Thomas Pfeiffer. Changes --- just capture "this" Repository: libksysguard Description --- This adds a new "Tools" button to the ksysguard widget which contains entries to related tools: - Run Command --> starts KRunner to execute a command - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts the lightweight System Monitor which has less features) - Info Center --> starts the Info Center which shows additional system information - Htop --> starts htop - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. The displayed shortcut is the one set in Global Keyboard Shortcuts. Currently, this shortcut is shown in the End Process... button tooltip but there it is hard-coded. This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ Diffs (updated) - CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 Diff: https://git.reviewboard.kde.org/r/128854/diff/ Testing --- - test if all tools start correctly - check if a changed shortcut for Kill Window is shown in the tools menu (see screenshot) - check if for example htop is not installed that there is no menu item for it File Attachments screenshot of the new Tools button https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png Thanks, Gregor Mi
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/ --- (Updated Aug. 3, 2017, 11:08 a.m.) Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, Thomas Lübking, and Thomas Pfeiffer. Changes --- * The Plasma requirement in CMakeLists.txt was not needed Repository: libksysguard Description --- This adds a new "Tools" button to the ksysguard widget which contains entries to related tools: - Run Command --> starts KRunner to execute a command - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts the lightweight System Monitor which has less features) - Info Center --> starts the Info Center which shows additional system information - Htop --> starts htop - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. The displayed shortcut is the one set in Global Keyboard Shortcuts. Currently, this shortcut is shown in the End Process... button tooltip but there it is hard-coded. This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ Diffs (updated) - CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 Diff: https://git.reviewboard.kde.org/r/128854/diff/ Testing --- - test if all tools start correctly - check if a changed shortcut for Kill Window is shown in the tools menu (see screenshot) - check if for example htop is not installed that there is no menu item for it File Attachments screenshot of the new Tools button https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png Thanks, Gregor Mi
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
> On July 31, 2017, 9:11 a.m., Kevin Funk wrote: > > processui/ksysguardprocesslist.cpp, line 354 > > <https://git.reviewboard.kde.org/r/128854/diff/3/?file=498252#file498252line354> > > > > Why this? When I try to capture "d", then I get the following compiler error: processui/ksysguardprocesslist.cpp:355:36: error: capture of non-variable ‘KSysGuardProcessList::d’ I also find the current solution a bit strange. See here: https://stackoverflow.com/questions/12944002/capture-by-value-class-members (C++14 might solve the issue) - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/#review103512 ------- On July 28, 2017, 12:20 p.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128854/ > --- > > (Updated July 28, 2017, 12:20 p.m.) > > > Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, > Thomas Lübking, and Thomas Pfeiffer. > > > Repository: libksysguard > > > Description > --- > > This adds a new "Tools" button to the ksysguard widget which contains entries > to related tools: > > - Run Command --> starts KRunner to execute a command > - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts > the lightweight System Monitor which has less features) > - Info Center --> starts the Info Center which shows additional system > information > - Htop --> starts htop > - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. > The displayed shortcut is the one set in Global Keyboard Shortcuts. > Currently, this shortcut is shown in the End Process... button tooltip but > there it is hard-coded. > > This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ > > > Diffs > - > > CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 > processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 > > Diff: https://git.reviewboard.kde.org/r/128854/diff/ > > > Testing > --- > > - test if all tools start correctly > - check if a changed shortcut for Kill Window is shown in the tools menu (see > screenshot) > - check if for example htop is not installed that there is no menu item for it > > > File Attachments > > > screenshot of the new Tools button > > https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png > > > Thanks, > > Gregor Mi > >
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
> On July 31, 2017, 9:11 a.m., Kevin Funk wrote: > > CMakeLists.txt, line 21 > > <https://git.reviewboard.kde.org/r/128854/diff/3/?file=498249#file498249line21> > > > > libksysguard should not require Plasma, it should stay optional at best. > > > > See one line below in that CMakeLists.txt... You are right. The Plasma requirement is not needed. I'll remove it. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/#review103512 ----------- On July 28, 2017, 12:20 p.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128854/ > --- > > (Updated July 28, 2017, 12:20 p.m.) > > > Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, > Thomas Lübking, and Thomas Pfeiffer. > > > Repository: libksysguard > > > Description > --- > > This adds a new "Tools" button to the ksysguard widget which contains entries > to related tools: > > - Run Command --> starts KRunner to execute a command > - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts > the lightweight System Monitor which has less features) > - Info Center --> starts the Info Center which shows additional system > information > - Htop --> starts htop > - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. > The displayed shortcut is the one set in Global Keyboard Shortcuts. > Currently, this shortcut is shown in the End Process... button tooltip but > there it is hard-coded. > > This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ > > > Diffs > - > > CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 > processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 > > Diff: https://git.reviewboard.kde.org/r/128854/diff/ > > > Testing > --- > > - test if all tools start correctly > - check if a changed shortcut for Kill Window is shown in the tools menu (see > screenshot) > - check if for example htop is not installed that there is no menu item for it > > > File Attachments > > > screenshot of the new Tools button > > https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png > > > Thanks, > > Gregor Mi > >
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
> On Sept. 8, 2016, 8:26 a.m., Martin Flöser wrote: > > processui/ksysguardprocesslist.cpp, lines 403-413 > > <https://git.reviewboard.kde.org/r/128854/diff/1/?file=476424#file476424line403> > > > > KWin does install the dbus xml file. You could use that to generate the > > code through QDbus "magic" Thanks for the review and for this hint. I'll do that maybe later. I am currently not that familiar with the QDbus magic :) - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/#review98994 ----------- On July 28, 2017, 12:20 p.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128854/ > --- > > (Updated July 28, 2017, 12:20 p.m.) > > > Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, > Thomas Lübking, and Thomas Pfeiffer. > > > Repository: libksysguard > > > Description > --- > > This adds a new "Tools" button to the ksysguard widget which contains entries > to related tools: > > - Run Command --> starts KRunner to execute a command > - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts > the lightweight System Monitor which has less features) > - Info Center --> starts the Info Center which shows additional system > information > - Htop --> starts htop > - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. > The displayed shortcut is the one set in Global Keyboard Shortcuts. > Currently, this shortcut is shown in the End Process... button tooltip but > there it is hard-coded. > > This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ > > > Diffs > - > > CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 > processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 > > Diff: https://git.reviewboard.kde.org/r/128854/diff/ > > > Testing > --- > > - test if all tools start correctly > - check if a changed shortcut for Kill Window is shown in the tools menu (see > screenshot) > - check if for example htop is not installed that there is no menu item for it > > > File Attachments > > > screenshot of the new Tools button > > https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png > > > Thanks, > > Gregor Mi > >
Re: Review Request 128854: Add a Tools button above the process list of System Monitor
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128854/ --- (Updated July 28, 2017, 12:20 p.m.) Review request for KDE Base Apps, Martin Flöser, John Tapsell, Ken Vermette, Thomas Lübking, and Thomas Pfeiffer. Changes --- Sorry for the late reply. I fixed the mentioned issues: * use QStringLiteral on several places * add context for translation * remove duplicate code by adding a function * Add additional tools to the Tools menu: Konsole, KSystemLog, Filelight, Sweeper, KMag Repository: libksysguard Description --- This adds a new "Tools" button to the ksysguard widget which contains entries to related tools: - Run Command --> starts KRunner to execute a command - KSysguard --> starts KSysguard (this is useful because Ctrl+Esc only starts the lightweight System Monitor which has less features) - Info Center --> starts the Info Center which shows additional system information - Htop --> starts htop - Kill a window (Ctrl+Alt+Esc) --> triggers the KWin kill window function. The displayed shortcut is the one set in Global Keyboard Shortcuts. Currently, this shortcut is shown in the End Process... button tooltip but there it is hard-coded. This RR replaces this old RR https://git.reviewboard.kde.org/r/122249/ Diffs (updated) - CMakeLists.txt 84a447990563cf1605c06989b255627e99ef31b3 processui/CMakeLists.txt 1927839282a6282c824f5bbb5d35ea94075f428e processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 79e9bb3c4e9d481a03d2e5992ab1c0692a06de28 Diff: https://git.reviewboard.kde.org/r/128854/diff/ Testing --- - test if all tools start correctly - check if a changed shortcut for Kill Window is shown in the tools menu (see screenshot) - check if for example htop is not installed that there is no menu item for it File Attachments screenshot of the new Tools button https://git.reviewboard.kde.org/media/uploaded/files/2016/09/07/ac70c386-6665-4898-94e7-b256759f5a5e__screenshot_20160907_140935.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly*
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly*
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 Jan. 24, 2016, 11:09 a.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. Changes --- add 1 screenshot and 1 mockup 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- File Attachments (updated) DEFERRED: 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 DEFERRED: new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png Ctrl+Esc, currently https://git.reviewboard.kde.org/media/uploaded/files/2016/01/24/8d537357-1e08-4d67-8b45-28933c1a0ccf__screenshot_20160124_115024.png Ctrl+Esc, with Tools... (menu contents see discussion) https://git.reviewboard.kde.org/media/uploaded/files/2016/01/24/71ebdcea-a793-460a-adb5-f05a7706f8b0__screenshot_20160124_115024-with-tools.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly*
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) 2. "Put it into the tooltip": Yes, sure this can and should be done. But also see point 5. 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly encouraged in the HIG. My mistake, thanks for clearing that! 4a. "Clicking a button": the original idea was that clicking the button actually triggers the function but for current technical reasons it was postponed. So, adding the menu item now might motivate someone to go a step further and implement the rest. One step at a time. 4b. "only used a single time": I myself am a user and due to the outstanding stability of the desktop I keep forgetting these shortcuts. ;-) Seriously, often I found myself in a situation where I knew there is a shortcut but could not remember which (and previously I was lost completely because I did not know that the killing window function exists at all). By now I think I will never forget it, though. :) 5. Funnily, the first time I actually saw that the shortcut is documented in the toolip was when I began to change the code to prepare this RR to make the shortcut more visible. :) Sure, this only a single user report but I don't think I am the only who does not read the whole tooltip. - Gregor ------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review91486 --- On April 20, 2015, 10:24 p.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122249/ > --- > > (Updated April 20, 2015, 10:24 p.m.) > > > Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas > Pfeiffer. > > > 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 66899e577a03786d894423a8f1ce5b3aeed6de8a > processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c > > 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 > new submenu > > https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png > > > Thanks, > > Gregor Mi > >
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/#review91486 --- > If someone has changed the shortcut, they should know what shortcut they set > it to, right? So having the tooltip just say "To kill a specific window, > press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" should do the > trick, right? In principle, I agree with you here. :) But... I have these premises in mind: 1. There might be a situation where the user can't remember what he has done. 2. I prefer precise over approximate information if it is reasonable easy to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now easily possible and the patch is already written) 3. Somewhere I read that tooltips are to be avoided where possible => this patch factors some information out of a tooltip which in itself is desirable. 4. For the user, the discoverability of the feature after applying this patch is better than before (though not perfect, sure). - Gregor Mi On April 20, 2015, 10:24 p.m., Gregor Mi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122249/ > --- > > (Updated April 20, 2015, 10:24 p.m.) > > > Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas > Pfeiffer. > > > 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 66899e577a03786d894423a8f1ce5b3aeed6de8a > processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 > processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 > processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c > > 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 > new submenu > > https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png > > > Thanks, > > Gregor Mi > >
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: > > What would likely be confusing is that the two button modes have different > > interaction flows: The "End Process" mode requires to first select a > > process and then press the button to work, whereas the "Kill specific > > window" mode requires to first press the button and then select the window > > to kill, and users have no easy way to understand how each one works and > > why they work differently. > > > > The ellipsis in the label "End Process..." adds to that confusion. It > > indicates that further input is necessary before the action can take > > effect. While the button does open a dialog to confirm killing the selected > > process, ellipses are actually reserved for actions where a dialog asks for > > new information, such as the "Save As..." button, not for actions that > > require confirmation. > > > > To avoid this confusion, it should be possible to also click "End > > Process..." even if no processes has been selected, whuch would then ask > > the user to select the process to kill. This could theoretically be done > > similarly to the "Kill specific window" function: Click the "End > > Process..." button and then click the process in the list that you want to > > end. Alternatively, if no process had been selected when "End Process..." > > is clicked, a dialog could be opened where the process to kill would be > > selected. Of course the current flow of ending a process could and should > > still work. > > Gregor Mi wrote: > Thanks for the feedback! The ellipsis in "End Process..." is the original > design. According to your explanation this was wrong in the first place. What > about removing the ellipses in both menu items so we will end up with "End > Process" and "Kill a specific window"? > > Thomas Pfeiffer wrote: > "Kill specific window" does always need additional input until it really > does something, doesn't it? As I understood it merely changes the cursor to > the kill cursor and then the user has to select the window to kill, right? > > Gregor Mi wrote: > Erm, right. To be exact, "Kill specific window..." only shows a message > box. But in the end - after the user presses the keyboard shortcut - the user > has to select a window. So this seems to be a special case. The intention > behind all this is to increase the discoverability of the hidden xkill > feature. > > Gregor Mi wrote: > > To avoid this confusion, it should be possible to also click "End > Process..." even if no processes has been selected, whuch would then ask the > user to select the process to kill. > > If "End Process" is clicked with no processes selected, there will be a > message box which says that the user has to select one more more processes > first. > > > This could theoretically be done similarly to the "Kill specific > window" function: Click the "End Process..." button and then click the > process in the list that you want to end. > > I think "Kill specific windows" should be considered as the special case > here. Changing or extending the "End Process" workflow would introduce more > complexity to the code. > > > Alternatively, if no process had been selected when "End Process..." is > clicked, a dialog could be opened where the process to kill would be > selected. Of course the current flow of ending a p>rocess could and should > still work. > > This would be a topic for another RR. > > Summary of final changes for this RR: > > - I would change the "End Process..." to "End Process" (remove ellipsis). > Ok? > - I am not sure if the ellipsis of "Kill specific window..." should be > removed or not. > > Thomas Pfeiffer wrote: > To be honest: The more I think about this feature, the less sense it > makes to me in general. > What is XKill's usecase anyway? If an application works normally, one can > just quit it normally. If an application is frozen, KWin quite reliably > detects that when trying to close the window and allows to kill the process. > Usually "End process" is used for applications that do not have a window > (either because they don't have a GUI in the first place or because the > window has disappeared but the process is still there), in which case xkill > would not help anyway. > Yes, there may be some s
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly*
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly*
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: > > What would likely be confusing is that the two button modes have different > > interaction flows: The "End Process" mode requires to first select a > > process and then press the button to work, whereas the "Kill specific > > window" mode requires to first press the button and then select the window > > to kill, and users have no easy way to understand how each one works and > > why they work differently. > > > > The ellipsis in the label "End Process..." adds to that confusion. It > > indicates that further input is necessary before the action can take > > effect. While the button does open a dialog to confirm killing the selected > > process, ellipses are actually reserved for actions where a dialog asks for > > new information, such as the "Save As..." button, not for actions that > > require confirmation. > > > > To avoid this confusion, it should be possible to also click "End > > Process..." even if no processes has been selected, whuch would then ask > > the user to select the process to kill. This could theoretically be done > > similarly to the "Kill specific window" function: Click the "End > > Process..." button and then click the process in the list that you want to > > end. Alternatively, if no process had been selected when "End Process..." > > is clicked, a dialog could be opened where the process to kill would be > > selected. Of course the current flow of ending a process could and should > > still work. > > Gregor Mi wrote: > Thanks for the feedback! The ellipsis in "End Process..." is the original > design. According to your explanation this was wrong in the first place. What > about removing the ellipses in both menu items so we will end up with "End > Process" and "Kill a specific window"? > > Thomas Pfeiffer wrote: > "Kill specific window" does always need additional input until it really > does something, doesn't it? As I understood it merely changes the cursor to > the kill cursor and then the user has to select the window to kill, right? > > Gregor Mi wrote: > Erm, right. To be exact, "Kill specific window..." only shows a message > box. But in the end - after the user presses the keyboard shortcut - the user > has to select a window. So this seems to be a special case. The intention > behind all this is to increase the discoverability of the hidden xkill > feature. > > Gregor Mi wrote: > > To avoid this confusion, it should be possible to also click "End > Process..." even if no processes has been selected, whuch would then ask the > user to select the process to kill. > > If "End Process" is clicked with no processes selected, there will be a > message box which says that the user has to select one more more processes > first. > > > This could theoretically be done similarly to the "Kill specific > window" function: Click the "End Process..." button and then click the > process in the list that you want to end. > > I think "Kill specific windows" should be considered as the special case > here. Changing or extending the "End Process" workflow would introduce more > complexity to the code. > > > Alternatively, if no process had been selected when "End Process..." is > clicked, a dialog could be opened where the process to kill would be > selected. Of course the current flow of ending a p>rocess could and should > still work. > > This would be a topic for another RR. > > Summary of final changes for this RR: > > - I would change the "End Process..." to "End Process" (remove ellipsis). > Ok? > - I am not sure if the ellipsis of "Kill specific window..." should be > removed or not. > > Thomas Pfeiffer wrote: > To be honest: The more I think about this feature, the less sense it > makes to me in general. > What is XKill's usecase anyway? If an application works normally, one can > just quit it normally. If an application is frozen, KWin quite reliably > detects that when trying to close the window and allows to kill the process. > Usually "End process" is used for applications that do not have a window > (either because they don't have a GUI in the first place or because the > window has disappeared but the process is still there), in which case xkill > would not help anyway. > Yes, there may be some s
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. Gregor Mi wrote: To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. Thomas Pfeiffer wrote: To be honest: The more I think about this feature, the less sense it makes to me in general. What is XKill's usecase anyway? If an application works normally, one can just quit it normally. If an application is frozen, KWin quite reliably detects that when trying to close the window and allows to kill the process. Usually End process is used for applications that do not have a window (either because they don't have a GUI in the first place or because the window has disappeared but the process is still there), in which case xkill would not help anyway. Yes, there may be some situations where it's useful, but they are really corner cases. Yes, very few people know it's there, but very few people miss the function, either. I heve never wished there was a way to hard kill a window without using the Close button in the windeco, and I have never met one who did. So we're complicating the GUI with a split button only to explain a feature to users which the vast majority of them don't need in the first place. If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please tell me about it. Martin Gräßlin wrote: If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. Gregor Mi wrote: To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. Thomas Pfeiffer wrote: To be honest: The more I think about this feature, the less sense it makes to me in general. What is XKill's usecase anyway? If an application works normally, one can just quit it normally. If an application is frozen, KWin quite reliably detects that when trying to close the window and allows to kill the process. Usually End process is used for applications that do not have a window (either because they don't have a GUI in the first place or because the window has disappeared but the process is still there), in which case xkill would not help anyway. Yes, there may be some situations where it's useful, but they are really corner cases. Yes, very few people know it's there, but very few people miss the function, either. I heve never wished there was a way to hard kill a window without using the Close button in the windeco, and I have never met one who did. So we're complicating the GUI with a split button only to explain a feature to users which the vast majority of them don't need in the first place. If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please tell me about it. Martin Gräßlin wrote: If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. Gregor Mi wrote: To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. Thomas Pfeiffer wrote: To be honest: The more I think about this feature, the less sense it makes to me in general. What is XKill's usecase anyway? If an application works normally, one can just quit it normally. If an application is frozen, KWin quite reliably detects that when trying to close the window and allows to kill the process. Usually End process is used for applications that do not have a window (either because they don't have a GUI in the first place or because the window has disappeared but the process is still there), in which case xkill would not help anyway. Yes, there may be some situations where it's useful, but they are really corner cases. Yes, very few people know it's there, but very few people miss the function, either. I heve never wished there was a way to hard kill a window without using the Close button in the windeco, and I have never met one who did. So we're complicating the GUI with a split button only to explain a feature to users which the vast majority of them don't need in the first place. If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please tell me about it. Martin Gräßlin wrote: If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. Gregor Mi wrote: To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. Thomas Pfeiffer wrote: To be honest: The more I think about this feature, the less sense it makes to me in general. What is XKill's usecase anyway? If an application works normally, one can just quit it normally. If an application is frozen, KWin quite reliably detects that when trying to close the window and allows to kill the process. Usually End process is used for applications that do not have a window (either because they don't have a GUI in the first place or because the window has disappeared but the process is still there), in which case xkill would not help anyway. Yes, there may be some situations where it's useful, but they are really corner cases. Yes, very few people know it's there, but very few people miss the function, either. I heve never wished there was a way to hard kill a window without using the Close button in the windeco, and I have never met one who did. So we're complicating the GUI with a split button only to explain a feature to users which the vast majority of them don't need in the first place. If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please tell me about it. Martin Gräßlin wrote: If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. Gregor Mi wrote: To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. Thomas Pfeiffer wrote: To be honest: The more I think about this feature, the less sense it makes to me in general. What is XKill's usecase anyway? If an application works normally, one can just quit it normally. If an application is frozen, KWin quite reliably detects that when trying to close the window and allows to kill the process. Usually End process is used for applications that do not have a window (either because they don't have a GUI in the first place or because the window has disappeared but the process is still there), in which case xkill would not help anyway. Yes, there may be some situations where it's useful, but they are really corner cases. Yes, very few people know it's there, but very few people miss the function, either. I heve never wished there was a way to hard kill a window without using the Close button in the windeco, and I have never met one who did. So we're complicating the GUI with a split button only to explain a feature to users which the vast majority of them don't need in the first place. If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please tell me about it. Martin Gräßlin wrote: If I have missed the killer usecase for xkill which makes it necessary to make it a lot more visible, please
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Gregor Mi wrote: Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. If End Process is clicked with no processes selected, there will be a message box which says that the user has to select one more more processes first. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. I think Kill specific windows should be considered as the special case here. Changing or extending the End Process workflow would introduce more complexity to the code. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. This would be a topic for another RR. Summary of final changes for this RR: - I would change the End Process... to End Process (remove ellipsis). Ok? - I am not sure if the ellipsis of Kill specific window... should be removed or not. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79262 --- On April 20, 2015, 10:24 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 20, 2015, 10:24 p.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- File Attachments New End Process button with drop down arrow https
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Gregor Mi wrote: Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? Thomas Pfeiffer wrote: Kill specific window does always need additional input until it really does something, doesn't it? As I understood it merely changes the cursor to the kill cursor and then the user has to select the window to kill, right? Erm, right. To be exact, Kill specific window... only shows a message box. But in the end - after the user presses the keyboard shortcut - the user has to select a window. So this seems to be a special case. The intention behind all this is to increase the discoverability of the hidden xkill feature. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79262 --- On April 20, 2015, 10:24 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 20, 2015, 10:24 p.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png Thanks, Gregor Mi
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 April 20, 2015, 10:24 p.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. Changes --- Remove message box screenshot because the text changed. There are two different texts depending if KWin is used or not. Message box without KWin: ``` Typically, the kill window method can be invoked with the global shortcut Ctrl+Alt+Esc. The mouse cursor will then change into a deadly skull or some other pirate symbol. If you left-click on a window, the corresponding process will be killed - and unsaved data be lost. Typically, with right-click you can abort. ``` Message box text with KWin: ``` The kill window method can be invoked with a global shortcut which is currently set to: Ctrl+Alt+Esc The mouse cursor will then change into a deadly skull or some other pirate symbol. If you left-click on a window, the corresponding process will be killed - and unsaved data be lost. With right-click or the Esc key you can abort. ``` 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- File Attachments (updated) 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 20, 2015, 10:36 p.m., Thomas Pfeiffer wrote: What would likely be confusing is that the two button modes have different interaction flows: The End Process mode requires to first select a process and then press the button to work, whereas the Kill specific window mode requires to first press the button and then select the window to kill, and users have no easy way to understand how each one works and why they work differently. The ellipsis in the label End Process... adds to that confusion. It indicates that further input is necessary before the action can take effect. While the button does open a dialog to confirm killing the selected process, ellipses are actually reserved for actions where a dialog asks for new information, such as the Save As... button, not for actions that require confirmation. To avoid this confusion, it should be possible to also click End Process... even if no processes has been selected, whuch would then ask the user to select the process to kill. This could theoretically be done similarly to the Kill specific window function: Click the End Process... button and then click the process in the list that you want to end. Alternatively, if no process had been selected when End Process... is clicked, a dialog could be opened where the process to kill would be selected. Of course the current flow of ending a process could and should still work. Thanks for the feedback! The ellipsis in End Process... is the original design. According to your explanation this was wrong in the first place. What about removing the ellipses in both menu items so we will end up with End Process and Kill a specific window? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79262 --- On April 20, 2015, 10:24 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 20, 2015, 10:24 p.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png Thanks, Gregor Mi
Re: KF5-QT5 branchgroup - jobs that need developeer attention
skanlite - all - wants a framework KF5Sane (does this exist?) Gregor Mi ported both libksane and Skanlite to KF5 last year. They are both in the frameworks branch. libksane is not yet a framework, but I think he prepared libksane for it. For me Skanlite compiles fine with kdesrc-build... As I tried a few weeks ago, it compiled for me, too. Did not do a recent build though. I have plans to split libksane into a non-gui and a gui library and it could be good to wait a bit with trying to get it into frameworks for BIC reasons. That said I think it would be nice to have a KF5 based release of both libksane and skanlite ASAP.
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 16, 2015, 3:14 p.m., Martin Gräßlin wrote: CMakeLists.txt, lines 22-33 https://git.reviewboard.kde.org/r/122249/diff/9/?file=360665#file360665line22 do we still need all those? I only see GlobalAccel used as a new dependency or am I missing something? Previously those were all in one line. Effectively only GlobalAccel was added. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79059 --- On April 17, 2015, 7:25 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 17, 2015, 7:25 a.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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
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 April 17, 2015, 7:25 a.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- - add translation hint - fiex message box text - remove empty line 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 (updated) - CMakeLists.txt 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 17, 2015, 7:31 a.m., Martin Gräßlin wrote: I'm still not sure about the text for the message box: could you please add Thomas Pfeiffer from Usability team to the review to get him comment on it? Sure. I'll add him. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79097 --- On April 17, 2015, 7:25 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 17, 2015, 7:25 a.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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 17, 2015, 7:31 a.m., Martin Gräßlin wrote: I'm still not sure about the text for the message box: could you please add Thomas Pfeiffer from Usability team to the review to get him comment on it? Gregor Mi wrote: Sure. I'll add him. I updated the text. Message box without KWin: ``` Typically, the kill window method can be invoked with the global shortcut Ctrl+Alt+Esc. The mouse cursor will then change into a deadly skull or some other pirate symbol. If you left-click on a window, the corresponding process will be killed - and unsaved data be lost. Typically, with right-click you can abort. ``` Message box text with KWin: ``` The kill window method can be invoked with a global shortcut which is currently set to: Ctrl+Alt+Esc The mouse cursor will then change into a deadly skull or some other pirate symbol. If you left-click on a window, the corresponding process will be killed - and unsaved data be lost. With right-click or the Esc key you can abort. ``` - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79097 --- On April 17, 2015, 7:32 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 17, 2015, 7:32 a.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 16, 2015, 3:14 p.m., Martin Gräßlin wrote: processui/ksysguardprocesslist.cpp, line 359 https://git.reviewboard.kde.org/r/122249/diff/9/?file=360668#file360668line359 You could know whether kwin is used - the window manager name is exported to the root window. Users might not know what KWin is. I also would like that better. I tried - QApplication::desktop()-window()-windowTitle(); - QApplication::desktop()-windowTitle(); Any other hint on how access this information? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79059 --- On April 17, 2015, 7:32 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 17, 2015, 7:32 a.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
On April 16, 2015, 3:14 p.m., Martin Gräßlin wrote: processui/ksysguardprocesslist.cpp, line 359 https://git.reviewboard.kde.org/r/122249/diff/9/?file=360668#file360668line359 You could know whether kwin is used - the window manager name is exported to the root window. Users might not know what KWin is. Gregor Mi wrote: I also would like that better. I tried - QApplication::desktop()-window()-windowTitle(); - QApplication::desktop()-windowTitle(); Any other hint on how access this information? Thomas Lübking wrote: http://api.kde.org/frameworks-api/frameworks5-apidocs/kwindowsystem/html/classNETRootInfo.html#ad6269ee8e82664340b1ac5dead86991f Ah, thanks. I found that NETRootInfo(QX11Info::connection(), NET::WMAllProperties).wmName() works. Though I had expected that NET::WMName instead of NET::WMAllProperties should also work, no? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review79059 --- On April 17, 2015, 7:32 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated April 17, 2015, 7:32 a.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. 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 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
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 April 17, 2015, 8:33 a.m.) Review request for KDE Base Apps, Martin Gräßlin, John Tapsell, and Thomas Pfeiffer. Changes --- detect KWin and act accordingly 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 (updated) - CMakeLists.txt 66899e577a03786d894423a8f1ce5b3aeed6de8a processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c 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 new submenu https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/eeaecc88-20bc-46d6-9c65-50ba4a7c182a__submenu.png new message box https://git.reviewboard.kde.org/media/uploaded/files/2015/04/10/5565e806-bf16-4ef2-89f2-ac122bf3421c__messagebox.png Thanks, Gregor Mi
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
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/#review77589 --- processui/keyboardshortcututil.cpp https://git.reviewboard.kde.org/r/122249/#comment53290 requires https://git.reviewboard.kde.org/r/122981/ to be submitted - Gregor Mi 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
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 March 13, 2015, 7:27 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- Simplify the KeyboardShortcutUtil::findGlobalShortcutByComponentAndActionName method by using KGlobalAccel API. Unfortunately, at least on my machine, this simplified code disables the shortcut. Pressing the displayed shortcut has no effect until Plasma session relogin. 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) - tests/keyboardshortcututiltest.cpp PRE-CREATION tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 processui/keyboardshortcututil.cpp PRE-CREATION processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 tests/keyboardshortcututiltest.h PRE-CREATION processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c processui/keyboardshortcututil.h PRE-CREATION processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 CMakeLists.txt 5352e70f6f37daae76c1b3e2c1c9f149d235e3cd 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated March 13, 2015, 7:29 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Repository: libksysguard Description (updated) --- 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 - tests/keyboardshortcututiltest.cpp PRE-CREATION tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 processui/keyboardshortcututil.cpp PRE-CREATION processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 tests/keyboardshortcututiltest.h PRE-CREATION processui/ksysguardprocesslist.cpp 450ca600b8aed7ca611ec638610b6c524c96080c processui/keyboardshortcututil.h PRE-CREATION processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 CMakeLists.txt 5352e70f6f37daae76c1b3e2c1c9f149d235e3cd 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 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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
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 March 13, 2015, 10:08 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- Fix the global shortcut gets unset issue by using a new method: KGlobalAccel::loadShortcutFromGlobalSettings 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 (updated) - 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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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
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
*/
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.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 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.
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
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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.
Issue 1.: *not* necessary, see
kde/workspace/plasma-workspace/kglobalaccel/component.h, line 98
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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.
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
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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.
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.
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.m.)
Review request for KDE Base Apps, Martin Gräßlin
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
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);
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)
*/
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.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 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.
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?
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/#review76861
---
On Feb. 27, 2015, 1:18 a.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122249/
---
(Updated Feb. 27, 2015, 1:18 a.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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Feb. 27, 2015, 1:18 a.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- Fix message box. What about having this message box as intermediate solution until the xkill generalization is done? 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 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. 23, 2015, 9:15 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- Replace invoking the kill method by just showing a message box that shows the global shortcut and a message what to expect. 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 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/#review76499 --- After update to latest master I got a crash (in master) of ksysguard when I click the End Process... button. The confirmation dialog is about to appear but then seg fault. export KDE_DEBUG=1 139]gregor@catgroove[KF5 set]:~/dev/kf5/src ksysguard ksysguard(3891)/kf5.kiconthemes KIconLoaderPrivate::initIconThemes: Theme tree: (Breeze) ksysguard(3891)/default KLocalizedStringPrivate::toString: Trying to convert empty KLocalizedString to QString. // click the End Process... buton ksysguard(3891)/kf5.kservice.sycoca KSycocaPrivate::openDatabase: Trying to open ksycoca from /home/gregor/.cache5/ksycoca5 ksysguard(3891)/default KNotificationManager::notify: Calling notify on Sound Segmentation fault I also wonder why I don't get a stack trace on console even though I build in debug configuration. Any hint? - Gregor Mi 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
--- 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
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 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
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 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: 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. 15, 2015, 4:35 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description (updated) --- 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 Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote:
processcore/processes_linux_p.cpp, line 171
https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171
Don't you change behavior here?
Before, we just wrote into the varialbes.
Now, we use the setters, which also sets 'changes |= Process::Gids;'
Is that maybe an issue? I myself don't know the code well enough to see
this here.
Gregor Mi wrote:
True. Thanks for noticing. The changes will be read in
ProcessModelPrivate::processChanged when a change is signaled by
KSysGuard::Process::processChanged. Then GUI model updates are triggered. So
probably the worst that can happen is one additional GUI update when
processChanged is emitted.
Gregor Mi wrote:
I did some analysis. There is no visible change in behaviour ksysguard.
ProcessesLocal::Private::readProcStatus where the change in discussion
was made (setter method with change detection instead of direct assignment).
is called by ProcessesLocal::updateProcessInfo in the same file:
```
bool ProcessesLocal::updateProcessInfo( long pid, Process *process)
{
bool success = true;
QString dir = /proc/ + QString::number(pid) + '/';
if(!d-readProcStat(dir, process)) success = false;
if(!d-readProcStatus(dir, process)) success = false;
...
```
Then there is
```
bool Processes::updateProcess( Process *ps, long ppid)
...
...
bool success = updateProcessInfo(ps);
emit processChanged(ps, false); ---
return success;
}
```
```
bool Processes::updateOrAddProcess( long pid)
...
...
Process *ps = d-mProcesses.value(pid);
if(!ps)
return addProcess(pid, ppid);
else
return updateProcess(ps, ppid); -
}
```
which is called e.g. every second.
The emitted processChanged signal is connected here:
```
connect( mProcesses, SIGNAL(processChanged(KSysGuard::Process*,bool)),
this, SLOT(processChanged(KSysGuard::Process*,bool)));
```
void ProcessModelPrivate::processChanged(KSysGuard::Process *process,
bool onlyTotalCpu):
Each change on a field that was recored in
ProcessesLocal::Private::readProcStatus will emit dataChanged signal, e.g.:
```
if(process-changes() KSysGuard::Process::Uids) {
totalUpdated++;
QModelIndex index = q-createIndex(row,
ProcessModel::HeadingUser, process);
emit q-dataChanged(index, index);
}
```
The dataChanged signal is from QAbstractItemModel. So as far as I can see
the worst thing could happen that the GUI is updated at more places than
needed.
When I start ksysguard the processChanged method is called about 300
times (1 time for each process in the list) in the update interval (every
second).
I tried to update everything at once and circument the dataChanged of
individual items with
```
QModelIndex index1 = q-createIndex(row, 0, process);
QModelIndex index2 = q-createIndex(row, q-columnCount() - 1,
process);
emit q-dataChanged(index1, index2);
return;
```
which had not visible effect.
So what now? Leave the setters and have maybe a performance penalty?
Remove the setters again and use references getters? Change the Process API
otherwise?
any new input?
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---
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
Re: Another proposal for modernization of our infrastructure
On 01/31/2015 08:57 PM, Alexander Neundorf wrote: hmm, looking back at our switch to git, I don't consider our standards for documentation of the developer workflow as very high unfortunately. :-/ Considering I wrote the majority of https://community.kde.org/Sysadmin/GitKdeOrgManual I guess I'll have to take that to heart, then ... For me as newcomer the mentioned wiki page and https://techbase.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages were valuable resources to get familiar with the KDE dev workflow. So it cannot be that bad. ;-)
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/processes_linux_p.cpp, line 159 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line159 In theorey, if we wanted to avoid the local varialbes here, we could add reference-getters, e.g.: qlonglong process-ruid(); These reference getters could be used as parameters to store the data directly in the desired varialbles, which would equal the current behavior. Not sure it's worth it, would be cool to have input from true KSysGuard developers here. Gregor Mi wrote: In the current RR state there are some reference-getters left because it made the porting easier. Should they be converted to non-reference getters, too? I drop the issue. The potential removal of remaining reference-getters can be done in another commit. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? 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 Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote:
processcore/processes_linux_p.cpp, line 171
https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171
Don't you change behavior here?
Before, we just wrote into the varialbes.
Now, we use the setters, which also sets 'changes |= Process::Gids;'
Is that maybe an issue? I myself don't know the code well enough to see
this here.
Gregor Mi wrote:
True. Thanks for noticing. The changes will be read in
ProcessModelPrivate::processChanged when a change is signaled by
KSysGuard::Process::processChanged. Then GUI model updates are triggered. So
probably the worst that can happen is one additional GUI update when
processChanged is emitted.
I did some analysis. There is no visible change in behaviour ksysguard.
ProcessesLocal::Private::readProcStatus where the change in discussion was made
(setter method with change detection instead of direct assignment).
is called by ProcessesLocal::updateProcessInfo in the same file:
```
bool ProcessesLocal::updateProcessInfo( long pid, Process *process)
{
bool success = true;
QString dir = /proc/ + QString::number(pid) + '/';
if(!d-readProcStat(dir, process)) success = false;
if(!d-readProcStatus(dir, process)) success = false;
...
```
Then there is
```
bool Processes::updateProcess( Process *ps, long ppid)
...
...
bool success = updateProcessInfo(ps);
emit processChanged(ps, false); ---
return success;
}
```
```
bool Processes::updateOrAddProcess( long pid)
...
...
Process *ps = d-mProcesses.value(pid);
if(!ps)
return addProcess(pid, ppid);
else
return updateProcess(ps, ppid); -
}
```
which is called e.g. every second.
The emitted processChanged signal is connected here:
```
connect( mProcesses, SIGNAL(processChanged(KSysGuard::Process*,bool)), this,
SLOT(processChanged(KSysGuard::Process*,bool)));
```
void ProcessModelPrivate::processChanged(KSysGuard::Process *process, bool
onlyTotalCpu):
Each change on a field that was recored in
ProcessesLocal::Private::readProcStatus will emit dataChanged signal, e.g.:
```
if(process-changes() KSysGuard::Process::Uids) {
totalUpdated++;
QModelIndex index = q-createIndex(row, ProcessModel::HeadingUser,
process);
emit q-dataChanged(index, index);
}
```
The dataChanged signal is from QAbstractItemModel. So as far as I can see the
worst thing could happen that the GUI is updated at more places than needed.
When I start ksysguard the processChanged method is called about 300 times (1
time for each process in the list) in the update interval (every second).
I tried to update everything at once and circument the dataChanged of
individual items with
```
QModelIndex index1 = q-createIndex(row, 0, process);
QModelIndex index2 = q-createIndex(row, q-columnCount() - 1, process);
emit q-dataChanged(index1, index2);
return;
```
which had not visible effect.
So what now? Leave the setters and have maybe a performance penalty? Remove the
setters again and use references getters? Change the Process API otherwise?
- Gregor
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
---
On Jan. 25, 2015, 10:27 p.m., Gregor Mi wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/
---
(Updated Jan. 25, 2015, 10:27 p.m.)
Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John
Tapsell.
Repository: libksysguard
Description
---
In process.h there are several public fields. This RR introduces a d-ptr.
(In a separate commit I would add the .reviewboardrc file)
What is the current policy on using small C++ macros as done in this RR? Use
it (code is more compact and readable) or don't use it (better for debugging)?
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
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 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 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 Jan. 28, 2015, 8:35 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- add screenshot 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 4dc142e864d8353ceafc3a6735ffa81e48291420 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 (updated) 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)
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 the window via XKill
*/
void Client::killWindow()
{
qCDebug(KWIN_CORE) Client::killWindow(): caption();
killProcess(false
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. 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? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/#review74740 --- On Jan. 25, 2015, 6:21 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Jan. 25, 2015, 6:21 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 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION tests/keyboardshortcututiltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/process.h, line 40 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40 Is virtual needed here? Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used. On Jan. 21, 2015, 10:10 a.m., Dominik Haumann wrote: processcore/processes_linux_p.cpp, line 171 https://git.reviewboard.kde.org/r/121831/diff/3/?file=343197#file343197line171 Don't you change behavior here? Before, we just wrote into the varialbes. Now, we use the setters, which also sets 'changes |= Process::Gids;' Is that maybe an issue? I myself don't know the code well enough to see this here. True. Thanks for noticing. The changes will be read in ProcessModelPrivate::processChanged when a change is signaled by KSysGuard::Process::processChanged. Then GUI model updates are triggered. So probably the worst that can happen is one additional GUI update when processChanged is emitted. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 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 Jan. 25, 2015, 5:28 p.m., Alex Richardson wrote: processui/scripting.h, line 74 https://git.reviewboard.kde.org/r/121831/diff/5/?file=344700#file344700line74 Isn't just changing the PROPERTY macro enough? Or is it used in some other file? Gregor Mi wrote: I did the refactoring in several steps so both methods were needed on the way. Now the macro can be renamed back as you noted. fixed now - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74713 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:20 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- Process: codestyle: write getters and setters in pairs to better keep track Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:27 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- more consistent order of methods Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 10:08 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- - process.h: consistent method naming, move getters before setters - scripting: remove/rename macro Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:46 a.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- Process: d-ptr for 32 more fields rebase on latest master Repository: libksysguard Description (updated) --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
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 Jan. 25, 2015, 6:21 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- Do not show the drop down menu if KWin dbus interface is not available 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 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION tests/keyboardshortcututiltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- Thanks, Gregor Mi
Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
On Jan. 25, 2015, 5:28 p.m., Alex Richardson wrote: processui/scripting.h, line 74 https://git.reviewboard.kde.org/r/121831/diff/5/?file=344700#file344700line74 Isn't just changing the PROPERTY macro enough? Or is it used in some other file? I did the refactoring in several steps so both methods were needed on the way. Now the macro can be renamed back as you noted. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74713 --- On Jan. 25, 2015, 12:01 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 25, 2015, 12:01 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 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 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/#review74716 --- What about einar77 gregormi: org.kde.kwin will likely go away in the future einar77 gregormi: see https://git.reviewboard.kde.org/r/122215/ ? - Gregor Mi On Jan. 25, 2015, 6:21 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122249/ --- (Updated Jan. 25, 2015, 6:21 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 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION tests/keyboardshortcututiltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- Thanks, Gregor Mi
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 Jan. 25, 2015, 5:47 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- call killWindow via dbus 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) - tests/keyboardshortcututiltest.cpp PRE-CREATION processui/CMakeLists.txt 7f87b85e0201e63d69070a71203bbb34851a79c6 processui/ProcessWidgetUI.ui e50f55cf1813b00d49b1716023df487ffbd536e3 processui/keyboardshortcututil.h PRE-CREATION processui/keyboardshortcututil.cpp PRE-CREATION processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- Thanks, Gregor Mi
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 Jan. 25, 2015, 5:33 p.m.) Review request for KDE Base Apps, Martin Gräßlin and John Tapsell. Changes --- @Martin: is this the correct way of dealing with the KWin/killWindow thing? 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 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION tests/keyboardshortcututiltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- Thanks, Gregor Mi
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/ --- Review request for KDE Base Apps 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 4dc142e864d8353ceafc3a6735ffa81e48291420 tests/CMakeLists.txt 967b03fae1e460bfb22e1a07ef05cf7b49412546 tests/keyboardshortcututiltest.h PRE-CREATION tests/keyboardshortcututiltest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122249/diff/ Testing --- 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/ --- (Updated Jan. 25, 2015, 12:01 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- Process: d-ptr for 6 more fields, add namespace to cpp file Process: d-ptr for the last two fields Ready to commit. Repository: libksysguard Description --- In process.h there are several public fields. This RR introduces a d-ptr. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - .reviewboardrc PRE-CREATION processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing (updated) --- 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 Jan. 19, 2015, 6:24 p.m., Aleix Pol Gonzalez wrote: processcore/process.h, line 103 https://git.reviewboard.kde.org/r/121831/diff/2/?file=342813#file342813line103 Maybe you want to prefix the attribute variables with m_ then? Thanks for the hint. I will move this member and the ones below it also to the ProcessPrivate class. There the m_ can (should?) be omitted, can't it? - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74353 --- On Jan. 19, 2015, 6:17 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 19, 2015, 6:17 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/ --- (Updated Jan. 19, 2015, 6:17 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/ --- (Updated Jan. 19, 2015, 9:33 p.m.) Review request for KDE Base Apps and John Tapsell. Changes --- Process: d-ptr for the next 11 fields (more to come) Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31 processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/ --- (Updated Jan. 19, 2015, 9:37 p.m.) Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell. Changes --- readd lost people Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31 processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/ --- (Updated Jan. 18, 2015, 11:03 p.m.) Review request for KDE Base Apps, Eike Hein and John Tapsell. Changes --- Updated to use d-ptr as suggested. I added you to this RR, Eike. Maybe you can take a look if the way I started to use the d-ptr is the correct one. Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs (updated) - .reviewboardrc PRE-CREATION CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/#review74096 --- processcore/process.h https://git.reviewboard.kde.org/r/121831/#comment51460 Requires SOVERSION bump. And: wouldn't it be better to move the now private variables behind a d_ptr? - Gregor Mi On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 12, 2015, 2:30 p.m.) Review request for KDE Base Apps and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 .reviewboardrc PRE-CREATION processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. 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/#review74100 --- see also last comments in https://git.reviewboard.kde.org/r/122072/ about redesigning it with d_ptr to make process.h future-proof - Gregor Mi On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/ --- (Updated Jan. 12, 2015, 2:30 p.m.) Review request for KDE Base Apps and John Tapsell. Repository: libksysguard Description --- In process.h there are several public fields. I propose to encapsulate the private fields with getter methods. I implemented it exemplary for the fields 'login', 'uid' and 'euid'. (In a separate commit I would add the .reviewboardrc file) What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)? Diffs - processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 .reviewboardrc PRE-CREATION processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 Diff: https://git.reviewboard.kde.org/r/121831/diff/ Testing --- Compiles and runs. Data is still shown. No error. Thanks, Gregor Mi
Re: Review Request 121717: libksysguard/processtable: Add new column Relative Start Time
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/ --- (Updated Jan. 13, 2015, 9:08 p.m.) Status -- This change has been marked as submitted. Review request for KDE Base Apps and John Tapsell. Repository: libksysguard Description --- This will add a new column Relative Start Time which shows how much time has elapsed since the process was started. Some details: - add new heading with default location between Shared Memory and Command and not visible by default - define What's this - define Tooltip - define sorting - add class TimeUtil with methods: - systemUptimeSeconds - systemUptimeAbsolute - secondsToHumanElapsedString (for this one a unit test was added, see chronotest.cpp) This code reformatting goes in separate commits: - ProcessModel.cpp: reformat code: consistent number of linebreaks between method definitions (1 blank line) - ProcessModel.h: reformat code: split long enum line into separte lines for better diffing Side note on sorting: I was wondering if the sorting of the PID column is exactly the same as with the new Relative Start Time column. When testing on my computer it was. But according to this post one cannot generally assume that sorting by PID will reflect the relative start order of the processes: http://stackoverflow.com/questions/822797/about-the-pid-of-the-process Diffs - processui/ProcessModel.h a338536023f9d003a44bcb8420b9288f8673ea92 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/ksysguardprocesslist.cpp 894e9a4d42112e01e742f1b0a2bcd6be7a844258 processui/timeutil.h PRE-CREATION tests/CMakeLists.txt 0fb3ab620564abf09f82d1609fc464d5597b2bd3 tests/chronotest.h PRE-CREATION tests/chronotest.cpp PRE-CREATION processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a Diff: https://git.reviewboard.kde.org/r/121717/diff/ Testing --- Run ksysguard, show new column, sort in both directions. Minor issue: as the seconds pass the values in the new column will not be updated automatically unless there is some user interaction (like mouse hovering/moving or sorting). New unit test passes. Thanks, Gregor Mi
Re: Review Request 121717: libksysguard/processtable: Add new column Relative Start Time
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/ --- (Updated Jan. 12, 2015, 2:07 p.m.) Review request for KDE Base Apps and John Tapsell. Changes --- fix issues and unit test Repository: libksysguard Description --- This will add a new column Relative Start Time which shows how much time has elapsed since the process was started. Some details: - add new heading with default location between Shared Memory and Command and not visible by default - define What's this - define Tooltip - define sorting - add class TimeUtil with methods: - systemUptimeSeconds - systemUptimeAbsolute - secondsToHumanElapsedString (for this one a unit test was added, see chronotest.cpp) This code reformatting goes in separate commits: - ProcessModel.cpp: reformat code: consistent number of linebreaks between method definitions (1 blank line) - ProcessModel.h: reformat code: split long enum line into separte lines for better diffing Side note on sorting: I was wondering if the sorting of the PID column is exactly the same as with the new Relative Start Time column. When testing on my computer it was. But according to this post one cannot generally assume that sorting by PID will reflect the relative start order of the processes: http://stackoverflow.com/questions/822797/about-the-pid-of-the-process Diffs (updated) - processui/ProcessModel.h a338536023f9d003a44bcb8420b9288f8673ea92 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/ksysguardprocesslist.cpp 894e9a4d42112e01e742f1b0a2bcd6be7a844258 processui/timeutil.h PRE-CREATION tests/CMakeLists.txt 0fb3ab620564abf09f82d1609fc464d5597b2bd3 tests/chronotest.h PRE-CREATION tests/chronotest.cpp PRE-CREATION processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a Diff: https://git.reviewboard.kde.org/r/121717/diff/ Testing --- Run ksysguard, show new column, sort in both directions. Minor issue: as the seconds pass the values in the new column will not be updated automatically unless there is some user interaction (like mouse hovering/moving or sorting). New unit test passes. Thanks, Gregor Mi
Re: Review Request 121717: libksysguard/processtable: Add new column Relative Start Time
On Jan. 10, 2015, 10:18 p.m., Albert Astals Cid wrote: processcore/process.cpp, line 203 https://git.reviewboard.kde.org/r/121717/diff/2/?file=337994#file337994line203 initialize to 0 in the constructor? Yes, I forgot that, thanks. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/#review73714 --- On Jan. 12, 2015, 2:07 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/ --- (Updated Jan. 12, 2015, 2:07 p.m.) Review request for KDE Base Apps and John Tapsell. Repository: libksysguard Description --- This will add a new column Relative Start Time which shows how much time has elapsed since the process was started. Some details: - add new heading with default location between Shared Memory and Command and not visible by default - define What's this - define Tooltip - define sorting - add class TimeUtil with methods: - systemUptimeSeconds - systemUptimeAbsolute - secondsToHumanElapsedString (for this one a unit test was added, see chronotest.cpp) This code reformatting goes in separate commits: - ProcessModel.cpp: reformat code: consistent number of linebreaks between method definitions (1 blank line) - ProcessModel.h: reformat code: split long enum line into separte lines for better diffing Side note on sorting: I was wondering if the sorting of the PID column is exactly the same as with the new Relative Start Time column. When testing on my computer it was. But according to this post one cannot generally assume that sorting by PID will reflect the relative start order of the processes: http://stackoverflow.com/questions/822797/about-the-pid-of-the-process Diffs - processui/ProcessModel.h a338536023f9d003a44bcb8420b9288f8673ea92 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/ksysguardprocesslist.cpp 894e9a4d42112e01e742f1b0a2bcd6be7a844258 processui/timeutil.h PRE-CREATION tests/CMakeLists.txt 0fb3ab620564abf09f82d1609fc464d5597b2bd3 tests/chronotest.h PRE-CREATION tests/chronotest.cpp PRE-CREATION processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a Diff: https://git.reviewboard.kde.org/r/121717/diff/ Testing --- Run ksysguard, show new column, sort in both directions. Minor issue: as the seconds pass the values in the new column will not be updated automatically unless there is some user interaction (like mouse hovering/moving or sorting). New unit test passes. Thanks, Gregor Mi
Re: Review Request 121717: libksysguard/processtable: Add new column Relative Start Time
On Jan. 10, 2015, 4:29 p.m., Dominik Haumann wrote: processui/ProcessModel.cpp, line 1655 https://git.reviewboard.kde.org/r/121717/diff/2/?file=337997#file337997line1655 What do you mean by the comment 'first iteration'? I removed it because it was not needed - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/#review73675 --- On Jan. 12, 2015, 2:07 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/ --- (Updated Jan. 12, 2015, 2:07 p.m.) Review request for KDE Base Apps and John Tapsell. Repository: libksysguard Description --- This will add a new column Relative Start Time which shows how much time has elapsed since the process was started. Some details: - add new heading with default location between Shared Memory and Command and not visible by default - define What's this - define Tooltip - define sorting - add class TimeUtil with methods: - systemUptimeSeconds - systemUptimeAbsolute - secondsToHumanElapsedString (for this one a unit test was added, see chronotest.cpp) This code reformatting goes in separate commits: - ProcessModel.cpp: reformat code: consistent number of linebreaks between method definitions (1 blank line) - ProcessModel.h: reformat code: split long enum line into separte lines for better diffing Side note on sorting: I was wondering if the sorting of the PID column is exactly the same as with the new Relative Start Time column. When testing on my computer it was. But according to this post one cannot generally assume that sorting by PID will reflect the relative start order of the processes: http://stackoverflow.com/questions/822797/about-the-pid-of-the-process Diffs - processui/ProcessModel.h a338536023f9d003a44bcb8420b9288f8673ea92 processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 processui/ksysguardprocesslist.cpp 894e9a4d42112e01e742f1b0a2bcd6be7a844258 processui/timeutil.h PRE-CREATION tests/CMakeLists.txt 0fb3ab620564abf09f82d1609fc464d5597b2bd3 tests/chronotest.h PRE-CREATION tests/chronotest.cpp PRE-CREATION processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a Diff: https://git.reviewboard.kde.org/r/121717/diff/ Testing --- Run ksysguard, show new column, sort in both directions. Minor issue: as the seconds pass the values in the new column will not be updated automatically unless there is some user interaction (like mouse hovering/moving or sorting). New unit test passes. Thanks, Gregor Mi
