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

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

+++ 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.

Comment 1

[Thomas D. (:thomas8)]

So the label should be "All Address books", but have its own entry for advanced search in the string list

Comment 2

[Thomas D. (:thomas8)]

"All Address Books" (correct capitalization)

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

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.

Comment 4

[:aceman]

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?

Comment 5

[Richard Marti (:Paenglab)]

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+.

Comment 6

[Suyash Agarwal (:sshagarwal)]

(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.

Comment 7

[:aceman]

(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.

Comment 8

[Suyash Agarwal (:sshagarwal)]

(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.

Comment 9

[Suyash Agarwal (:sshagarwal)]

So, is something needed to be done? Or, are we good to go for review?

Comment 10

[:aceman]

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 :)

Comment 11

[Suyash Agarwal (:sshagarwal)]

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 :)

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[Magnus Melin [:mkmelin]]

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 + "?")

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

(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.

Comment 15

[:aceman]

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.

Comment 16

[Suyash Agarwal (:sshagarwal)]

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.

Comment 17

[Kent James (:rkent)]

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.

Comment 18

[Kent James (:rkent)]

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