1528655 - Fix gloda's unchangeable assignment of ' from me ' contact to be based on default account's default identity and make sure changes in Account Manager are reflected immediately.

Status: RESOLVED FIXED

(MailNews Core :: Search, enhancement)

Comment 1

[alta88]

Attachment: glodaDefaultContact.patch

Currently gloda will always assume the very first identity ever created (and not deleted) is the 'From me' contact. There isn't any way for a user to change this, unless they know how to reorder accounts/identities and rebuild the gloda db from scratch.

This patch:

1. Uses the default account's default identity as the From me (Gloda.myContact) contact.
2. Updates the gloda contact entry automatically if the current contact name is not the same as the default msgIdentity name.
3. Updates automatically (without restart) the contact if the default account is changed, or the default identity in the default account is changed.

The patch does not:

1. Update gloda for account/identity adds, updates, deletes. See Bug 475502, Bug 1333603, maybe others.

Comment 2

[alta88]

Attachment: glodaDefaultContact.patch

Comment 3

[alta88]

Attachment: glodaDefaultContact.patch

Also auto update each gloda identity with new name, if it differs from AM identity, not just the default contact. On restart or default account/identity change.

Comment 4

[:aceman]

Comment on attachment 9044709 [details] [diff] [review]
glodaDefaultContact.patch

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

Thanks, so this does not yet handle identity removals, but at least it add updating the ones in gloda if the matching identity changes in AM? For non-default identities that will happen at next startup (via Gloda._init() I assume)?

::: mailnews/base/prefs/content/AccountManager.js
@@ +759,5 @@
>    MailServices.accounts.defaultAccount = currentAccount;
>    markDefaultServer(currentAccount, previousDefault);
> +  // Update gloda's myContact with the new default account's default identity.
> +  if ("Gloda" in window.opener) {
> +    window.opener.Gloda._initMyIdentities();

Could you just import the Gloda module (ChromeUtils.import) and call that? Or will that not be right for some reason?

::: mailnews/base/prefs/content/am-identities-list.js
@@ +147,5 @@
>    // Rebuilt the identity list and select the moved identity again.
>    refreshIdentityList(0);
> +  // Update gloda's myContact with the new default indentity.
> +  if ("Gloda" in window.opener.parent.opener) {
> +    window.opener.parent.opener.Gloda._initMyIdentities();

Could you just import the Gloda module (ChromeUtils.import) and call that? Or will that not be right for some reason?

::: mailnews/db/gloda/modules/gloda.js
@@ +514,4 @@
>      let myContact = null;
>      let myIdentities = {};
> +    // Process each email at most once; stored here.
> +    let myEmailAddresses = {};

Looks like this could be a Set().

@@ +533,5 @@
>  
>      for (let iIdentity = 0; iIdentity < numIdentities; iIdentity++) {
>        let msgIdentity =
>          MailServices.accounts.allIdentities.queryElementAt(iIdentity,
>                                                             Ci.nsIMsgIdentity);

Looks like a job for fixIterator() as we do not use the 'iIdentity' counter and it could potentially be faster (hopefully a single call too MailServices.accounts.allIdentities).

@@ +536,5 @@
>          MailServices.accounts.allIdentities.queryElementAt(iIdentity,
>                                                             Ci.nsIMsgIdentity);
>  
> +      let emailAddress = msgIdentity.email;
> +      let replyTo = msgIdentity.replyTo;

Just moving these 2 so that 'let's are grouped on top?

@@ +564,3 @@
>              existingIdentities.push(identity);
> +            if (isDefaultMsgIdentity) {
> +              defaultIdentity = identity

Missing semicolon?

@@ +571,4 @@
>            myEmailAddresses[parsed.addresses[0]] = true;
>          }
>        }
>        if (replyTo) {

Are the 'emailAddress' and 'replyTo' code blocks the same?
They could be merged like 
for (let address of [emailAddress, replyTo]) {
 ...single instance of the code using 'address'...
}

@@ +589,3 @@
>              existingIdentities.push(identity);
> +            if (isDefaultMsgIdentity) {
> +              defaultIdentity = identity

Missing semicolon?

Comment 5

[alta88]

(In reply to :aceman from comment #4)

Comment on attachment 9044709 [details] [diff] [review]
glodaDefaultContact.patch

Review of attachment 9044709 [details] [diff] [review]:

Thanks, so this does not yet handle identity removals, but at least it add
updating the ones in gloda if the matching identity changes in AM? For
non-default identities that will happen at next startup (via Gloda._init() I
assume)?

Yes, exactly, _initMyIdentities() sets up each startup.

::: mailnews/base/prefs/content/AccountManager.js
@@ +759,5 @@

MailServices.accounts.defaultAccount = currentAccount;
markDefaultServer(currentAccount, previousDefault);

• // Update gloda's myContact with the new default account's default identity.
• if ("Gloda" in window.opener) {
• window.opener.Gloda._initMyIdentities();

Could you just import the Gloda module (ChromeUtils.import) and call that?
Or will that not be right for some reason?

Well, I assume that creates a new reference and memory, so I just went with pointing to an existing reference, especially since the dialogs are modal. Anyway, the better thing to do is send out a notification for new/change/delete of identity, but that should be fixed in Bug 475502. So I'd rather just do it fast here.

All the rest have been updated; there's so much to renovate in gloda and hardly any of the modern stuff was invented back then. If you're fine with that great, it does make a review a bit harder to see real changes so I initially avoid it.

Comment 6

[alta88]

Attachment: glodaDefaultContact.patch

Comment 7

[alta88]

Attachment: glodaDefaultContact.patch

some more modernizations.

Comment 8

[:aceman]

(In reply to alta88 from comment #5)

::: mailnews/base/prefs/content/AccountManager.js
@@ +759,5 @@

MailServices.accounts.defaultAccount = currentAccount;
markDefaultServer(currentAccount, previousDefault);

• // Update gloda's myContact with the new default account's default identity.
• if ("Gloda" in window.opener) {
• window.opener.Gloda._initMyIdentities();

Could you just import the Gloda module (ChromeUtils.import) and call that?
Or will that not be right for some reason?

Well, I assume that creates a new reference and memory, so I just went with pointing to an existing reference, especially since the dialogs are modal.

If the gloda module is a proper JS module there should not be objects created twice. We already import gloda.js multiple times.
at various places: https://searchfox.org/comm-central/search?q=gloda.js&path=

Magnus, can you check this please?

Comment 9

[alta88]

Attachment: glodaDefaultContact.patch

It should work that way, and if you think it does, that's good enough; it's an unimportant matter of style somewhat anyway.

Comment 10

[:aceman]

Comment on attachment 9047077 [details] [diff] [review]
glodaDefaultContact.patch

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

Thanks.
I assume you drop the console.log() calls when landing it.

Comment 11

[alta88]

Attachment: glodaDefaultContact.patch

remove debug.

Comment 12

[alta88]

attempting to run local xpc test doesn't seem to work, and i don't know if it should given the current state of tests.

comm/obj-dir$ ../../mach xpcshell-test mailnews/db/gloda
..
 0:00.33 ERROR no tests to run using specified combination of filters: skip_if, run_if, fail_if, pathprefix(['mailnews/db/gloda'])

perhaps the sheriff can advise, or even send to try if he deems it necessary?

Comment 13

[:aceman]

It is 'mach xpcshell-test comm/mailnews/db/gloda/' if you have c-c source under m-c/comm (as is the recommended state now).
And the gloda tests pass with the patch locally.

Comment 14

[alta88]

hmm, indeed, the root has to be m-c and neither comm or comm/obj-dir. thanks.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06eaa04c74ee
Fix gloda's unchangeable assignment of 'from me' contact and make sure changes in Account Manager are reflected immediately. r=aceman

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #8)

If the gloda module is a proper JS module there should not be objects
created twice. We already import gloda.js multiple times.

Yeah importing multiple times will just refer back to the same shared object, so no additional overhead.