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

Fri Feb 8 11:03:53 GMT 2013

> 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.
>
> 
> Daniel Kreuter wrote:
>     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.
> 
> Frank Reininghaus wrote:
>     I wasn't referring to "month + year" being appended to the group name. What I find strange is that there are two different "Last week" groups separated by the groups "Thursday", "Wednesday", and "Tuesday". It's like there are two different "last" weeks, with a few days in between which belong to the "current" week. This looks like a broken mess to me, but considering that nobody else seems to consider this a bug, maybe I'm wrong after all ;-)
> 
> Daniel Kreuter wrote:
>     I've changed the logic now. When the month aren't the same I print Last Month, Two Month Ago and so on as long as the year stays the same. Everything older will have the title Older. Maybe we can shorten that to lets say 4 month or so.
>     
>     But this doesn't solve the first day of week problem. Maybe we should rename Last Week to One Week ago? Then it doesn't matter which day was the first day of the week.
>
> 
> Frank Reininghaus wrote:
>     > I've changed the logic now. When the month aren't the same I print Last Month, Two Month Ago and so on as long as the year stays the same.
>     
>     I'm not sure if that is a good idea. Imagine what you would see on March 1 - everything you did in February would just be listed as 'Last Month'. Right now, it's grouped into Yesterday etc., and I think that this is a good thing.
>     
>     Concerning 'Last week'/'One week ago': maybe you're right, 'One week ago' might be slightly clearer.
> 
> Daniel Kreuter wrote:
>     Good point. I will change that so far that we say after 4-5 weeks, One Month Ago, making it depending on the daysDistance.
>     The exact algorithm will not be that easy but I think I will find a solution for that too. 
>     
>     >Concerning 'Last week'/'One week ago': maybe you're right, 'One week ago' might be slightly clearer.
>     It isn't only clearer, it also doesn't tell you something dependend on the current weekday or the firstDayOfWeek setting you may have, because you may think ok the file is older than one week.

I think the easiest solution for all problems might be, as I alrady said, to replace

switch (currentWeek - modifiedWeek)

by

switch (daysDistance / 7),

optionally replace 'Last Week' by 'One week ago', and leave everything else unchanged. Alternatively, if there is really a big demand for having things grouped by calendar weeks, which I doubt, then one should drop the "daysDistance <= 7 * x" conditions for the 'last month' grouping and also do it using calendar weeks (with your firstWeekDay fix applied) there, to make it consistent.

I don't see why anything more complicated would be needed.

- Frank

-----------------------------------------------------------
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/20130208/c360285d/attachment.htm>