Review Request 121717: libksysguard/processtable: Add new column "Relative Start Time"

[Dominik Haumann]

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


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.


processui/ProcessModel.cpp
<https://git.reviewboard.kde.org/r/121717/#comment50788>

    Correct, i18n won't work this way.



processui/ProcessModel.cpp
<https://git.reviewboard.kde.org/r/121717/#comment50787>

    Missing i18n. Besides this, the translators need more control in order to properly do their job:



processui/ProcessModel.cpp
<https://git.reviewboard.kde.org/r/121717/#comment50789>

    Missing i18n:
    const auto tooltip = i18n("Clock ticks since system boot: %1<br/>Seconds since system boot: %2 (System boot time: %3)<br/>Absolute start time: %4<br/>Relative start time: %5",
        clockTicksSinceSystemBoot,
        secondsSinceSystemBoot,
        systemBootTime.toString(),
        absoluteStartTime.toString(),
        timeUtil.secondsToHumanString1(relativeStartTime));



processui/ProcessModel.cpp
<https://git.reviewboard.kde.org/r/121717/#comment50790>

    I guess returning the startTime is fine, no need for extra conversions, as long as the order is ok.



processui/timeutil.h
<https://git.reviewboard.kde.org/r/121717/#comment50791>

    Is this true for everything one develops: The current implementation is always one way to implement something ;)
    
    I suggest to drop the '1'. The unit test, and add an example of the resulting text for each if-block.



processui/timeutil.h
<https://git.reviewboard.kde.org/r/121717/#comment50792>

    Translation issue for several reasons:
    
    1. i18n is missing.
    
    2. You always have to give the translators full control over the string they have to translate.
    
    That imples dropping the _*Unit member variables, because it doesn't not work for translation to save partially translated strings.


- Dominik Haumann


On Dec. 29, 2014, 8:21 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121717/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2014, 8:21 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 
>   processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 
>   processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a 
>   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 
> 
> 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).
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150103/1eadd521/attachment.htm>