"Advanced Address Book Search" lacks label for "All Address Books" in AB selector dropdown

RESOLVED FIXED in Thunderbird 39.0

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: bugzilla2007, Assigned: sshagarwal)

Tracking

Trunk
Thunderbird 39.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1136792 +++
+++ This bug was initially created as a clone of Bug #170270 +++

STR

1 Open Address Book
2 Ctrl+Shift+F to show "Advanced Address Book Search"

Actual
- AB selector dropdown ("Search in:") is empty because there's no label for "All Address Books" entry which is the default selection (and working correctly afasics)

Expected
- "All Address Books" needs a label in the selector

This is really irritating and must be fixed immediately as it requires a string.
Should NOT technically use the same string as in contacts side bar, main AB etc; only as a last resort if we're too late to get a new string.
So the label should be "All Address books", but have its own entry for advanced search in the string list
"All Address Books" (correct capitalization)
Blocks: 1136801
No longer blocks: 1136801
Assignee: nobody → syshagarwal
Posted patch Patch v1 (obsolete) — Splinter Review
I knew I had written this thing on bug 170270, its still in patch v6.5 there.
https://bugzilla.mozilla.org/attachment.cgi?id=8560318&action=diff#a/mail/base/content/ABSearchDialog.js_sec2

So, it shows up for me now.

Thanks.
Attachment #8569706 - Flags: ui-review?(richard.marti)
Attachment #8569706 - Flags: feedback?(acelists)
Comment on attachment 8569706 [details] [diff] [review]
Patch v1

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

::: mail/base/content/ABSearchDialog.js
@@ +64,5 @@
> +  // Initialize globals, see abCommon.js , InitCommonJS()
> +  abList = document.getElementById("abPopup");
> +  if (abList.getItemAtIndex(0) != kAllDirectoryRoot + "?")
> +    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> +                        kAllDirectoryRoot + "?");

So this means this bug can be fixed without adding a new string?

