Review Request: KLocale: Keep track of original KSharedConfig/KConfig for later use.

[Oswald Buddenhagen]

> On 2010-12-05 09:13:59, Oswald Buddenhagen wrote:
> > 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 ...
> >
> 
> John Layt wrote:
>     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.

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.


- Oswald


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


On 2010-12-05 02:08:07, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6047/
> -----------------------------------------------------------
> 
> (Updated 2010-12-05 02:08:07)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale.cpp 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_mac.cpp 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_mac_p.h 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_p.h 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_unix.cpp 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_unix_p.h 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_win.cpp 1203620 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_win_p.h 1203620 
> 
> Diff: http://svn.reviewboard.kde.org/r/6047/diff
> 
> 
> Testing
> -------
> 
> All unit tests pass, my problem with KCalendarSystem not loading user settings when created by a KLocale is fixed.
> 
> 
> Thanks,
> 
> John
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101205/9d7ec869/attachment.htm>