<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>
<p>On July 21st, 2010, 11:35 a.m., <b>John Layt</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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>
</blockquote>
<p>On July 21st, 2010, 12:13 p.m., <b>David Jarvie</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>File names ending in private_p.h seem really strange - "private" and "_p" both mean the same thing, that it's a private class. If the class is called FooPrivate, I'd expect the header to be foo_p.h.</pre>
</blockquote>
</blockquote>
<pre>While mostly used for the d-> Private class header, the _p.h is also sometimes used with 'normal' internal classes to indicate that they are not part of the public api and shouldn't be installed or used.
I could have left the KLocalePrivate implementation in the klocale.cpp file, but decided it was tidier to split it out, hence calling it klocaleprivate.cpp as klocale_p.cpp felt wrong to me, which meant I needed klocaleprivate_p.h to match it.
Generally, I prefer one class per file, filename is the classname, _p on the .h for non-public classes, but it all gets muddied with the d-> Private class implementation.
I guess we need to be more explicit on the naming standard in http://techbase.kde.org/Policies/Library_Code_Policy, does Qt have rules written down somewhere?</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>