Review Request 108667: Let dolphin look at the firstDayOfWeek as set up in the system's locale settings.

[Frank Reininghaus]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108667/#review26587
-----------------------------------------------------------


Thanks for the new patch! Looks indeed better now. I've added some comments below, mostly coding-style related.

Have you already tested the code for different 'modified week days'? It looks like it might go wrong if the weekStartDay is Monday and a file has been modified, e.g., on a Monday 10 days ago. But I might be wrong, I haven't actually tested it yet.


dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20189>

    I think the comment might be slightly misleading. I think that the '53' is actually a trick to make the calculation work if 'currentDate' is at the beginning of January, but 'modifiedDate' is at the end of December.



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20190>

    Coding style: put the space behind the '*'.



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20191>

    const int



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20192>

    The comment does not apply anymore for the current version of the patch if I'm not mistaken.



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20193>

    Coding style: space after 'if'.



dolphin/src/kitemviews/kfileitemmodel.cpp
<http://git.reviewboard.kde.org/r/108667/#comment20194>

    Indentation is 4 spaces. Moreover, in most Dolphin code the '++' is put in front of the variable. I'd prefer to also do that here to keep it consistent.


- Frank Reininghaus


On Feb. 1, 2013, 5:03 p.m., Daniel Kreuter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108667/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2013, 5:03 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> When sorting files by date, dolphin doesn't note the user's system settings. There it's possible to define another first day of week instead of the default (f.e. Sunday instead of Monday).
> This patch fixes this issue.
> 
> 
> This addresses bug 181337.
>     http://bugs.kde.org/show_bug.cgi?id=181337
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 1062aa5 
> 
> Diff: http://git.reviewboard.kde.org/r/108667/diff/
> 
> 
> Testing
> -------
> 
> Performed the steps described in comment #3 of the bug report which I won't post here again.
> 
> 
> Thanks,
> 
> Daniel Kreuter
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130203/a561581c/attachment.htm>