Review Request: KLocale: Common localization of AM/PM text

[John Layt]

> On 2010-10-16 23:42:16, Albert Astals Cid wrote:
> > In general i think you are not following the kdelibs coding style, e.g. if ( foo ) needs to be if (foo) afair
> > 
> > Also wondering if we really need the KDayPeriod constructorand the setDayPeriod functions to be public, would anyone really need to use them? Can't we just make them private and friends for KLocalePrivate or the class that really needs to use them?
> > 
> > Other than that and the few fixlets in the other comments it seems nice and autotested :-)

Thanks Albert, knew you could be relied upon to find my const mistakes :-)

The two scenarios I can think of for external use of the constructor and setDayPeriod() are in the Locale KCM to set the users choice, and if an app wanted to do some fancy time formatting different from the default locale am/pm, e.g. "3 in the afternoon".  The KCM I guess could be done with a friend (is that ok for a class living in kdebase?), and the other could be done using a KConfig passed into the KLocale constructor like I did in the unit tests (although it's not exactly dev-friendly, it's not exactly a common requirement either).

I had initially hoped to keep KDayPeriod itself entirely private, I was a little reluctant to expose the internal workings when I'm not sure of its utility just yet.  I'll have a think if using friends will enable me to keep it completely private and internal.


- John


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


On 2010-10-16 19:59:29, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5639/
> -----------------------------------------------------------
> 
> (Updated 2010-10-16 19:59:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This change moves the localization of AM/PM into KLocale allowing for a single consistent translation that can be overridden at system, country or user level.
> 
> Currently AM/PM text is translated in several different places which could lead to inconsistencies.  The translation is also only done at language level, meaning country level localization cannot be performed and users cannot modify them to their personal preference.  Further, it will become a problem on the Mac and Windows platforms when we switch to use the platform localization settings as they do provide a common localization which users can override.
> 
> Initially I implemented simple amText() and pmText() methods, but further research found that the Unicode CLDR project defines something called Day Periods (http://www.unicode.org/reports/tr35/tr35-15.html#DayPeriodRules) to cater for cultures that split the day into more than 2 periods, so I have implemented that instead.  If people feel it's too complicated a solution I can easily switch back to the amText()/pmText() solution.
> 
> Note I have changed our default text from lowercase to uppercase AM/PM.  POSIX, Unicode, Windows and Mac all default to uppercase, the POSIX %p format symbol is defined as uppercase, with the more recent %P defined as lowercase.
> 
> KDateTime is not yet changed to use this, that will happen when the shared date/time format routines are completed and djarvie approves.
> 
> The KCM will be modified to allow users to override the value in a later change.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/CMakeLists.txt 1186497 
>   /trunk/KDE/kdelibs/kdecore/date/kdatetimeformatter.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/date/kdayperiod.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/date/kdayperiod.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.h 1186497 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_mac.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_p.h 1186497 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_win.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/tests/klocaletest.h 1186497 
>   /trunk/KDE/kdelibs/kdecore/tests/klocaletest.cpp 1186497 
>   /trunk/KDE/kdelibs/kdecore/tests/klocaletimeformattest.cpp 1186497 
> 
> Diff: http://svn.reviewboard.kde.org/r/5639/diff
> 
> 
> Testing
> -------
> 
> Full unit tests written for new code.  All existing tests passed after having am/pm changed to AM/PM.
> 
> 
> Thanks,
> 
> John
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101019/8ad99647/attachment.htm>