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

[Daniel Kreuter]

> On Feb. 5, 2013, 9:44 a.m., Frank Reininghaus wrote:
> > Thanks for the new patch, looks better!
> > 
> > I just tried to test it, created a set of files with different modification times and first wanted to check what it looks like without patch:
> > 
> > http://ompldr.org/vaGN3ag/testi.png
> > 
> > So it seems that the 'first week day' issue is not the only thing that needs fixing in that function ;-) The problem is:
> > 
> > 1. If a file was modified in the current month, it is only shown in the 'current week' (i.e., 'Today', 'Yesterday', or 'Wednesday', etc.) if it is from the same calendar week. If that is not the case, the 'Last Week'/'Two weeks ago' stuff is determined based on the calendar week distance.
> > 
> > 2. If a file was modified last month, it is shown in the 'current' week if it is less than 8 days old. Similarly, it is shown in 'Last Week' if it is less than 15 days old, etc.
> > 
> > This is why we get such a broken grouping. I'm thinking that the easiest way to fix this and to resolve the 'first week day' issue is to throw away the concept of 'calendar weeks' in that function and always group based on the age in days (like it's currently done for files from last month).
> > 
> > This would mean that we replace
> > 
> > switch (currentWeek - modifiedWeek) {
> >     // ...
> > }
> > 
> > by
> > 
> > switch (daysDistance / 7) {
> >     // ...
> > }
> > 
> > and remove all code related to currentWeek/modifiedWeek.
> > 
> > OTOH, this would change the behaviour. However, considering that there is to my knowledge no bug report about this issue that I found the first time I tried grouping by date, I wonder how many people would notice... What do you think?
> 
> Daniel Kreuter wrote:
>     i found that behavior strange too when I first realized it. But when you have a look at the code you see that this is not a bug but a feature.
> 
> Frank Reininghaus wrote:
>     Just to be sure: do you think that the 'feature' is
>     
>     a) the thing that you see in my screenshot (in which case I strongly disagree), or
>     b) the 'group by calendar week' thing that is provided for the current month?
>     
>     I think that we really should make the grouping consistent, no matter if the modification time is this month or last month. So the options are either to always use the 'days distance' (which is currently done for last month) or to always use the 'calendar week' system.
>     
>     I don't have any strong opinions about that (except for the fact that the 'days distance' is much easier to implement), but I think that we should make it consistent now that the function is being looked into. Fixing the 'first week day' issue in one part and leaving the rest in the inconsistent state will only make it harder to fix it properly later on.
>

I meant the thing in the screenshot, because that's the behavior if you are between two month.
Then he looks at the daysDistance and prints that (what you proposed for the whole method), but appends the month + year to the group name.

But ok if you think we should only consider days distance at all, that would be ok for me, and it would be much easier to implement the first day of week logic. 


- Daniel


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


On Feb. 4, 2013, 7:40 p.m., Daniel Kreuter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108667/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2013, 7:40 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/20130206/caccbbde/attachment.htm>