[Kde-pim] Review Request: Replace hard coded country list of kaddressbook by the l10n resources maintained in systemsettings

[Urs Joss]

> On 2009-05-15 04:47:55, Thomas McGuire wrote:
> > Hi! It looks like you got that code from kdebase/runtime/kcontrol/locale/countryselectordialog.cpp, right?
> > I don't like code duplication much. Can you add a new function to kdelibs that returns the list of country names and tags, and then make the country selector dialog and kaddressbook use it?
> > 
> > Thanks,
> > Thomas
> 
> Thomas McGuire wrote:
>     Heh, I should actually read stuff before I reply, that would have saved me from asking on IRC about where the widget that does the same lives :)
> 
> Urs Joss wrote:
>     Whoaah, hi Thomas
>     
>     Thanks for the suggestion, I absolutely agree about code duplication. It just did not cross my mind at all to even *touch* kdelibs in my second contribution here ;-)
>     
>     It was relatively easy to strip down the konctrol code (it groups by region and only lists the countries that apply to each region), but I will look into generalizing a function. I won't be able to check this out within the next couple of days though. Can this request just sit here like this or do I have to do something with it in the mean time?
>     
>     Thanks and cheers
>     Urs
> 
> Thomas McGuire wrote:
>     The request can simply sit here, no problem.
> 
> Thomas McGuire wrote:
>     Actually, turns out there is already such a function, see
>     http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKLocale.html#b0b405544e895b19bd136f072c781d8b

Hi Thomas

Thanks for the hint, this seems to work - at least in my backport to 4.2.3.

IMHO it makes not too much sense to try to port this code snippet to kdebase/runtime/kcontrol/locale/countryselectordialog.cpp, as there the functionality is needed to parse the region. It seems that KLocale does not support that (yet). Don't know if the ability to parse countries by region is needed anywhere else.


- Urs


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


On 2009-05-15 12:37:09, Urs Joss wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/700/
> -----------------------------------------------------------
> 
> (Updated 2009-05-15 12:37:09)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Inspired by bug 168410 I adapted the source for the country list used in countryselectordialog.cpp of kcontrol (roughly lines 271 to 290). Thus kaddressbook reads the system wide country names from as found in the entry.desktop files of the l10n subdirectories.
> 
> I did not try to follow up the rationale of implementing a hard-coded list in addresseditwidget.cpp. While it certainly gives kaddressbook developers more freedom in adding countries, it also led to outdated countries. The patch removes the burden of maintaining this list.
> 
> 
> This addresses bug 168410.
>     https://bugs.kde.org/show_bug.cgi?id=168410
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepim/kaddressbook/addresseditwidget.cpp 968241 
> 
> Diff: http://reviewboard.kde.org/r/700/diff
> 
> 
> Testing
> -------
> 
> I have applied the patch to the kaddressbook-4.2.3 in gentoo. I have successfully modified existing addresses (switching countries). I have also added new addresses to existing contacts, and last but not least created new contacts with new addresses.
> 
> 
> Thanks,
> 
> Urs
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/