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

[Frank Reininghaus]

> On Feb. 1, 2013, 10:44 a.m., Frank Reininghaus wrote:
> > Sorry about the last comments, it seems that I must have hit some key which made the browser submit the form.
> > 
> > 
> > In order to keep the code more readable, I wonder if one could slightly modify your approach and adjust the currentWeek and modifiedWeek variables directly? One could just add an
> > 
> > if (weekStartDay != 1) {
> >     // Adjust week
> > }
> > 
> > after the determination of currentWeek and modifiedWeek, respectively. This would prevent the need for the odd condition in the switch () statement.
> > 
> > Moreover, I think that this would greatly benefit from a unit test (but we can work on that together when the patch is ready if you want).
> 
> Daniel Kreuter wrote:
>     Hi Frank
>     
>     Thanks for your review. I will adjust the currentWeek or modifiedWeek so we can remove the calculation from the switch.
>     
>     Moving it to kdelibs: Which API should handle this? Seems not so easy for me atm.
>     
>     Unit testing would be grat and if you can help me there would be nice, since I only know Junit (java)

Sounds good, thanks! Concerning the test, I think I would add a parameter 'currentDate' to dateRoleGroups() which defaults to the current date. In the unit test, this would be set to a fixed value (or several different values), we don't want to make the outcome of the test depend on the day when it is run ;-) I would then just feed a model with a bunch of files with made-up modification times, set the locale's weekStartDay setting to different values and check that the result of dateRoleGroups() is as expected.

> Moving it to kdelibs: Which API should handle this? Seems not so easy for me atm.

I'm not sure. I think that the date/time stuff is going through major changes on the way to Qt 5/KDE Frameworks. Some solution might already be available there, but even if that is the case, I think that it has to be done in Dolphin itself until we can depend on that.


- Frank


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


On Jan. 30, 2013, 6:06 p.m., Daniel Kreuter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108667/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2013, 6:06 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 ab39e55 
> 
> 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/20130201/b364c9fe/attachment.htm>