<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://reviewboard.kde.org/r/4683/">http://reviewboard.kde.org/r/4683/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 20th, 2010, 9:24 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>Looks good. I was just surprised by the naming of the files, since usually (in Qt) a mac specific file would be called foo_mac.cpp / foo_mac_p.h rather than foomacprivate.cpp / foomacprivate.h
(example where a derived private class is defined: qeventdispatcher_mac_p.h:class QEventDispatcherMacPrivate)</pre>
</blockquote>
</blockquote>
<pre>I did wonder about the naming, but the foo_win.cpp examples I looked at all seemed to be cases where they are complete separate re-implementations of the class foo for win (i.e. the class name really is foo and the _win is needed to avoid file name clashes), rather than derived classes (i.e. the foowin is the real name of the class and there is no file name clash). However your example disproves that theory :-) Not fussed either way.</pre>
<br />
<p>- John</p>
<br />
<p>On July 18th, 2010, 9:22 p.m., John Layt wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 and David Faure.</div>
<div>By John Layt.</div>
<p style="color: grey;"><i>Updated 2010-07-18 21:22:04</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;">Move KLocale to a fully private implementation model. This is to enable the progressive implementation of separate private classes for Windows and Mac platforms that will override the base KDE implementation and directly access the host system locale functions while still using the KDE unique locale methods where necessary.
1) Separate the KLocalePrivate class out to own .h and .ccp files
2) Make all KLocalePrivate data members private, all access to be via virtual public methods
3) Move all method implementations into KLocalePrivate virtual methods
3) Make all KLocale methods point to KLocalePrivate methods only, no direct access to data members or implementation details
4) Create derived KLocaleWindowsPrivate and KLocaleMacPrivate classes
Most implementations are unchanged other than the minimum required, except those where Win/Mac specific code has been moved to the Win/Mac classes.
Particular attention should be taken of the copy ctor and assignment op which I'm not 100% on.
Note I did 'svn cp' the klocale.cpp to the klocaleprivate.cpp but not to the .h or the Win/Mac .h and .cpp as the diff just became too huge and difficult to read.</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;">Ran lots of existing unit tests. Not yet tested compile on Windows or Mac, will do that once review approved as it requires some work to set up.</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">(1151073)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale.h <span style="color: grey">(1151073)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp <span style="color: grey">(1151073)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocale.cpp <span style="color: grey">(1151073)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocalemacprivate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocalemacprivate_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocaleprivate_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocalewindowsprivate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdelibs/kdecore/localization/klocalewindowsprivate_p.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4683/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>