Closed Bug 1591364 Opened 5 months ago Closed 5 months ago

Address book setting always jumps back to Last used directory

Categories

(Thunderbird :: Preferences, defect, P1)

defect

Tracking

(thunderbird_esr6870+ fixed, thunderbird71 fixed, thunderbird72 fixed)

VERIFIED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: riemertenbrink, Assigned: aleca)

References

(Regression)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.120 Safari/537.36

Steps to reproduce:

In Thunderbird versions 68.1.2 & 68.2 go to
Options->Composition->Addressing
at "Default startup directory in the address book windows:" select an option other than "Last used directory" e.g. "All Address Books"
close Options

Actual results:

The setting is not saved, go back to Options->Composition->Addressing
at "Default startup directory in the address book windows:" the setting jumped back to "Last used directory"

Expected results:

The selected Directory should have been saved.

Confirmed.

Alice, can you please check when that broke.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alice0775)

Thanks, Alice, record time. Our old friend, bug 1542711 which - as you may remember - also caused some LDAP chagrin.

Flags: needinfo?(alessandro)
Regressed by: 1542711

Wow, bug 1542711 is officially my personal nightmare.
I'll take a look at this and submit a patch, thanks for reporting it.

Flags: needinfo?(alessandro)
Assignee: nobody → alessandro
Priority: -- → P2

Actually, it's a P1 since this is a regression in our release version :/

Priority: P2 → P1

This issue is not present on trunk, so a fix I implemented in one of the many reported regressions fixed it.
Maybe is this one on bug 1560751, which will be available in 68b3?

Is this bug only present in beta 2?

Flags: needinfo?(jorgk)

Forget about the BETA questions, I'm just extremely confused.
The fact that the patch of bug 1560751 is the solution to our problem is still valid tho.

Right, the issue is not present on trunk and also not on TB 71 beta 1. Bug 1560751 was fixed in TB 68 beta 3 and is consequently also fixed in the succeeding ESR 68 series.

So something fixed the issue on trunk. Let's ask Alice again.

Alice, thanks again for finding the original regression, can you also locate where that got fixed on trunk. Many thanks in advance.

Flags: needinfo?(jorgk) → needinfo?(alice0775)

Tried in 71.0b1, the bug seems squished there.

OK, so by the looks of it, bug 1560751 from the first regression range made it "always blank" and then re-arranging the prefs in the second range made it work again. So when testing the compose sidebar for bug 1560751 we didn't notice that it destroyed the preference.

Now it won't be much fun to figure out what part of bug 1498332 or bug 1565789 fixed the issue.

Alex, back to you. Building TB 68 locally isn't much fun, but you can just get a debug build from the Treeherder and unpack onmi.ja and then change JS files directly. Talk to me on IRC for details.

Flags: needinfo?(alessandro)

Thank you so much Alice for the detailed overview, and thanks Jorg for referencing the necessary bugs.

I'm compiling 68 locally to hunt this down.
Wish me luck!

Flags: needinfo?(alessandro)
Status: NEW → ASSIGNED
Attached patch 1591364-address-book.patch (obsolete) — Splinter Review

I can't find what changed in 71 that caused this to work, but I think I found the issue from another point of view.
All the other menulists work as expected, only the defaultStartupDirList wasn't getting a value at all, even if the pref was saved correctly the value always returned empty.

This issue happens in the initAbDefaultStartupDir method, where we're trying to select the menulist item based on the saved pref.
For whatever reason, the item is not selected and the value gets emptied.

Since the menulist CE automatically selects the item based on the stored value, simply setting the correct value fixes the problem.

What do you think? Is this an acceptable fix?

Attachment #9105047 - Flags: review?(jorgk)
Comment on attachment 9105047 [details] [diff] [review]
1591364-address-book.patch

Thanks for the patch. I did some research myself: Compared menulist-addrbooks.js, compose.inc.xul and compose.js between trunk and ESR 68 and saw no significant difference.

