Closed Bug 1140768 Opened 5 years ago Closed 5 years ago

Typo of "gShowAbColumnInComposeSidbar" in abContactsPanel.js

Categories

(Thunderbird :: Address Book, defect)

defect
Not set

Tracking

(thunderbird38+ fixed, thunderbird39 fixed)

RESOLVED FIXED
Thunderbird 39.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

3.25 KB, patch
aceman
: review+
Details | Diff | Splinter Review
gShowAbColumnInComposeSidbar in abContactsPanel.sj should have been gShowAbColumnInComposeSidebar ...
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #8574347 - Flags: feedback?(syshagarwal)
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?
(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.
OS: Linux → All
Hardware: x86 → All
Comment on attachment 8574347 [details] [diff] [review]
patch

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

Then its fine :)

Thanks.
Attachment #8574347 - Flags: feedback?(syshagarwal) → feedback+
Comment on attachment 8574347 [details] [diff] [review]
patch

Thanks.
Attachment #8574347 - Flags: review?(mkmelin+mozilla)
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 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.
Attachment #8574347 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v1.1Splinter Review
Thanks.
Attachment #8574347 - Attachment is obsolete: true
Attachment #8574432 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/8b2f80525b32
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
You need to log in before you can comment on or make changes to this bug.