Re: Sysadmin report on the modernization of our infrastructure
On January 21, 2015 05:12:07 PM Ben Cooksley wrote: Hi all, As promised in the earlier thread, i'd like to present the sysadmin report on the state of the infrastructure surrounding our code. It contains a detailed summary of what is broken with our existing systems, why change is necessary and an evaluation of the options we considered. We have also made a proposal based on our evaluations and the wishlist of functionality drawn up the community. Thanks for putting this report together! I think the outline of the current issues and the listed requirements are very useful, and as outlined Phabricator does seem like a good contender. I do have one important question regarding its review system, how does it handle a series of commits? For more complicated changes, there may be several commits to get from point A to B that I'd like to get reviewed. ReviewBoard doesn't currently handles this, and instead squashes them all into one patch (see for instance: https://git.reviewboard.kde.org/r/117010/ , which was originally made up of 5 different commits). One of the plus points of Gerrit for me was that it would have shown each of these commits separately (though having each become a change isn't ideal to me). How would Phabricator handle a set of commits like this? -- Matthew signature.asc Description: This is a digitally signed message part.
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
Review Request 122252: KRecursiveFilterProxyModel: fix emitting superfluous dataChanged signals
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122252/ --- Review request for kdelibs and Christian Mollekopf. Repository: kdelibs Description --- by using an internal cache of the filtering state. The tricky part is that filterAcceptsRow() must not use the cache (too bad, it would be faster), because of the setFilterFixedString() feature which can change the filtering status of indexes without KRFPM even being informed (QSFPM has no virtual method, no event...) So it only ever writes to the cache, but when dataChanged() or row insertion/removal comes in, we can look into the cache to find the old state and compare. Diffs - kdeui/itemviews/krecursivefilterproxymodel.h c16b62186fb9203ff297bd9fd28d9c13a1c8bdc4 kdeui/itemviews/krecursivefilterproxymodel.cpp efa286ad87ded962b20c8a581b659d1b154ebf3a kdeui/tests/krecursivefilterproxymodeltest.cpp 3bcb72980730cb22f887ae8fa5fbd91b5609aeb6 Diff: https://git.reviewboard.kde.org/r/122252/diff/ Testing --- Unit tests. Using kmail (and especially testing the substring filtering in the move dialog, which an earlier version of this patch broke). Looking at the number of calls to Akonadi::FavoriteCollectionsModel::Private::reload() for a single dataChanged() signal from the ETM, which went from 10 to 4 (still too many, but the remaining problem is elsewhere). Thanks, David Faure
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: Sysadmin report on the modernization of our infrastructure
On Sun, Jan 25, 2015 at 12:23 PM, Matthew Dawson matt...@mjdsystems.ca wrote: On January 21, 2015 05:12:07 PM Ben Cooksley wrote: Hi all, As promised in the earlier thread, i'd like to present the sysadmin report on the state of the infrastructure surrounding our code. It contains a detailed summary of what is broken with our existing systems, why change is necessary and an evaluation of the options we considered. We have also made a proposal based on our evaluations and the wishlist of functionality drawn up the community. Thanks for putting this report together! I think the outline of the current issues and the listed requirements are very useful, and as outlined Phabricator does seem like a good contender. Not a problem. I do have one important question regarding its review system, how does it handle a series of commits? For more complicated changes, there may be several commits to get from point A to B that I'd like to get reviewed. ReviewBoard doesn't currently handles this, and instead squashes them all into one patch (see for instance: https://git.reviewboard.kde.org/r/117010/ , which was originally made up of 5 different commits). One of the plus points of Gerrit for me was that it would have shown each of these commits separately (though having each become a change isn't ideal to me). How would Phabricator handle a set of commits like this? The actual diff itself is shown as a single merged entity ala Reviewboard. However the chain of commits used to generate the change (assuming you use Arcanist to submit it) is shown in the user interface, complete with SHA's and commit messages. -- Matthew Cheers, Ben
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
