[Kde-pim] [PATCH] kmail recipient picker fixes

[Thomas McGuire]

Hello,

On Tuesday 13 November 2007, Sander van Grieken wrote:
> On Monday 12 November 2007 20:15:12 Thomas McGuire wrote:
> > On Sunday 11 November 2007, Sander van Grieken wrote:
> > > Sorry to post a big patch again, but please disregard previous patch
> > > and use this one.
> >
> > OK, I've commited it now with revision 735840. I've made some small
> > adjustments (adding const, replacing the deprecated replace() and style
> > fixes) before commiting.
> >
> > However, there is still one problem, which leads to a crash. If you have
> > two resources with the same name (try with kcmshell4 kresources), it will
> > crash. The reason is probably insertCollection(), which compares by name,
> > which won't work in this case.
> > I suggest comparing by some unique ID string instead. I think it is best
> > if the collection ID string is the same as the normal name, expect for
> > resources, where it probably can be Resource::identifier(), which is
> > supposed to be unique (see
> > http://api.kde.org/4.0-api/kdepimlibs-apidocs/kresources/html/classKRES_1
> >_1 Resource.html ).
> >
> > There was (is?) a bug in KDEPIM that there were two resources with the
> > same name by default, so this is not unimportant.
>
> Yep that would be much better. I'll look into it.
Thanks for the patch, I'll review it later.

> > I also noticed (but didn't try) that the code won't work if you have the
> > same category in multiple resources. In this case, it seems like only the
> > addressees from the last inserted addressbook are inserted into the
> > category in this case. The old code was also buggy, there the categorys
> > would be created twice. I think the category logic in insertAddressBook()
> > needs some adjustments. Maybe there should be a map in RecipientsPicker
> > which maps category names to collections (similar to the local one, but
> > on the class-level). Then in insertAddressBook(), try to get an existing
> > category collection if one exits, else create a new one, then add the
> > recipient.
>
> I think it's always the same AddressBook instance that gets passed. If
> that's true, it seems like it's working correctly. Or is there something
> like multiple addressbooks?
No, there are no multiple addressbooks, I was wrong here, sorry. That happens 
if you read the code but don't try it out...

> > I hope you understood my confused writing here :)
> >
> > Please fix this, at least the crash with two resources with the same name
> > (against the new trunk version, so the patch contains only those
> > changes).
> >
> > Thanks again for the patch, it is always good to see problems in trunk
> > get fixed.
>
> It was an itch in need of scratching :)
>
> I was also hoping this would find its way back into 3.5
Well, then there needs to be separate patch, as the code in 3.5 is probably 
different. For example, I only had the issue with the duplication in trunk. I 
am myself only involved in trunk really, and don't really care to much about 
3.5. However, I think if you send a patch for 3.5, somebody else will 
probably commit it (and I'll help to review it).

Regards,
Thomas
_______________________________________________
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/