@@ +79,5 @@
>  function searchOnUnload()
>  {
> +  let abPopup = document.getElementById('abPopup');
> +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> +    document.getElementById('abPopup').removeItemAt(0);

Why is this needed? This code seems to be run just before closing the window. Why clean this up?
Attachment #8569706 - Flags: feedback?(acelists)
Comment on attachment 8569706 [details] [diff] [review]
Patch v1

The All ABs label is visible in the advanced search now. Not really a ui-r needed, but as you asked: ui-r+.
Attachment #8569706 - Flags: ui-review?(richard.marti) → ui-review+
Blocks: 1137147
No longer blocks: 1137147
See Also: → 1137147
(In reply to :aceman from comment #4)
> Comment on attachment 8569706 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8569706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/ABSearchDialog.js
> @@ +64,5 @@
> > +  // Initialize globals, see abCommon.js , InitCommonJS()
> > +  abList = document.getElementById("abPopup");
> > +  if (abList.getItemAtIndex(0) != kAllDirectoryRoot + "?")
> > +    abList.insertItemAt(0, gAddressBookBundle.getString("allAddressBooks"),
> > +                        kAllDirectoryRoot + "?");
> 
> So this means this bug can be fixed without adding a new string?
Ya, no string is being added here.
I had this in the previous patch on the other bug, just missed it somewhere in the iterations.

> @@ +79,5 @@
> >  function searchOnUnload()
> >  {
> > +  let abPopup = document.getElementById('abPopup');
> > +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> > +    document.getElementById('abPopup').removeItemAt(0);
> 
> Why is this needed? This code seems to be run just before closing the
> window. Why clean this up?

I think abPopup's list comes from pref (addressBookWidgets.xml) and is used at other places (not sure where) but I think its okay to just remove it since it is not just being used here.
(In reply to Suyash Agarwal (:sshagarwal) from comment #6)
> > @@ +79,5 @@
> > >  function searchOnUnload()
> > >  {
> > > +  let abPopup = document.getElementById('abPopup');
> > > +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> > > +    document.getElementById('abPopup').removeItemAt(0);
> > 
> > Why is this needed? This code seems to be run just before closing the
> > window. Why clean this up?
> 
> I think abPopup's list comes from pref (addressBookWidgets.xml) and is used
> at other places (not sure where) but I think its okay to just remove it
> since it is not just being used here.

Ok, I didn't look that deep. If there is such a relation then you maybe need the code. Maybe then also this all ABs item could be managed in addressBookWidgets.xml, in a new bug as an enhancement. Depends on how many abLists use this item.
(In reply to :aceman from comment #7)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #6)
> > > @@ +79,5 @@
> > > >  function searchOnUnload()
> > > >  {
> > > > +  let abPopup = document.getElementById('abPopup');
> > > > +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> > > > +    document.getElementById('abPopup').removeItemAt(0);
> > > 
> > > Why is this needed? This code seems to be run just before closing the
> > > window. Why clean this up?
> > 
> > I think abPopup's list comes from pref (addressBookWidgets.xml) and is used
> > at other places (not sure where) but I think its okay to just remove it
> > since it is not just being used here.
> 
> Ok, I didn't look that deep. If there is such a relation then you maybe need
> the code. Maybe then also this all ABs item could be managed in
> addressBookWidgets.xml, in a new bug as an enhancement. Depends on how many
> abLists use this item.

Ya, initially I wanted it to do there. But then at places like "edit contact", "All ABs" will be listed
and we don't want that. Hence these tweaks.
So, is something needed to be done? Or, are we good to go for review?
Flags: needinfo?(acelists)
Yes, I have tested it and it creates the label in the Find Addresses dialog.
We can continue with reviews. But please file the bug about creating the label inside the right binding in addressBookWidgets.xml and CC me there :)
Flags: needinfo?(acelists)
Comment on attachment 8569706 [details] [diff] [review]
Patch v1

(In reply to :aceman from comment #10)
> Yes, I have tested it and it creates the label in the Find Addresses dialog.
> We can continue with reviews. But please file the bug about creating the
> label inside the right binding in addressBookWidgets.xml and CC me there :)

Okay, thanks :)
Attachment #8569706 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8569706 [details] [diff] [review]
Patch v1

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

::: mail/base/content/ABSearchDialog.js
@@ +79,5 @@
>  function searchOnUnload()
>  {
> +  let abPopup = document.getElementById('abPopup');
> +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> +    document.getElementById('abPopup').removeItemAt(0);

Let's remove it if it's not needed
Attachment #8569706 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8569706 [details] [diff] [review]
Patch v1

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

::: mail/base/content/ABSearchDialog.js
@@ +62,5 @@
>                                         Components.interfaces.nsIPrefLocalizedString).data;
>  
> +  // Initialize globals, see abCommon.js , InitCommonJS()
> +  abList = document.getElementById("abPopup");
> +  if (abList.getItemAtIndex(0) != kAllDirectoryRoot + "?")

Oh, and please put a parenthesis aroun the right side
(kAllDirectoryRoot + "?")
Posted patch Patch v2Splinter Review
(In reply to Magnus Melin from comment #12)
> Comment on attachment 8569706 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8569706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/ABSearchDialog.js
> @@ +79,5 @@
> >  function searchOnUnload()
> >  {
> > +  let abPopup = document.getElementById('abPopup');
> > +  if (abPopup.getItemAtIndex(0) == kAllDirectoryRoot + "?")
> > +    document.getElementById('abPopup').removeItemAt(0);
> 
> Let's remove it if it's not needed
I request to please keep it here before I investigate it further in a followup where I'll be moving this "All ABs" addition to addrbookWidgets.xml instead of spreading it all around the tree as I think there will be side-effects of this abPopup being used at other places like "edit contact". I don't remember where exactly this same list is used. And keeping this has no harm till then.

Thanks.
Attachment #8569706 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #8574031 - Flags: ui-review+
Attachment #8574031 - Flags: review+
The dialog adds the item and then removes it. I think that is sound for this bug. We do not want to get the All ABs item randomly in some other dialog that is not prepared for it.
Maybe the code is unneded, but it should do no harm. If this is going into TB38, we should better err on the side of caution.
Okay, thanks.

I'll probably be trying to write a few tests for "All ABs" and will
try to cover a few hidden use-cases.
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Comment on attachment 8574031 [details] [diff] [review]
Patch v2

pushed https://hg.mozilla.org/comm-central/rev/65249101c9ef

We'll do comm-aurora after a nightly cycle.
Attachment #8574031 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
Attachment #8574031 - Flags: approval-comm-aurora? → approval-comm-aurora+
Blocks: 1153840
Blocks: 170270
No longer depends on: 170270
No longer blocks: 1153840
No longer depends on: 1136792
You need to log in before you can comment on or make changes to this bug.