Add UID property to address book interfaces

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

Trunk
Thunderbird 64.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6064+ fixed, thunderbird64 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Assignee

Description

10 months ago
This is the same patch I posted in bug 1469238. It really should be in its own bug, so here we are.

Updated

9 months ago
Version: unspecified → Trunk
Assignee

Updated

9 months ago
Blocks: 1487008
Assignee

Updated

9 months ago
No longer blocks: 1487008
Assignee

Comment 1

9 months ago
Updated patch to go with the other bug. It's the same as the first, but without the letter G.
Attachment #8998739 - Attachment is obsolete: true
Attachment #9005103 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9005103 [details] [diff] [review]
1482040-addressbook-guids-2.diff

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

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +108,5 @@
>     * implementations SHOULD NOT, in general, modify this property.
>     */
>    attribute AUTF8String localId;
>  
> +  attribute ACString UID;

probably better make it AUTF8String (like localId) above.
And please add documentation too.
Attachment #9005103 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Updated

9 months ago
Summary: Add GUID property to address book interfaces → Add UID property to address book interfaces
Assignee

Comment 3

9 months ago
I've not changed it to AUTF8String because there is no situation where it will need non-ASCII characters. I've documented what the contents of the property should be and stated that others should not change the value once set.
Attachment #9005103 - Attachment is obsolete: true
Attachment #9005159 - Flags: review+
Yes we're not perhaps expecting non-ASCII. But I think anything new should use UTF-8 for everything, so there's one thing less to worry about.
Comment on attachment 9005159 [details] [diff] [review]
1482040-addressbook-guids-3.diff

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

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +110,5 @@
>    attribute AUTF8String localId;
>  
>    /**
> +   * A unique identifier for this card.
> +   * It should be 9 random bytes, base 64 encoded: 12 ASCII characters.

I'll comment on the other bug

@@ +111,5 @@
>  
>    /**
> +   * A unique identifier for this card.
> +   * It should be 9 random bytes, base 64 encoded: 12 ASCII characters.
> +   * This attribute is set by the WebExtensions interface and

Hmm, we might want to use it internally too? At least in the future
Assignee

Comment 6

8 months ago
UIDs are now set on read if empty, and cannot be set by script.
Attachment #9005159 - Attachment is obsolete: true
Attachment #9008360 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9008360 [details] [diff] [review]
1482040-addressbook-guids-4.diff

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

Great, r=mkmelin
Attachment #9008360 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 8

8 months ago
Changed to keep the UID in memory as well as in the prefs for address books, so we still have it when making a delete notification.

Also some style changes I learned about shortly after submitting last time.
Attachment #9008360 - Attachment is obsolete: true
Attachment #9008981 - Flags: review+
Comment on attachment 9008981 [details] [diff] [review]
1482040-addressbook-guids-5.diff

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

Great that you fixed that!

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +154,5 @@
> +NS_IMETHODIMP nsAbDirProperty::GetUID(nsACString &aUID)
> +{
> +  nsresult rv = NS_OK;
> +  aUID = mUID;
> +  if (aUID.IsEmpty() && !m_IsMailList) {

nit: while the same thing, it's more logical to test mUID.IsEmpty() and then go on to assign as nesseary

But I actually had a question too. Why are mailLists treated differently here and in nsAbDirProperty::SetUID? The test you added says they do have an uid.
Assignee

Comment 10

8 months ago
(In reply to Magnus Melin [:mkmelin] from comment #9)
> nit: while the same thing, it's more logical to test mUID.IsEmpty() and then
> go on to assign as nesseary

You mean like I've done it here?

> But I actually had a question too. Why are mailLists treated differently
> here and in nsAbDirProperty::SetUID? The test you added says they do have an
> uid.

Address books store their UID in the prefs (ldap_2.servers.foo.uid) but mailing lists are stored in the mork database as an nsIAbCard. But mailing lists also implement nsIAbDirectory so the UID is copied from the card object to the directory object and we only keep it in memory.
Attachment #9008981 - Attachment is obsolete: true
Attachment #9011989 - Flags: review+
(In reply to Geoff Lankow (:darktrojan) from comment #10)
> You mean like I've done it here?

Yes, thx!

Comment 12

8 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2bab51e3b5c2
Add UID property to address book interfaces; r=mkmelin
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED

Updated

7 months ago
Target Milestone: --- → Thunderbird 64.0
Ben requested backport of bug 1490626 which depends on this bug. I needed to verify that the backport will indeed allow us to import the API from bug 1469238 as an Experiment API and use that in Thunderbird 60 after the backport. To save you from having to fix the not quite trivial whitespace changes twice, this is the diff I ended up using for ESR 60.
Attachment #9021487 - Flags: approval-comm-esr60?

Comment 15

7 months ago
Comment on attachment 9021487 [details] [diff] [review]
Whitespace changes for ESR 60 uplift

Sorry, I've fixed the white-space issues in ESR 60 now. Can you rebase this again, this time it will be a three-minute job. Next time, please ask.
Attachment #9021487 - Attachment is obsolete: true
Attachment #9021487 - Flags: approval-comm-esr60?
(In reply to Jorg K from comment #15)
> Sorry, I've fixed the white-space issues in ESR 60 now. Can you rebase this
> again, this time it will be a three-minute job. Next time, please ask.

I double-checked and the original patch now applies to nsAddrDatabase.cpp correctly but you didn't backport the white-space changes to nsAbDirProperty.* so there's still one white space conflict in each file.

Comment 17

7 months ago
You mean https://hg.mozilla.org/comm-central/rev/7692c0e19b39. I didn't intend to backport that, so your patch is welcome.
Comment on attachment 9021588 [details] [diff] [review]
Fewer whitespace changes for ESR 60 uplift

[Approval Request Comment]
User impact if declined:
Owl needs to support 2 different address book APIs, one for TB 60 and one for TB trunk.

Testing completed (on c-c, etc.):
Owl on TB 60 with this patch works.

Risk to taking this patch (and alternatives if risky):
Most of the patch adds new functions, so that should be safe.
nsAddrDatabase.cpp adds a few calls in existing code.
Attachment #9021588 - Flags: approval-comm-esr60?

Comment 20

7 months ago
Comment on attachment 9021588 [details] [diff] [review]
Fewer whitespace changes for ESR 60 uplift

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

Comment 22

6 months ago
As you can see, I've uplifted this now, but a test is not working:

TEST-UNEXPECTED-FAIL | comm/mailnews/addrbook/test/unit/test_uid.js | xpcshell return code: 0
TypeError: MailServices.ab.directories is not iterable at /builds/worker/workspace/build/tests/xpcshell/tests/comm/mailnews/addrbook/test/unit/test_uid.js:19

Line 19 is:   for (let book of MailServices.ab.directories) {

Can someone please provide a fix for this.
Flags: needinfo?(neil)
Flags: needinfo?(ggladden)

Updated

6 months ago
Flags: needinfo?(ggladden) → needinfo?(geoff)
Neil will look into it.
This fixes the test locally for me.
Flags: needinfo?(neil)

Comment 25

6 months ago
Comment on attachment 9026410 [details] [diff] [review]
Fix test failure on ESR60

Thanks for looking into it, Neil. I'll get Geoff to review it and I'll get it landed after that.

Layman's question: Would this work:
  for (let book of fixIterator(MailServices.ab.directories, Ci.nsIAbDirectory) {

I'm not sure how that could be applied to the second call site, perhaps
  let bookCards = [...fixIterator(book.childCards,Ci.nsIAbCard)];

Either way, it's just a small test fix.
Flags: needinfo?(geoff)
Attachment #9026410 - Flags: review?(geoff)
Assignee

Comment 26

6 months ago
Comment on attachment 9026410 [details] [diff] [review]
Fix test failure on ESR60

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

I'm happy with this. The fixIterator way should work too, but as this is a test on ESR, I'm really not bothered which you use.
Attachment #9026410 - Flags: review?(geoff) → review+
I didn't know whether fixIterator was in scope, as the other tests didn't seem to use it.

Comment 29

6 months ago
https://hg.mozilla.org/releases/comm-esr60/rev/a6e340fe4c07d16d962abab48404aad0d07848b8
Fix failure in test_uid.js by iterating MailServices.ab.directories 'manually'. r=darktrojan
Thanks, everybody

Updated

6 months ago
Attachment #9026410 - Attachment description: Test fix → Fix test failure on ESR60

Updated

6 months ago
Depends on: 1511885

Comment 31

6 months ago
Backed out from the ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/e4d520b5dc17e8a0e3ba4d760556b1ba4aad4842
Backed out changeset a6e340fe4c07 and 7fe1827e6b39 (bug 1482040) for causing address book and auto-complete slowness (bug 1511885). a=backout

Comment 33

4 months ago

Hi Geoff,

I tried to use the new UID but with mailing lists I run into an issue: the UID is not persistent and changes between requests. I am still working with old address book interface and not the new mailextension you are working with. Are you seeing the same thing?

I have a standard address book listener and in onItemPropertyChanged I just log the UID to the console, if Item.isMailList.

This may be related to a general (stupid) bug in Mailing lists cards, that they do not keep any property assigned to them except NickName. Can anyone confirm this?

I am on TB 60.4

Hi John, please file a new bug about that, and cc Geoff. (You can use "Clone This Bug" below)

Crash on Mac: bug 1523904

You need to log in before you can comment on or make changes to this bug.