<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>
</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;">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>
<br />
<p>- Albert</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>