> On Jan. 3, 2015, 12:35 p.m., Dominik Haumann wrote: > > The feature itself is nice, but in its current implementation, the > > translators won't be able to properly translate the strings into other > > languages, see detailed comments. > > > > Besides that, would be nice to get another review.
Thanks a lot for the review. All issues are fixed now. > On Jan. 3, 2015, 12:35 p.m., Dominik Haumann wrote: > > processui/ProcessModel.cpp, line 1656 > > <https://git.reviewboard.kde.org/r/121717/diff/1/?file=336313#file336313line1656> > > > > I guess returning the startTime is fine, no need for extra conversions, > > as long as the order is ok. The PlainValueRole is not for sorting. It is used in the overloaded QAbstractItemModel::mimeData method. According to this code comment ```case PlainValueRole: //Used to return a plain value. For copying to a clipboard etc``` it is used for clipboard. Although I could not find out how to copy a row to clipboard. So, I only changed the comment. - Gregor ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121717/#review73001 ----------------------------------------------------------- On Jan. 3, 2015, 8:02 p.m., Gregor Mi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121717/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2015, 8:02 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 > - secondsToHumanString1 (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 > ----- > > processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 > 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.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 > processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a > processui/ProcessModel.h a338536023f9d003a44bcb8420b9288f8673ea92 > processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 > > 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 > >
