Review Request: KLocale - Move to fully private implementation

[John Layt]

> On 2010-07-20 09:24:06, David Faure wrote:
> > 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)
> 
> John Layt wrote:
>     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.
> 
> David Jarvie wrote:
>     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.

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?


- John


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4683/#review6647
-----------------------------------------------------------


On 2010-07-18 21:22:04, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4683/
> -----------------------------------------------------------
> 
> (Updated 2010-07-18 21:22:04)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/CMakeLists.txt 1151073 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.h 1151073 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1151073 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1151073 
>   /trunk/KDE/kdelibs/kdecore/localization/klocalemacprivate.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/localization/klocalemacprivate_p.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/localization/klocaleprivate_p.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/localization/klocalewindowsprivate.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdecore/localization/klocalewindowsprivate_p.h PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4683/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> John
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100721/0b73cd41/attachment.htm>