[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Fix Message Group Aggregation by Date
From:       "John Layt" <johnlayt () googlemail ! com>
Date:       2010-01-11 12:03:36
Message-ID: 20100111120336.16061.22368 () localhost
[Download RAW message or body]


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

(Updated 2010-01-11 12:03:36.405596)


Review request for KDE PIM.


Changes
-------

Remove code duplication.  The first half of case options GroupByDate and \
GroupByDateRange are the same, but can't be moved outside the case for efficiency \
reasons (could be repeated 1000's of times when not needed if not grouped by date).  \
Instead I've merged them into 1 case option, which removes the duplication but \
perhaps is a little less clear.  This also includes GroupByDate now using day names \
for the last 7 days, it was just easier.


Summary
-------

See bug report, message aggregation by date gets confused in January due to use of \
ISO week numbers.  Dates are also not consistently localised.

I've assumed we want the week split to start from KLocale::weekStartDay() and not \
KLocale::workingWeekStartDay() and not by ISO week number which would always be \
Monday.  

I've kept the other splits as current, but there does seem to be a little \
inconsistency, i.e. if GroupByDate as soon as the mail falls into the previous month \
or week it doesn't get considered for day name except for Yesterday which is always \
grouped, whereas in GroupByDateGroup if the mail falls in the last 7 days it gets a \
day name regardless of what week or month it belongs to.  It seems the wrong way \
around to me.

I'm also wondering about the widespread use of time_t from time.h, shouldn't we be \
using something from KDE or Qt instead, or was there some reason not to?

Now 4.4 has been branched, should I commit this for RC2, or wait for 4.4.1?

I've tried to stick with the code style, but it is a little quirky :-)


This addresses bug 179076.
    https://bugs.kde.org/show_bug.cgi?id=179076


Diffs (updated)
-----

  trunk/KDE/kdepim/messagelist/core/model.cpp 1072229 

Diff: http://reviewboard.kde.org/r/2521/diff


Testing
-------

Before and after test on my inbox, now sorts correctly.


Thanks,

John

_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/


[prev in list] [next in list] [prev in thread] [next in thread]