I was able to confirm the result of your research. On ESR 68 `dirItem` is returned null, but on trunk, it's returned non-null. Dumping out the URI, I see jsaddrbook://abook.sqlite thanks to the new AB technology, on ESR 68 I see something like moz-abmdbdirectory://abook.mab, the pref has moz-abmdbdirectory://abook.mab. Next I dumped out the values of the children of the menupopup and saw:
moz-abdirectory://?
moz-abmdbdirectory://abook.mab
moz-abmdbdirectory://abook.mab/MailList1
moz-abmdbdirectory://abook-6.mab
and more. ? being "all address books". Clearly the value we were looking for was there.

So I came to the conclusion that the value selector just doesn't work in TB 68 :-(. Let me suggest a different patch.
Comment on attachment 9105047 [details] [diff] [review]
1591364-address-book.patch

Based on the comment, you can't just assign the value:
// It may happen that the stored URI is not in the list. <==== Read here ;-)
// In that case select the "none" value and let the AB code clear out
// the invalid value, unless the user selects something here.
Attachment #9105047 - Flags: review?(jorgk)

But that comment is not relevant anymore since the de-xbl of the menulist.

If the stored URI is not in the list, this code in the menulist-addrbook.js takes care of it:

// Attempt to select the persisted or otherwise first directory.
      this.selectedIndex = this._directories.findIndex((d) => {
        return d && d[type] == this.value;
      });

      if (!this.selectedItem && this.menupopup.hasChildNodes()) {
        this.selectedIndex = 0;
      }
Attached patch 1591364-TB68-only.patch (obsolete) — Splinter Review

Looks like we need to hand-roll the faulty selector :-(

Attachment #9105047 - Attachment is obsolete: true
Attachment #9105059 - Flags: review?(mkmelin+mozilla)
Attachment #9105059 - Flags: review?(alessandro)
Comment on attachment 9105059 [details] [diff] [review]
1591364-TB68-only.patch

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

I'm gonna let Magnus decide on this.
I personally think this is a bit of an overkill, and also unnecessary since the CE is already dealing with the possibility of having a URI not present in the list.
Simply setting the value with whatever string is stored in the prefs seems more straightforward and less cumbersome.
Attachment #9105059 - Flags: review?(alessandro)

If the stored URI is not in the list, this code in the menulist-addrbook.js takes care of it:

Sorry, I didn't see this in time. But what happens then:
dirList.value = startupURI;

So that will then select the first entry? Which is all address books? But the intent was to select "last used" which doesn't have a URL.

I don't care, this way or the other.

Comment on attachment 9105047 [details] [diff] [review]
1591364-address-book.patch

Magnus, according to our research, the query selector doesn't work. So we have two different solutions.

Alex' solution is also fit for trunk use.
Attachment #9105047 - Attachment is obsolete: false
Attachment #9105047 - Flags: review?(mkmelin+mozilla)

(In reply to Jorg K (GMT+2) from comment #19)

Sorry, I didn't see this in time. But what happens then:
dirList.value = startupURI;

So that will then select the first entry? Which is all address books? But the intent was to select "last used" which doesn't have a URL.

It selects index 0, which for me is always "Last used directory"...isn't it?

Comment on attachment 9105047 [details] [diff] [review]
1591364-address-book.patch

How long since you last pulled your c-esr68? This is pre-prettier, so not older than the beginning of September. As discussed on IRC, we could use your approach on trunk as well. Let me upload a rebased patch.
Attachment #9105047 - Attachment is obsolete: true
Attachment #9105047 - Flags: review?(mkmelin+mozilla)

This is Alex patch for trunk. WFM, so I have no problem using this on all branches. Magnus?

Attachment #9105165 - Flags: review?(mkmelin+mozilla)
Attachment #9105059 - Attachment is obsolete: true
Attachment #9105059 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9105165 [details] [diff] [review]
1591364-address-book.patch

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

This one looks better to me.

I'd argue though that the "startup pref" is quite unnecessary and could be removed. We can just persist what's last used, which is a much more user friendly approach (findable and easy to change). For another bug though.
That menu is also broken on trunk in that it lists all mail lists too, which I don't think is intended.
Attachment #9105165 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/013567062ce3
Fix address book default startup directory in preferences panel. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Comment on attachment 9105165 [details] [diff] [review]
1591364-address-book.patch

I'll rebase as necessary.
Attachment #9105165 - Flags: approval-comm-esr68+
Attachment #9105165 - Flags: approval-comm-beta+

Working in my TB 68.2.1 build.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.