<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/6047/">http://svn.reviewboard.kde.org/r/6047/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 5th, 2010, 9:13 a.m., <b>Oswald Buddenhagen</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;">the kconfig magic is a monstrosity, but it seems reasonably safe.
though i wonder whether it wouldn't be better to get everything you may later need from the config and then just throw it away ...
</pre>
</blockquote>
<p>On December 5th, 2010, 11:27 a.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;">I had thought of caching the user settings, but they are at a per-calendar sub-group level, so that's 10 calendars x 2 or 3 settings each = 20 to 30 extra values that would need to be stored as int's and QStrings when most of the time they will be unused or the system default anyway. I decided in the vast majority of cases, i.e. when using the global config with no user calendar settings, that a null Ptr was the most efficient solution.
I do wonder if I need to deprecate the constructor and methods that take a KConfig* and have new ones taking a KShardedConfig::Ptr instead for consistency and safety.</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 wouldn't deprecate anything in this commit.
in the bigger picture, in kde5 i'd like to merge kconfig with ksharedconfig, so it would probably make sense to deprecate kconfig-taking functions - throughout kdelibs.
</pre>
<br />
<p>- Oswald</p>
<br />
<p>On December 5th, 2010, 2:08 a.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-12-05 02:08:07</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;">KLocale can be constructed with reference to a config file, variously passed in as either a KSharedConfig::Ptr or KConfig*, or just defaulting to the global, however KLocale doesn't keep track of the original config used as it hasn't needed to. Now however KCalendarSystem also reads some user settings from the config file, but the delayed creation of the calendar object by KLocale means the config file is not available when needed and so those settings are effectively ignored. Changing the calendar system also requires the config for the new calendar system to be read.
This fix keeps a KSharedConfig pointer to the config file in KLocale (null if using the global) and cleans up some use of the config itself. However I'm a little uncertain I've done the right thing when a KConfig* is passed in rather than a KSharedConfig::Ptr, taking a copy of the KConfig in a new KSharedConfig seemed the only way, unless someone knows a better option. I'm also not sure when doing the copy if the new KSharedConfig should use the same file name as the original KConfig, a null QString(), or something else?
Note: the apidox already warns that when passing the KSharedConfig::Ptr in that it must exist for the life of the KLocale object, but there's no such warning on the KConfig* constructor or setCountry/setLanguage methods.</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;">All unit tests pass, my problem with KCalendarSystem not loading user settings when created by a KLocale is fixed.</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/localization/klocale.cpp <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_mac.cpp <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_mac_p.h <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_p.h <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_unix.cpp <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_unix_p.h <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_win.cpp <span style="color: grey">(1203620)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale_win_p.h <span style="color: grey">(1203620)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/6047/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>