Too many onItemRemoved notifications are sent, if the AB contains lists.
Categories
(Thunderbird :: Address Book, defect)
Tracking
(thunderbird68 fixed, thunderbird69 fixed)
People
(Reporter: TbSync, Assigned: TbSync)
Details
Attachments
(1 file, 2 obsolete files)
10.99 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36
Steps to reproduce:
Add a contact to an empty addressbook
Add 2 lists to the same addressbook, but keep the lists empty (no members)
Delete the just created contact again
Actual results:
The onItemRemoved
AB listener gets notified 3 times:
- 1x for the original contact
- 1x for each list (aItem being the contact and aParentDir being the list with
.isMailList = true
) even though the contact was not part of that list.
Expected results:
The onItemRemoved
AB listener should only get notified for the true deletion in the addressbook and for the removal from a list, if he really was part of a list.
Alternativly, a new addrbook-list-member-removed
observer would solve this.
Updated•6 years ago
|
Comment 1•6 years ago
|
||
This seems to be the culprit. It needs some sort of check that aCard
is a member of this
, but that may not be straightforward.
Assignee | ||
Comment 2•6 years ago
|
||
I traced it into here:
https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAddrDatabase.cpp#1509
If I comment out that line, I do not get any notifications from the MLs. So it must be something inside DeleteCardFromAllMailLists
.
Or is that the wrong path?
Assignee | ||
Comment 3•6 years ago
|
||
Not true, testing tricked me.
Comment 4•6 years ago
|
||
We can't check a mailing list to see if the card is in it, because it isn't. The card has already been removed. That's a problem in my plan to fix this bug.
Assignee | ||
Comment 5•6 years ago
|
||
The main reason I need this fixed, is to be able to learn if a member has been removed from a list. That is currently not possible.
How about adding a new dedicated observer notification, like addrbook-list-member-removed, which could be send for each ML the card is part of, before the card is actually deleted? Would that be possible?
Assignee | ||
Comment 6•6 years ago
|
||
I tried :-)
I did move the call to DeleteCardFromAllMailList
a bit upwards, so I can use the nsIAbCard (it gets reset later on).
If that is not possible, than I would just pass on the UID of the card include that in the observer notification, instead of the card itself.
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
Updated•6 years ago
|
Comment 9•6 years ago
|
||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
Regarding the *cardFound
bool, it is an optional parameter, so it might not be present, can I still assign a value to it, without causing a crash? That is why I checked for NULL and did not touch it, if it is NULL.
Are we using C++ 11?
Geoff, do you handle this?
Comment 12•6 years ago
|
||
It's never null, since you assign it a value in the method signature. That's the
bool *cardFound = false) part
Comment 13•6 years ago
|
||
I finally have a working clang-format
again, and it didn't like my formatting!
Comment 14•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 15•6 years ago
|
||
Thanks for your help, Geoff!
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
Bonus points for apparently having run the clang-formatter before submitting the patch !
Comment 18•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0f025fb08630
Adding addrbook-list-member-removed observer notification (test by darktrojan). r=mkmelin
Updated•6 years ago
|
Assignee | ||
Comment 19•6 years ago
|
||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/dd314c2df68a5c00f363b4a8a0b21961b046cea1
Description
•