Add new notifications we need for address book API

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 64.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6064+ fixed, thunderbird64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 months ago
There's some places in the address book code where the existing notifications just don't cut it. I'm adding some new ones using the observer service instead of the current mechanism to avoid breaking something. Perhaps in future we can convert all of these to observer service notifications.
Assignee

Comment 1

9 months ago
Attachment #9008364 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9008364 [details] [diff] [review]
1490626-addressbook-notifications-1.diff

Review of attachment 9008364 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except the naming :) r=mkmelin

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +670,5 @@
> +    nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +    if (observerService) {
> +      nsCString thisUID;
> +      this->GetUID(thisUID);
> +      observerService->NotifyObservers(card, "ab:contactCreated", NS_ConvertUTF8toUTF16(thisUID).get());

A topic naming convention as ab-contact-created, ab-contact-updated and so on seems more common.
Attachment #9008364 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 3

8 months ago
New notification names and using nsAutoCString instead of nsCString for local variables.
Attachment #9008364 - Attachment is obsolete: true
Attachment #9008982 - Flags: review+

Comment 4

8 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/60d7adb0d3bd
Add new notifications we need for WebExt address book API; r=mkmelin
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED

Updated

7 months ago
Target Milestone: --- → Thunderbird 64.0
Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

> NS_IMETHODIMP nsAbMDBDirectory::DropCard(nsIAbCard* aCard, bool needToCopyCard)
...
>     mDatabase->CreateNewListCardAndAddToDB(this, m_dbRowID, newCard, false /* notify */);
>+    nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
>+    if (observerService) {
>+      nsAutoCString thisUID;
>+      this->GetUID(thisUID);
>+      observerService->NotifyObservers(newCard, "addrbook-list-member-added", NS_ConvertUTF8toUTF16(thisUID).get());
>+    }
>   }
>   else {
>     mDatabase->CreateNewCardAndAddToDB(newCard, true /* notify */, this);
>   }
No notification for the else case (dragging and dropping a card from one addressbook to another). (I didn't test dragging from one addressbook to a list because I haven't got as far as lists yet.)
Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

We would like to ship our addon for TB 60 that uses the new API. (We had before created our own WebExperiment, which was then obsoleted by this nice new Thunderbird API.) The addon is "Owl for Exchange".

We can import the WebExtension API as WebExperiment into our addon, but we still need these notifications from TB for edits to work. It would therefore be nice for this patch to be backported to TB 60.

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined:
Users of our addon will not be able to edit contacts that come from Exchange via our addon. Edits will be lost and overwritten after restart / re-login.

Testing completed (on c-c, etc.):
Landed on TB trunk a while ago.

Risk to taking this patch (and alternatives if risky):
This adds only new notifications that didn't exist before. Therefore, nothing in Thunderbird 60 should be listening to the notifications. Thus, this is only new code and should be safe to backport.
Attachment #9008982 - Flags: approval-comm-esr60?
> No notification for the else case (dragging and dropping a card from one addressbook to another).

@Neil: Can you please file a follow-up bug on this, and mention the bug number here?

Comment 8

7 months ago
(In reply to Ben Bucksch (:BenB) from comment #6)
> We would like to ship our addon for TB 60 that uses the new API ...
OK, the code doesn't quite apply to comm-esr60, but there is only a trivial merge conflict. This rely on the ID thing, bug 1482040?
OK. We'll look at it, and backport it as necessary.
Assignee

Comment 10

7 months ago
Yes, you will need bug 1482040 to make the API and the notifications work. I'm tempted to say just backport the API as well, but I'm not sure that's a good idea.

Updated

7 months ago
Depends on: 1503218

Comment 11

7 months ago
Comment on attachment 9008982 [details] [diff] [review]
1490626-addressbook-notifications-2.diff

Good for 60.4.
Attachment #9008982 - Flags: approval-comm-esr60? → approval-comm-esr60+

Updated

6 months ago
Depends on: 1511885

Comment 13

6 months ago
Backed out from the ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/d6e310cff401f6945897aad017e4663dfc9f40cc
Backed out changeset 73ed0515afc2 (bug 1490626) for causing address book and auto-complete slowness (bug 1511885). a=backout
You need to log in before you can comment on or make changes to this bug.