[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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic