1136792 - "Advanced Address Book Search" in "All Address Books" affects/filters main AB contacts list pane

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

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

STR

1 Open Address Book
2 Select "All Address Books" node (noting that all local contacts are shown, but LDAP contacts are correctly and explicitly not shown)
3 Ctrl+Shift+F to show "Advanced Address Book Search", window size (not maximized)
4 Display Name contains: type "a", press Enter to perform search (ensure some matches)
5 close Advanced AB search, and look at main AB list view

Actual
- search is applied to the *main* AB window, too (obviously shouldn't), which is suddenly showing LDAP results matching "a", plus only those local contacts matching "a", BUT there's no search term in main AB, so it's not easy to notice filter and get rid of it
-> confusing, error-prone, wrong behaviour

Expected

Advanced search must only be executed within its own window

Comment 1

[Thomas D. (:thomas8)]

The search inside advanced AB search is executed correctly, but it shouldn't be simultaneously executed in the main AB list. Probably using wrong/non-unique IDs, URLs, or other such references.

Comment 2

[Suyash Agarwal (:sshagarwal)]

Is this a regression? Or did it happen before too?

Comment 3

[:aceman]

I can't reproduce this. At least on Linux, there is no Ctrl+Shift+F. In the menu it is shown as Shift+F but it doesn't work. Anyway, clicking the menuitem for "Search addresses" opens advanced search. I type the "a". I can see bug 1136798, but not this one. No sync with the main AB window.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Same here. Unable to reproduce it on mac.

Comment 5

[Thomas D. (:thomas8)]

I'm on windows 8, and it's always reproducable.
Only when "All Address Books" is the target AB of Advanced AB Search Dialogue.
For single AB targets, everything is normal (as in TB 24), no wrong syncing of views.
It's a regression, well, because it only happens on the new feature which we introduced.

It might be important to note that the error occurs already at step 4, immediately when you press Enter to confirm advanced search, the main AB view in the background of search window changes the result set.
You may not notice after step 5 if you don't look very closely which results are actually shown, and which are missing.

Let me know if you need screenshots.

Comment 6

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

I'm on Windows 8.1 and can't reproduce with the latest Daily.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Okay, this bug exists.
Try with these STR:

Make sure you have ldap as one of the ABs in your addressbook and All ABs is selected in your main AB window.
Make a search for "@" in Advanced AB, and you'll see the bug.
Actually, its by design of SetAbView() and to remove our "only local searches show up and not ldap" I did a refresh of the URI, so the bug.

I'll figure something out and then upload the patch.

Thanks.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Okay sorry, I was quick enough to respond. There is no such code! :P I'll need to investigate further.

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: b1136792.patch

Thanks to Mark Banner for clearing my view on AB Views and Directories.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8572492 [details] [diff] [review]
b1136792.patch

Now the search is only in the Advanced Search.

Comment 11

[:aceman]

Comment on attachment 8572492 [details] [diff] [review]
b1136792.patch

I still can't reproduce the bug, even with a working LDAP server in the list of ABs. So I can't comment on the patch.

If searching in LDAP or All ABs I get frequent crashes. I also noticed the Addressbook column in Advanced search is empty for all cards. In main AB window it works fine.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8572492 [details] [diff] [review]
b1136792.patch

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

Could you explain what isDirectoryQuery and isMDirectoryQuery do/correspond to?

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

I have added the necessary comments in the code. But for quick reference, mentioning it here as well.

isDirectoryQuery is a boolean variable that will be true if the parent directory to which the card is being added (aParentDir argument in the method) is a query directory, false otherwise.
isMDirectoryQuery will be true if mDirectory is a query directory.
Now, deciding which directory is mDirectory is not trivial. If the selected directory in main AB window is not "All ABs", the selected directory will be mDirectory, else, if the selection is "All ABs", the last AB directory in the list will be mDirectory.

directoryQuery and mDirectoryQuery are the latest additions to this patch. They are the query strings for directory (Advanced AB Search window) and mDirectory (main AB window) respectively.

This was needed because if main AB window also has a search term and the Advanced AB window is also triggering a search, simply checking for both to be query directories will not be sufficient.

Thanks.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8577114 [details] [diff] [review]
Patch v2

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

Seems to work, r=mkmelin

::: mailnews/addrbook/src/nsAbView.cpp
@@ +835,5 @@
> +  nsCString uri;
> +  aDir->GetURI(uri);
> +  int32_t searchBegin = uri.FindChar('?');
> +  if (searchBegin == kNotFound)
> +    return nsCString("");

this should probably be EmptyCString() or  NS_LITERAL_CSTRING("")

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Thanks.

Comment 16

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

Pushed: https://hg.mozilla.org/comm-central/rev/b91d8b0bd3a2

Should this be uplifted to aurora?

Comment 17

[Suyash Agarwal (:sshagarwal)]

(In reply to Archaeopteryx [:aryx] from comment #16)
> Pushed: https://hg.mozilla.org/comm-central/rev/b91d8b0bd3a2
> 
> Should this be uplifted to aurora?

Ya! This definitely needs to be in.

Thanks.

Comment 18

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

Comment on attachment 8577668 [details] [diff] [review]
Patch for check-in

[Approval Request Comment]
Regression caused by (bug #): bug 170270
User impact if declined: Advanced address book search also affects main address book window
Testing completed (on c-c, etc.): not yet in Daily
Risk to taking this patch (and alternatives if risky): low

Comment 19

[Kent James (:rkent)]

Comment on attachment 8577668 [details] [diff] [review]
Patch for check-in

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