[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: KLocale: Common localization of AM/PM text
From: "John Layt" <johnlayt () googlemail ! com>
Date: 2010-10-21 20:28:40
Message-ID: 20101021202840.25761.48499 () vidsolbach ! de
[Download RAW message or body]
> 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 :-)
>
> John Layt wrote:
> 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.
> Albert Astals Cid wrote:
> I don't think friending in kdelibs a class that exist in kdebase makes sense.
>
> Also i seems to me that setDayPeriod is not permanent (only sets a variable) so it \
> can not be used to set the users choice, right?
And even if the kcm did friend, it would still need a public header file, so overall \
not something I'll do.
The KCM saves changes by writing to the config file, but it also uses the api to set \
the example displayed to the user before the setting gets saved. However, I'm \
rewriting the kcm so I'll try to find a workaround using config first before forcing \
everything public.
I'll make everything private for now including KDayPeriod, then expose the minimal I \
can get away with, which in KLocale will initially be the equivalent of amText() and \
pmText() calls:
QString dayPeriodText(const QTime &time, DateTimeComponentFormat format = \
ShortName) const;
- 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
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5639/">http://svn.reviewboard.kde.org/r/5639/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 16th, 2010, 11:42 p.m., <b>Albert \
Astals Cid</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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 :-)</pre> </blockquote>
<p>On October 19th, 2010, 8:09 p.m., <b>John Layt</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
<p>On October 20th, 2010, 6:04 p.m., <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't think \
friending in kdelibs a class that exist in kdebase makes sense.
Also i seems to me that setDayPeriod is not permanent (only sets a variable) so it \
can not be used to set the users choice, right?</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">And even if the kcm did \
friend, it would still need a public header file, so overall not something I'll \
do.
The KCM saves changes by writing to the config file, but it also uses the api to set \
the example displayed to the user before the setting gets saved. However, I'm \
rewriting the kcm so I'll try to find a workaround using config first before \
forcing everything public.
I'll make everything private for now including KDayPeriod, then expose the \
minimal I can get away with, which in KLocale will initially be the equivalent of \
amText() and pmText() calls:
QString dayPeriodText(const QTime &time, DateTimeComponentFormat format = \
ShortName) const; </pre>
<br />
<p>- John</p>
<br />
<p>On October 16th, 2010, 7:59 p.m., John Layt wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kdelibs.</div>
<div>By John Layt.</div>
<p style="color: grey;"><i>Updated 2010-10-16 19:59:29</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Full unit tests written for new code. All existing tests passed after \
having am/pm changed to AM/PM.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdelibs/kdecore/CMakeLists.txt <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/date/kdatetimeformatter.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/date/kdayperiod.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/date/kdayperiod.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale.h <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_mac.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_p.h <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_win.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/tests/klocaletest.h <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/tests/klocaletest.cpp <span style="color: \
grey">(1186497)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/tests/klocaletimeformattest.cpp <span style="color: \
grey">(1186497)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5639/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic