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

[Albert Astals Cid]

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


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 :-)


/trunk/KDE/kdelibs/kdecore/date/kdayperiod.h
<http://svn.reviewboard.kde.org/r/5639/#comment8466>

    Since 4.6



/trunk/KDE/kdelibs/kdecore/localization/klocale.h
<http://svn.reviewboard.kde.org/r/5639/#comment8467>

    componant -> component?



/trunk/KDE/kdelibs/kdecore/localization/klocale.h
<http://svn.reviewboard.kde.org/r/5639/#comment8468>

    const &



/trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp
<http://svn.reviewboard.kde.org/r/5639/#comment8469>

    Make per
    iod stringlist const for faster []



/trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp
<http://svn.reviewboard.kde.org/r/5639/#comment8470>

    make dayPeriodText const so future readers see it doesn't change more easily



/trunk/KDE/kdelibs/kdecore/localization/klocale_p.h
<http://svn.reviewboard.kde.org/r/5639/#comment8473>

    const &? It seems you are only reading from cg



/trunk/KDE/kdelibs/kdecore/localization/klocale_p.h
<http://svn.reviewboard.kde.org/r/5639/#comment8471>

    const &



/trunk/KDE/kdelibs/kdecore/tests/klocaletest.h
<http://svn.reviewboard.kde.org/r/5639/#comment8472>

    spacing is off


- Albert


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/20101016/7c932a4d/attachment.htm>