[Kde-pim] Review Request: Fix Message Group Aggregation by Date

[John Layt]

-----------------------------------------------------------
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 at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/