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

[Thomas McGuire]

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

Ship it!


John, many thanks for fixing this, very much appreciated.
Admittedly I didn't review the logic parts, I trust you that you got those right. There is one issue of a bit of code duplication, fix that before committing if you want.
So please go ahead and commit, so that the week 53 bug is fixed :)

The usage of time_t is mostly for historical reasons, the code dealing with message dates is from 1996 or so.
I'd rather not change that for KMail 1, but if anyone feels like it, the KMail 2 port which will soon arrive in trunk is free for all.
The new mail parsing library, KMime, uses KDateTime, but that is currently converted to time_t before passing the date to the message list (again, historical reasons only).

> I've kept the other splits as current, but there does seem to be a little inconsistency

Oh, indeed. Maybe stuff for another patch.

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

I think it can go into RC2, this will probably not break stuff.

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

You're doing great :) The coding style is again for historic reasons, and we can't change it now. The coding style for if statements here is from Szymon :)


trunk/KDE/kdepim/messagelist/core/model.cpp
<http://reviewboard.kde.org/r/2521/#comment2865>

    Little code duplication here, same code is above.


- Thomas


On 2010-01-07 12:02:06, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2521/
> -----------------------------------------------------------
> 
> (Updated 2010-01-07 12:02:06)
> 
> 
> Review request for KDE PIM.
> 
> 
> 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
> -----
> 
>   trunk/KDE/kdepim/messagelist/core/model.cpp 1070882 
> 
> 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/