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

List:       kde-core-devel
Subject:    Re: Review Request: Switch implementation of formatDate() and
From:       "John Layt" <johnlayt () googlemail ! com>
Date:       2009-08-17 16:11:02
Message-ID: 20090817161102.8725.70066 () localhost
[Download RAW message or body]


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

(Updated 2009-08-17 16:11:02.234419)


Review request for kdelibs.


Changes
-------

Bump.  Any comments?  I need to commit this so I can put my Plasma Clock/Calendar \
changes up for review.  If no response I'll commit Tuesday.


Summary (updated)
-------

[Bump.  Any comments?  I need to commit this so I can put my Plasma Clock/Calendar \
changes up for review.  If no response I'll commit Tuesday.]

This review request is more for the refactoring model to see if there is a scenario I \
have missed, rather than for the code which is mostly a cleaned-up copy-and-paste.

Currently the KCalendarSystem implementations of formatDate() and readDate() point to \
the KLocale implementations.  The problem with this is that the KLocale \
implementations in turn point to the global calendar when obtaining date components \
like month name or year.  This is fine when you only ever show the global calendar, \
but if you want to show a different calendar at the same time, e.g. the Hebrew \
calendar alongside the global Gregorian, then calling KCalendarSystem::formatDate() \
on the Hebrew calendar returns the Gregorian date and not the Hebrew date as \
expected.  There is a workaround to this by creating a new KLocale instance, creating \
the new KCalendarSystem using this and pointing the KLocale back at the new \
KCalendarSystem.  This however is an ugly hack and we shouldn't expect the app \
programmer to know to do this.  The app programmer would expect they can create a new \
KCalendarSystem instance and have the correct dates returned.  Only if they want \
dates formatted in a different format than the global format should they have to \
create a new KLocale first.

The solution is to reverse the situation by moving the formatDate() and readDate() \
implementations from KLocale into KCalendarSystem, with the KLocale implementations \
pointing to its internal calendar instance for formatDate() and readDate(), and \
KCalendarSystem using its correct internal date component routines and pointing back \
to KLocale only to get the layout formats.  Another advantage to this model is that \
any calendar system that needs non-standard date formatting rules can now override \
the formatDate() directly without adding complexity to the standard routines.

A couple of code clean-ups and bug-fixes are included, especially around FancyDate \
format and date validity checking.


Diffs
-----

  trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.h 1010188 
  trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.cpp 1010188 
  trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhebrew.cpp 1010188 
  trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1010188 

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


Testing
-------

All current klocaletest unit test cases for formatDaet(), formatDateTime(), \
readDate(), and readDateTime() pass the same as they did before the changes.

I have implemented support in the plasma calendar widget to allow the user to select \
what calendar system to display.  Without this change the wrong dates are \
displayed/read.  With these changes the correct dates are displayed/read.


Thanks,

John


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