Closed Bug 1136792 Opened 5 years ago Closed 5 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set

Tracking

(thunderbird38+ fixed, thunderbird39 fixed)

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

People

(Reporter: bugzilla2007, Assigned: sshagarwal)

References

()

Details

Attachments

(1 file, 2 obsolete files)

+++ 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
No longer blocks: 535715, 1133652, 1136326, 1124564
Keywords: feature
Priority: P2 → --
Whiteboard: [GS][ux-papercut]
Summary: "Advanced Address Book Search" in "All Address Books" affects main AB contacts list pane → "Advanced Address Book Search" in "All Address Books" affects/filters main AB contacts list pane
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.
Assignee: nobody → syshagarwal
Blocks: 1136798
Blocks: 1136801
No longer blocks: 1136801
Severity: enhancement → normal
Is this a regression? Or did it happen before too?
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.
Same here. Unable to reproduce it on mac.
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.
I'm on Windows 8.1 and can't reproduce with the latest Daily.
Blocks: 1137147
No longer blocks: 1137147
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.
Okay sorry, I was quick enough to respond. There is no such code! :P I'll need to investigate further.
Attached patch b1136792.patch (obsolete) — Splinter Review
Thanks to Mark Banner for clearing my view on AB Views and Directories.
Attachment #8572492 - Flags: ui-review?(richard.marti)
Attachment #8572492 - Flags: review?(mkmelin+mozilla)
Attachment #8572492 - Flags: feedback?(acelists)
Comment on attachment 8572492 [details] [diff] [review]
b1136792.patch

Now the search is only in the Advanced Search.
Attachment #8572492 - Flags: ui-review?(richard.marti) → ui-review+
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.
Attachment #8572492 - Flags: feedback?(acelists)
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?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8572492 - Attachment is obsolete: true
Attachment #8572492 - Flags: review?(mkmelin+mozilla)
Attachment #8577114 - Flags: review?(mkmelin+mozilla)
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("")
Attachment #8577114 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Attachment #8577114 - Attachment is obsolete: true
Attachment #8577668 - Flags: ui-review+
Attachment #8577668 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed: https://hg.mozilla.org/comm-central/rev/b91d8b0bd3a2

Should this be uplifted to aurora?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
(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 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
Attachment #8577668 - Flags: approval-comm-aurora?
Comment on attachment 8577668 [details] [diff] [review]
Patch for check-in

https://hg.mozilla.org/releases/comm-aurora/rev/431d3bd231a4
Attachment #8577668 - Flags: approval-comm-aurora? → approval-comm-aurora+
Blocks: 170270
No longer depends on: 170270
No longer blocks: 1136798
You need to log in before you can comment on or make changes to this bug.