1140768 - Typo of "gShowAbColumnInComposeSidbar" in abContactsPanel.js

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[:aceman]

gShowAbColumnInComposeSidbar in abContactsPanel.sj should have been gShowAbColumnInComposeSidebar ...

Comment 1

[:aceman]

Attachment: patch

Fixing the typo and cleaning up the logic. Suyash please check if this still does what was intended in bug 170270.

Especially the changes:
-  if (abList.selectedIndex == 0) {
+  if (abList.value.startsWith(kAllDirectoryRoot + "?")) {
(What if the 0-th item isn't the All ABs one?)

-  document.getElementById("addrbook").setAttribute("hidden", gShowAbColumnInComposeSidbar);
+  document.getElementById("addrbook").hidden = !gShowAbColumnInComposeSidebar;
(I invert the value, but it looked correct to me with the current name of the variable)

Comment 2

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8574347 [details] [diff] [review]
patch

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

::: mail/components/addrbook/content/abContactsPanel.js
@@ +65,4 @@
>    // other cases.
>    let addrbookColumn = document.getElementById("addrbook");
> +  if (abList.value.startsWith(kAllDirectoryRoot + "?")) {
> +    addrbookColumn.hidden = !gShowAbColumnInComposeSidebar;

Ya, this (!gShowAb..) is right, I made a mistake in my patch.
But abList.index(0) will always be "All ABs" now so instead of this
string compare should't we simply check for index?

Comment 3

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) away till 9th of March from comment #2)
> But abList.index(0) will always be "All ABs" now so instead of this
> string compare should't we simply check for index?
What if some extension adds some new item at index 0? :)
Yes, if it would be hard to do, testing .index==0 would be fine. But when we can easily make it more robust then why not.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8574347 [details] [diff] [review]
patch

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

Then its fine :)

Thanks.

Comment 5

[:aceman]

Comment on attachment 8574347 [details] [diff] [review]
patch

Thanks.

Comment 6

[:aceman]

This needs to go into branches that have bug 170270 as the typo is throwing exception when using the Contacts sidebar in compose window.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8574347 [details] [diff] [review]
patch

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

::: mail/components/addrbook/content/abContactsPanel.js
@@ +60,5 @@
>    else
>      ChangeDirectoryByURI(document.getElementById('addressbookList').value);
>  
>    // Hide the addressbook column if the selected addressbook isn't
> +  // "All address books". Since the the column is redundant in all

one "the" is enough.

Comment 8

[:aceman]

Attachment: patch v1.1

Thanks.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/8b2f80525b32

Comment 10

[Kent James (:rkent)]

Comment on attachment 8574432 [details] [diff] [review]
patch v1.1

[Triage Comment]

https://hg.mozilla.org/releases/comm-aurora/rev/dab4d97e3877