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

[Sander van Grieken]

On Sunday 11 November 2007 01:17:25 Thomas McGuire wrote:
> Hi Sander,
>
> On Saturday 10 November 2007, Sander van Grieken wrote:
> > This is the first time I'm posting to this list. I understand that this
> > is the place to be if one wants to submit patches?
>
> Yes, this is the right place.
>
> > I've created a patch to kmail/recipientspicker that fixes crashes when
> > receiving addressbook-changed events from kaddressbook, fixes duplicate
> > resource and recipient entries and fixes some memory management problems.
>
> Thanks for the patch! I tried it, but as soon as I open the recipient
> picker, KMail crashes with the backtrace below (without the patch, it
> doesn't crash). But just before that, I could see that the duplicated
> entries are indeed gone :)

Hi Thomas,

Thanks for testing it, and for your swift response!

> Some things I noticed:
> - What happens if a recipient has multiple e-mail addresses? According to
> the logic of genKey(), he will be added to the collection only once, since
> the key is made with the preferred e-mail address. However, there should be
> one entry for each e-mail address. (I am not sure about the actual behavior
> since I couldn't test it) - genKey() should be generateKey()

Thanks, I didn't notice that. 

> - You sometimes don't adhere to the (admittedly strange) coding style of
> KMail with
>
>   regard to parenthesis. For example:
>   > if (!mDistributionList.isEmpty())
>
>   should be
>
>   >if ( !mDistributionList.isEmpty() )
>
>   or
>
>   > mAllRecipients->addItem(*rcptIt);
>
>   should be
>
>   > mAllRecipients->addItem( *rcptIt );
>
>   But this is minor, and you did it correctly in most places.

Should be all ok now :)

> As for the crash, I think the reason is that a recipient can be in multiple
> collections. For example, if you put a person in your address book into a
> category, he will be in two collections, the one for the catogory and the
> one for the resource. This might be problematic, because it looks like the
> code then double-deletes the items in ~RecipientsPicker(). But again, I am
> not sure and might be wrong, I didn't have time to look at this more
> toroughly.

Aah, yes I know where this crash comes from. The old implementation put 
everything in the (derived) mAllRecipients list and used that as 
the 'datastore' and treated the other lists as only reference containers. The 
new implementation inverts this and makes all lists datastores except for 
mAllRecipients, which is now only a reference container. 

The crash comes from the fact that the category lists are also reference 
containers, resulting in the double-free because they are still treated like 
a datastore in the destructor..

> Again, thanks for the work, I hope you'll come up with an improved version
> of the patch.

Here it is. Fixing the above problem also exposed another problem with 
distribution list entries that crash after an addressbook-changed event, so 
I've also fixed that.

grtz,
Sander

-------------- next part --------------
A non-text attachment was scrubbed...
Name: recipientspicker-fixes.patch
Type: text/x-diff
Size: 19719 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20071111/9f5ff99b/attachment.patch>
-------------- next part --------------
_______________________________________________
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/