Review Request: Add new KDate class to simplify date localization

David Jarvie djarvie at kde.org
Tue Oct 26 14:35:17 BST 2010 

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


Looks a good piece of work. I have only had time so far to look at the header file.

The main issue is that it needs a class level comment to explain what the class is for. One extra item which the class comment could have would be an explanation of how QDate relates to a KDate using the Gregorian calendar and western locales.

I'm also concerned about the handling of weeks - is the ISO week the only week which exists, or do some calendar systems have alternatives? If so, some revamping might be needed.


/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8657>

    Fix punctuation:
    "and Locale. Use setCalendar()..."



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8658>

    Fix punctuation:
    "and Locale. Use setCalendar()..."
    
    Can setCalendar() actually set a locale? It looks as if it can only set a calendar system.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8656>

    Copy the comment paragraph from setDate(int,int,int) to clarify the meaning of the parameters



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8659>

    Explicitly say what effect this has on the date value already held in the instance



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8655>

    year -> date



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8660>

    Does the ISO week number cater for different calendar systems? More explanation would save the developer having to consult the (unreferenced) ISO standard - refer to weekNumber() which explains it.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8661>

    Can a constructor really set a different locale?



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8662>

    Same comment as for currentDate()



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8664>

    Julian Date -> Julian Day



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8663>

    Same comment as for currentDate()



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8665>

    What happens for an invalid date?



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8666>

    The the -> If the



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8668>

    I wonder about the method name. Possibly addYearsTo() instead? Similarly for addMonthsOn() and addDaysOn().



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8667>

    The the -> If the



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8669>

    The the -> If the



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8670>

    What are the implications of two dates which use different calendar systems? How are years, months and days calculated in that case?
    
    Similarly for the other difference methods.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8671>

    State that the last day of the month is a special case. (I presume that the second last day of the month won't be treated in the same way.)



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8672>

    Explain ISO months - refer to weekNumber() which explains it.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8673>

    I don't know whether there are any calendar systems which have a week different from 7 days. If so, the method should be called isoWeekNumber().



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8674>

    State whether this means a leap year in the calendar system being used, or in the Gregorian system.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8676>

    Does this mean the first day of the year of the currently set date?
    
    Similar comment for the other first...Of...() and last...Of...() methods.



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8677>

    @see yearInEra()



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8679>

    literal -> as a numeric string? (if not, explain)



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8678>

    ISO week number? As above, possibly should be called isoWeekNumberString().



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8682>

    What does "You should almost always translate your format string as documented" mean?



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8683>

    Spelling "comaptability"



/trunk/KDE/kdelibs/kdecore/date/kdate.h
<http://svn.reviewboard.kde.org/r/5692/#comment8684>

    componants -> components


- David


On 2010-10-25 20:54:08, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5692/
> -----------------------------------------------------------
> 
> (Updated 2010-10-25 20:54:08)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The KCalendarSystem api for localizing dates is awkward, inconvenient, unintuitive, and long-winded, causing many mistakes to be made or localization to be ignored altogether.  This change adds a new KDate class designed to make localizing dates as easy as using QDate.
> 
> Some QDate code may look like:
> 
>     QDate myDate( aYear, aMonth, aDay );
>     int doy = myDate.dayOfYear();
> 
> The KDE localized date code looks something like:
> 
>     QDate myDate;
>     KGlobal::locale()->calendar()->setDate( myDate, aYear, aMonth, aDay );
>     int doy = KGlobal::locale()->calendar()->dayOfYear( myDate );
> 
> The localized KDate code would look like:
> 
>     KDate myDate( aYear, aMonth, aDay );
>     int doy = myDate.dayOfYear();
> 
> Much easier.
> 
> KDate is a thin wrapper around KCalendarSystem and QDate, with a near identical api to QDate and as such can be used as a drop-in replacement with very few changes.  Some deprecated or unnecessary KCalendarSystem methods have not been included, but these can still be accessed via the calendar() methods.  Some new convenience methods have also been added such as setCurrentDate() and addYearsOn().
> 
> Some methods have QDate overloads for convenience, and the assignment and comparison operators partially work with QDate on the rhs.  If anyone knows how to make it work with QDate on the lhs, or any other QDate compatibility ideas, I'm all ears.
> 
> For now I only intend this to be used as a convenience class by apps internally and not to be used in kdelibs api as I don't see much advantage in that, but I may do so if the demand for convenience is there.
> 
> I have named it KDate, but there is the possibility people may get confused and think that KDateTime also localizes datetime's, but that is not the case.  If people think this will be a problem KLocalizedDate is an alternative if more awkward name.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/CMakeLists.txt 1189756 
>   /trunk/KDE/kdelibs/kdecore/date/kdate.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/date/kdate.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/tests/kcalendartest.h 1189756 
>   /trunk/KDE/kdelibs/kdecore/tests/kcalendartest.cpp 1189756 
> 
> Diff: http://svn.reviewboard.kde.org/r/5692/diff
> 
> 
> Testing
> -------
> 
> Full unit tests included.
> 
> 
> Thanks,
> 
> John
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101026/2fd7d643/attachment.htm>

More information about the kde-core-devel mailing list