170270 - Enable search in multiple/all address books

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement, P2)

Description

[Sean Gao]

The fuction to search in multiple address books or even the whole address books
space is fairly helpful for users. Sometimes if we don't know which address book
we have put a guy's address in, it is borning to search the address books one by
one.

Comment 1

[Bogdan Stroe]

*** Bug 246478 has been marked as a duplicate of this bug. ***

Comment 2

[Wayne Mery (:wsmwk)]

similar TB Bug 250513

Comment 3

[Mark Banner (:standard8)]

*** Bug 222377 has been marked as a duplicate of this bug. ***

Comment 4

[Mark Banner (:standard8)]

*** Bug 250513 has been marked as a duplicate of this bug. ***

Comment 5

[Wayne Mery (:wsmwk)]

like mail search, the results should display each search hit's container, i.e.
the name of the address book of each hit.

Comment 7

[Frank]

What is interesting about this issue is if you rely on the auto-address lookup, when entering your To addresses, TB will search across all books. So why not include the same functionalty in search? 

Actually, auto-address lookup might almost work for email address searching BUT, a) this process does not include the domain portion of the email when searching, and b) it does not show "all" matches while the search is being resolved, and c) should find what you are looking for there is no way to get to the contact record, should you be looking for it to modify it. It would be helpful if something like the Contact Side bar (which I have installed) showed the location of the contact once found - maybe this would be triggered by clicking on the icon next to the email address in the To field of the email being composed.

Comment 8

[Tara Athan]

In addition to searching all address books, the search should apply to "other" fields. Because Thunderbird doesn't really share contacts between address books (that is you can duplicate a contact to another address book, but you have to update them separately), I prefer to have a few address books and then put tags into a field which is searchable.  That way I can put in multiple tags for one card and it will get pulled up for a search according to each tag.  As it is, I cannot use the Custom fields for this, although this would be the logical choice, because these fields are not searchable. 

Comment 9

[Markus Klaschka]

Hmm sounds not really cool.
This "search all books"-Problem makes me crazy every day in the office.

The problem is, that there is nearly nothing documented and nearly everything is written in Javascript.

So, if there is someone who can help me, and kick me in the right direction, I'll try to fix this bug.
I'm sure I'll need more than twice the time than a involved mozilla-developer, but I waited more than half a year, now we are a few version further and nothing happend...

I think the main problem is, that this bug isn't seen as needed to fix, but searching all books at the same time is very necessary for company's which plan to use TB. I'm sure that's the biggest thing why many company's still use Outlook after trying to migrate to TB.

Greetings and regards

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to comment #8)
> In addition to searching all address books, the search should apply to "other"
> fields. 

"searching all fields" exists in a different bug.  Tara, can you do a search and post the bug number here?


(In reply to comment #9)
>...
> I think the main problem is, that this bug isn't seen as needed to fix, 

Actually it's more a question of manpower - good thing you've come along :)
Mark may be able give you some tips to get started.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #9)
> So, if there is someone who can help me, and kick me in the right direction,
> I'll try to fix this bug.

The search code is mainly based under:

http://mxr.mozilla.org/seamonkey/source/mailnews/base/search/

There are some Thunderbird chrome files under:

http://mxr.mozilla.org/seamonkey/source/mail/base/content/

Basically, I expect the main work will be to alter ABSearchDialog.*. I haven't looked too much into these yet but I'm pretty sure they are the ones that control it all.

You should be able to do this fine for local address books (Personal etc, outlook), but I think you may struggle with LDAP address books due to bug 135231. So if you look at just local address books in the first instance, then we'll see how we can fit LDAP address books in later.

Comment 12

[Markus Klaschka]

I'm on it.
The structure is a bit confusing, but I think there's no documentation ( ? )

Cause the code of the AbSearch is splittet into diferent files, components... I have to spend a bit more time till I find the best way to add this feature.

I don't think that we have to have an eye on bug135231 for this, if we only search book by book and put the result into the ResultDialog, but in:
mailnews/base/search/resources/content/ABSearchDialog.js

...this here is used to "output" the results:
315     SetAbView(searchUri, null, null);

...so I have to check what SetAbView() is doing.

Cause of this here I think it could take a time to fix this bug
197     var searchUri = currentAbURI + "?(";

...I don't think it's possible to just do a:
searchUri += ";" + currentAbURI + "?(";

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> The structure is a bit confusing, but I think there's no documentation ( ? )

There's very little unfortunately.

> I don't think that we have to have an eye on bug135231 for this, if we only
> search book by book

We will because you can't get the results synchronously like you can for local address books, but that's not the main concern yet.

> mailnews/base/search/resources/content/ABSearchDialog.js
> ...this here is used to "output" the results:
> 315     SetAbView(searchUri, null, null);
> ...so I have to check what SetAbView() is doing.

The results window/table is a tree that is controlled by the nsAbView class. The main function there to look at is Init:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/src/nsAbView.cpp&rev=1.65&mark=190-261#190

> Cause of this here I think it could take a time to fix this bug
> 197     var searchUri = currentAbURI + "?(";
> ...I don't think it's possible to just do a:
> searchUri += ";" + currentAbURI + "?(";

I think what you need to do for the all address book case is to make this searchUri = "moz-abdirectory://?("

URIs for address books normally take the form of "moz-ab***directory://..." where *** is the type and ... depends on the particular address book.

So in nsAbView::Init we'll need to detect the moz-abdirectory special case and get all the address books below that (get the directory in the same was as init does currently, then call GetChildNodes on it - that gives an enumerator with all the child directories), then we can use the enumeration of the directories to go through all the directories calling the appropriate functions.

Comment 17

[[e-mail]]

This is a serious bug. If you have 16 address books with 15 people in each address book and you are looking for one person amongst those 16 address books, you will need to click on each address book and do the search up to 16 times.
There must be a way to do a global all my address books search.

Comment 22

[Kalenz]

Please see also Bug 607320.

Comment 25

[ugoveri]

It's incredible how this crippled functionality still persists since ever... We are already heading for version 5.0 and nothing. There is been made a tremendous effort to make thunderbird more appealing for the masses, with a simpler yet more powerful interface, but the addressbook handling is for sure one of the main weak points! A lot of friends and companies don't use it because of this.

Comment 26

[[e-mail]]

Unable to search multiple "address book" at the same time

Diatribe follows:

What is the point of having multiple address books if you can't search through them at the same time?

This leads to having one massive "unorganized / flat" address book, just so you can do one search.

This is like having 7 different googles. If you want to search NA, then you need to do that search, then a Europe search, then a Asia search... on and on and on...

Reproducible: Always

Steps to Reproduce:
1.>Tools >Address Book
2.>Edit > Search Addresses
3. Oh! Darn! I can't search "ALL MY ADDRESS BOOKS". Gee, I'll just search through all 18 of them manually. Lucky I don't have 50 address books like I do at work "with any other email program"! That would take a LONG LONG time.

Actual Results:  
frustration.

Expected Results:  
a simple result.

1: Be able to select multiple address books and search through those
or
2: "search all address books"

Comment 27

[sravot]

All has been said!

We are an association with 40 adress books...

Comment 28

[Brian Edge]

I think that as this request has been active since Sept 2002, we can maybe assume it is never going to happen.

What would probably be a better suggestion is for Mozilla Thunderbird to pubish an interface spec. for the Address Book and then change it to being an "optional Add-on".

This would allow many software guru's to write their own Address Book application or provide an interface to some of the ready written Address Book apps.

These could then be made available as Add-On's, Freeware or Shareware apps, etc, and include more and better features than the current Thunderbird Address book offers at present.

Food for thought?

Comment 29

[David :Bienvenu]

This could be done as a simple extension today, I believe.

Comment 30

[[e-mail]]

Please take this seriously.

If this is left to be done as an extension, then as thunderbird goes through future versions, IF the extension is not kept up to date, even if it's a month or two of non-functioning search, then the "search multiple address books" thing will become broken, and, in the end make for very frustrated users.

To me, searching multiple address books is a fundamental part of an email program, and should be built into thunderbird.

There are small companies, plus around 8 individual clients who I have told to not use thunderbird, only because of this missing function.

For me personally, now that I have looked through my address books in thunderbird, they are a complete mess, multiple people in a sordid set of groupings, and I can honestly say for myself, if I could do searches across multiple address books, that it would be much more organized.

My suspicion is that many people don't expand their use of the address books because it completely collapses around the fact that you can't search through them.

I would suggest that we be able to search ALL address books by default, and then someone can write a add-on to focus how you do searches by creating custom searches across your selection of books.

Also, if you are typing an email address into the recipient, it instantly finds the email address from all address books.

Is there a reason why you can't search multiple address books? Security?

Comment 31

[David :Bienvenu]

No one has written a patch to do so. I would welcome such a patch. If you're volunteering, that's great. If not, perhaps you can find a volunteer to do it.

Comment 32

[Thomas D. (:thomas8)]

changed URL according to our conventions (tag-based, not topic-based)
FTR: canonical gs topic is http://gsfn.us/t/19cqn

OT: Major Bugs like this one should be in a central and public queue where they will be eliminated by TB developers one by one based on priority, so users will have more clarity about the time frames, instead of being postponed to indefinite, which is almost an insult in view of such major shortcomings.

Comment 33

[Kalenz]

Because the two bugs are on similar topics, can I once again draw your attention to bug 607320. I proposed a solution to that bug, that I think might also be helpful here, and I hope can be implemented without too much new code needing written...

Comment 34

[[e-mail]]

Are they similar?
The first line in the description of bug 607320 (link above) says "looking for an address, that I know is not in my address book,".

Meanwhile this bug is about searching across multiple address books, not just one, and has existed for 9 years.

If Mozilla wants to relook at search for Thunderbird in general, then I suppose a big list of all the types of search required/wanted/optional/customizable could be created.

The bug of not being able to search across multiple address books while in the address book is what is being brought to the attention of the programmers or program managers here.

Comment 35

[Kalenz]

The bottomline with both bugs is that search algorithms for finding email addresses in the Compose and Address Book interfaces are hopelessly outdated.

Gloda search for addresses already exists (see my last post under bug 607320), from the main window, however it only works to retrieve emails associated with a certain address (sent by/to/cc etc.), it does not work to send a new email to those addresses, or to manage them within an address book. However, the search algorithm does exist, and works beautifully! It should then be reasonably easy, one would think, to access that same search algorithm from the Compose window (bug 607320) and from the address book (this bug), to retrieve any email address.

Comment 36

[supporto]

Global search for all the address books is a must have functionality, certainly not a feature.
Had same experience of TB not being used because of this.
Is anything we could do to make this to be taken into account by maintainers? First comment here is from 2002, the bug is still marked as "NEW" with anyone assigned to it :(

Comment 37

[[e-mail]]

Unfortunately, Thunderbird 9 does not allow "search multiple address book" yet.

Comment 38

[[e-mail]]

Unfortunately, Thunderbird 10 does not allow "search multiple address books" yet.
Coming up on 10 years for a bug! Very impressive.

May I suggest that fixing this bug will probably get the largest amount of new users out of any other bug fix. You will also be able to put, "Able to search multiple address books" as a new feature. I am sure that MANY people have stopped using and recommending Thunderbird because of this bug over the past ten years.

And no, I am not a programmer.

Comment 39

[[e-mail]]

Unfortunately, Thunderbird 15 does not allow "search multiple address books" yet.
Coming up on 10 years for a bug! Very impressive.

Comment 40

[Thomas D. (:thomas8)]

Aceman, or anybody with code knowledge in this corner:

Composition's autocomplete already *does* search all ABs (only with a poor search algorithm). I just saw the following simple line in the contexts of Aceman's patch for bug 529584, attachment 763273 [details] [diff] [review], which creates the search for *all* ABs, and I was wondering if perhaps we can copy that little trick for Contact's sidebar at least (don't know if it's harder for AB quicksearch):

http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#420

420       // Now do the searching
421       var allABs = this._abManager.directories;

so then, allABs are passed as search target with the query.

Perhaps fixing this bug could be as easy as adding a single menuitem "All ABs" to the contacts sidebar AB-selector dropdown, and then trigger the existing query against allABs aka this._abManager.directories instead of just against the selected AB?

Comment 41

[:aceman]

Good idea.

Comment 42

[Touchard]

Hello !

I think the best would be that there is only one address book and there is possibility to add (and change) labels to files.
Of course, we can list files with one or any label. We can export files with any label. And imported files wear a special label.
Please, excuse my bad english, I am french.

Comment 43

[Jon Jenkins]

There is something missing here: not only does the auto complete search all address books but so does the Edit/Find/Search Messages when matching for "in" or "not in" address books. Surely this cannot be this hard to do even if it is a a bit of a hack!

Comment 44

[[e-mail]]

Is it possible to "rewrite from scratch" the addressbook and then do a one time transfer of data from the old format to the new format?

I don't think it's acceptable to have the addressbook as a "plugin" because then you will have many different plugins storing data in different ways and you never know which plugin will last more than a year or two. This should be in core.

This bug is now documented at 11 years 4 months...

What does:
Priority: -- → P2
Whiteboard: [gs] → [gs][patchlove]
mean?

Comment 45

[Thomas D. (:thomas8)]

(In reply to [e-mail] from comment #44)

tb/shootedit, thank you for your continued interest.

> Is it possible to "rewrite from scratch" the addressbook and then do a one
> time transfer of data from the old format to the new format?

Mozilla's Mike Conley made an effort to "rewrite AB from scratch", around here:
http://mikeconley.ca/blog/tag/ensemble/
He is currently looking for someone to finish that effort.
So practically there are no timelines on this, and therefore imho it's better to fix the worst bugs in the current, old AB (although I acknowledge that current AB is ultimately beyond repair because the design is wrong).

> I don't think it's acceptable to have the addressbook as a "plugin" because
> then you will have many different plugins storing data in different ways and
> you never know which plugin will last more than a year or two. This should
> be in core.

I think that's technical implementation details which we should leave to the experts. I think the idea is that TB has a plugabble interface that can connect with a variety of AB types when they use that interface. TB would have it's own "plugin" AB which will be maintained as part of main TB product, so for all practical purposes, it's part of core.

> This bug is now documented at 11 years 4 months...

That's more than unfortunate, but it's a general problem seen in TB bugs.
Pls be aware that Mozilla has mostly defunded TB, so TB is now maintained almost exclusively by volunteers, or former mozilla messaging employees in their free time.
If you have coding skills, most welcome!
 
> What does:
> Priority: -- → P2
> Whiteboard: [gs] → [gs][patchlove]
> mean?

P2 means Priority2. Given the manpower situation and lack of leadership, it's practically meaningless, but I've set it anyway trying to indicate that this deserves higher priority.

[gs] means that users have reported this problem / suggested this enhancement on TB's support platform, http://getsatisfaction.com/mozilla_messaging/
We need help of users like you to tag getsatisfaction reports with respective bug numbers ("bug 170270" for this bug) so that interested parties can have an impression how popular this is on getsatisfaction, using the link in the URL field of this bug:
http://getsatisfaction.com/mozilla_messaging/tags/bug_170270
If there are hundreds of GS reports, it *might* lead to faster fix of this bug.
However, we have hundreds of bugs that are of same or higher importance, and without sufficient manpower who have time, will, skill to fix it, it's kinda hard to get things done.

[patchlove] is a label that should probably be set by experienced contributors only as a way of indicating "we would love to have a patch for this", iow, it's another flag trying to make this bug more relevant between thousands of other bugs. I'm not sure if somebody actually follows on these, or how relevant that label is in the big scene of things, these labels come and go. And unfortunately, the more often we use them, the more meaningless they become. Back to square one, manpower problem. We need volunteer contributors to fix bugs after they have been tagged...

I myself am a long-term volunteer contributor in TB QA/bug triage/UX.

Comment 46

[Magnus Melin [:mkmelin]]

no, [patchlove] is used to indicate the bug has some kind of abandoned (wip) patch waiting for someone to finish it

Comment 47

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #46)
> no, [patchlove] is used to indicate the bug has some kind of abandoned (wip)
> patch waiting for someone to finish it

Oh, I see, thanks for the update. So the official translation of [patchlove] is "give love to patches which have not yet checked in". Which looks like a smaller subset of "we would love to have a patch for this" ;)

https://wiki.mozilla.org/Thunderbird:PatchLove

Comment 48

[Thomas D. (:thomas8)]

Aceman, could you perhaps give this a try, as you said in comment 41 that the simple patch outlined in my comment 40 looks like a "good idea"?

With 13 duplicates and 38 votes, this might be an excellent papercut bug (pls confirm in Whiteboard if you agree; and no, I'm not much willing to edit that old papercut table...).

Comment 49

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #48)
> Aceman, could you perhaps give this a try, as you said in comment 41 that
> the simple patch outlined in my comment 40 looks like a "good idea"?
> 
> With 13 duplicates and 38 votes, this might be an excellent papercut bug
> (pls confirm in Whiteboard if you agree; and no, I'm not much willing to
> edit that old papercut table...).

Comment 51

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch draft v1

Okay, so far, this works for the addressbook.
This makes searching in *all* addressbooks possible.
Searching in *multiple* addressbooks requires completely different
search interface, possibly one like for mails. But I think we
can do with this as we are going to replace this addressbook in the
near future anyway.

In CSS I am just using a placeholder image that'll
be removed eventually.

If anyone would like to provide a feedback, that'll be great.

Thanks.

Comment 52

[Thomas D. (:thomas8)]

Suyash, thanks for this gorgeous Christmas present, and I hope yours was good! :)

Can you attach a screenshot of main AB (and contacts side bar if applicable) with your patch?

Did you play around with cross-AB search results a bit?
- search so that you get matches from several ABs
- drag single/multiple selected cross-AB matches into composition
- context menu actions on single/multiple selected cross-AB matches
- other actions like toolbar Delete on single/multiple selected cross-AB matches
For all of these, do they work correctly?

Comment 53

[Suyash Agarwal (:sshagarwal)]

Attachment: screenshot.png

(In reply to Thomas D. from comment #52)
> Can you attach a screenshot of main AB (and contacts side bar if applicable)
> with your patch?
Please find the attached image.
I couldn't figure out how to show what is to be shown, so here's an attempt:
On the left is the selection of the first addressbook, on the right is searchquery result for "invalid". Though the selected ab is personalAB, that's an error and it will be unselected.

> Did you play around with cross-AB search results a bit?
> [checked] search so that you get matches from several ABs
> [checked] drag single/multiple selected cross-AB matches into composition
> [checked] context menu actions on single/multiple selected cross-AB matches
> [checked] other actions like toolbar Delete on single/multiple selected cross-AB
> matches
> For all of these, do they work correctly?
Yes, they do (for addressbook only). I haven't addressed sidebar so far.

Thanks.

Comment 54

[Thomas D. (:thomas8)]

Suyash, thx 4 screenshot & rapid reply.

Feedback:
1) This bug is important enough that imho almost any way of fixing the problem in current(old) AB would be acceptable, as long as we deliver this for TB38 and it actually works in a reasonable understandable way.

2) Having said which, I was surprised by the push-button (next to searchfield) implementation proposed by attachment 8542400 [details] (which has some disadvantages), and I think we could provide a better, more intuitive and more efficient UI/UX:
- Can you try to just add "All address books" as another entry at the top of the AB selector pane on the left? So we would have:
* All Address Books
* Personal Address Book
* ...
* Collected Addresses
I think that would be the most intuitive solution which blends in nicely into existing design (might need some tweaking of context menu etc.).

3) If 2) is too much work, we could try this as a workaround:
- Add mini-toolbar on top of AB selector pane, with "Search All Address Books" button type=checkbox

4) I suspect that it's much easier to fix this in Contacts Side Bar first (which would suffice for a large number of scenarios):
- Just add another list entry, "All Address Books", to the dropdownlist titled "Address Books:"
- Make "All Address Books" list entry point to the all-ab-URL, in analogy to other list entries

Comment 55

[Thomas D. (:thomas8)]

Also, whereever we claim "All ABs", we need to check how that relates to LDAP ABs which are probably excluded, and how/if we can indicate that. Or include them? :) Sorry I'm not into LDAP...

Comment 56

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.7

Okay so this patch fixes the issues for both viz. addressbook search
and contacts side bar search.

Thanks.

Comment 57

[Suyash Agarwal (:sshagarwal)]

Attachment: composition-screenshot.png

Comment 58

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #54)
> Feedback:
> 2) Having said which, I was surprised by the push-button (next to
> searchfield) implementation proposed by attachment 8542400 [details] (which
> has some disadvantages), and I think we could provide a better, more
> intuitive and more efficient UI/UX:
> - Can you try to just add "All address books" as another entry at the top of
> the AB selector pane on the left? So we would have:
> * All Address Books
> * Personal Address Book
> * ...
> * Collected Addresses
> I think that would be the most intuitive solution which blends in nicely
> into existing design (might need some tweaking of context menu etc.).
Ya, this has varied issues.
a) If we provide it as an entry, first expectation would be that whenever it
is selected, all contacts from all addressbooks should be listed.
b) Deletion of this AB should be allowed and should result in deletion of
all addressbooks.
c) Properties: I don't know how that's gonna work.
d) Other features available (I am not aware of) will need to be handled as well.
So, this can be done, to an extent. But if searching through all ABs is the only
thing that's needed before we have another AB interface in place, I think we should
leave this option :)
Also, we have this "All ABs" option in contacts side panel in composition window, so
I think that will suffice (?).
> 3) If 2) is too much work, we could try this as a workaround:
> - Add mini-toolbar on top of AB selector pane, with "Search All Address
> Books" button type=checkbox
Ya, this can be done.
> 4) I suspect that it's much easier to fix this in Contacts Side Bar first
> (which would suffice for a large number of scenarios):
> - Just add another list entry, "All Address Books", to the dropdownlist
> titled "Address Books:"
> - Make "All Address Books" list entry point to the all-ab-URL, in analogy to
> other list entries
Done sir.

(In reply to Thomas D. from comment #55)
> Also, whereever we claim "All ABs", we need to check how that relates to
> LDAP ABs which are probably excluded, and how/if we can indicate that. Or
> include them? :) Sorry I'm not into LDAP...
We need input from other people on how to handle this.
Currently, it iterates over all the ABs and ignores ones that aren't available.

Please find the attached screenshot for the composition window.

Thanks.

Comment 59

[Suyash Agarwal (:sshagarwal)]

Oh and all the actions that were possible on the search results of single addressbook
in the contacts side bar in the composition window (drag&drop, properties, deletion etc)
are possible with "All Address Books" too.

Please mark the proper reviewers. I didn't want to disturb anyone on the New Year :P

Thanks.

Comment 60

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #58)

First of all - THANKS - having this for contacts side bar rocks! :)

> (In reply to Thomas D. from comment #54)
> > Feedback:
> > 2) Having said which, I was surprised by the push-button (next to
> > searchfield) implementation proposed by attachment 8542400 [details] (which
> > has some disadvantages)

The push-button "Search All ABs" approach in main AB is better than status quo, but isn't good in terms of UX. Plus, I have informed doubts whether it'll actually save you much work, compared over solution 2) where we create another virtual "AB" called "All Address Books" in the main list of ABs.

a) ux-efficiency: users who want all-ab-search always, are forced to explicitly click/press the new button for each search, instead of just convenient Enter
b) ux-natural-mapping: it just feels wrong to push-click "all abs button" every time *on the right* whilst any other AB is simply and lastingly click-selected *on the left*.
c) you will NOT avoid any of the problems associated with virtual all-abs-AB by making it a push-button search (as I will show below).
d) context menu of ABs is very small, and allowed actions on ABs are few, so that's not too hard ;)

> > , and I think we could provide a better, more
> > intuitive and more efficient UI/UX:
> > - Can you try to just add "All address books" as another entry at the top of
> > the AB selector pane on the left? So we would have:
> > * All Address Books
> > * Personal Address Book
> > * ...
> > * Collected Addresses
> > I think that would be the most intuitive solution which blends in nicely
> > into existing design (might need some tweaking of context menu etc.).
> Ya, this has varied issues.
> a) If we provide it as an entry, first expectation would be that whenever it
> is selected, all contacts from all addressbooks should be listed.

Yes, that's a very cool feature (find duplicates etc.) ;)
Even if you don't allow empty searches, you are basically providing the same thing when user just searches for single character. All contacts from all ABs can be considered as a find-all-search. So this doesn't create extra problems afasics.

> b) Deletion of this AB should be allowed and should result in deletion of
> all addressbooks.

No, just disable/remove deletion for "All ABs" element. It's not safe, and very unlikely scenario.

> c) Properties: I don't know how that's gonna work.

AB Properties only shows the name of AB, use "Collected Addresses" as an example how to disable renaming (which I think is appropriate here). If it's too hard, disable/remove Properties.

> d) Other features available (I am not aware of) will need to be handled as
> well.

You need to handle them anyway even with your push-button approach.
Relevant AB commands (from context and main menu) are enabled even when users select *contacts* from all-ABs search results. So far, these commands could just use the parent dir of the selected cards, which were necessarily in the same parent AB. But now, for multiple selected contacts from cross-AB search results (from push-button), there will be multiple parent ABs, so AB commands have to be tested/adjusted/disabled accordingly.

> So, this can be done, to an extent. But if searching through all ABs is the
> only thing that's needed before we have another AB interface in place, I think we
> should leave this option :)
> Also, we have this "All ABs" option in contacts side panel in composition
> window, so I think that will suffice (?).

Contacts side bar is great, but let's give it a shot for the main AB, too.
Nobody knows when the new AB will really come.
Disabling menus should be the lesser problem after you have solved the main issue, accessing all ABs.

> > 3) If 2) is too much work, we could try this as a workaround:
> > - Add mini-toolbar on top of AB selector pane, with "Search All Address
> > Books" button type=checkbox
> Ya, this can be done.

Per above explanation, even the checkbox button approach will effectively not save us from doing the work wrt AB commands.

> > 4) I suspect that it's much easier to fix this in Contacts Side Bar first
> > (which would suffice for a large number of scenarios):
> > - Just add another list entry, "All Address Books", to the dropdownlist
> > titled "Address Books:"
> > - Make "All Address Books" list entry point to the all-ab-URL, in analogy to
> > other list entries
> Done sir.
> 
> (In reply to Thomas D. from comment #55)
> > Also, whereever we claim "All ABs", we need to check how that relates to
> > LDAP ABs which are probably excluded, and how/if we can indicate that. Or
> > include them? :) Sorry I'm not into LDAP...
> We need input from other people on how to handle this.
> Currently, it iterates over all the ABs and ignores ones that aren't
> available.

We must find out a bit more how LDAP integrates into our UIs here.
E.g. in the contacts side bar, it might require a tooltip on the "All Address Books" dropdown entry: "All local address books" (because a few more centimeters to the right, autocomplete DOES search LDAP).

> Please find the attached screenshot for the composition window.

Perfect!

Further points:
- For contact card properties, now need a display of parent AB
- For contacts list pane, need column "Address Book" (to sort by containing AB)
- For bonus points, search result cards (and cards in All ABs listing) should have "Open containing Address Book" contextual command

Perhaps we should split up the patch and land the Contact Side Bar fix first, so that this can land quickly and be tested in iterations, while we continue on main AB implementation.

Comment 61

[Richard Marti (:Paenglab)]

First, also many thanks from me to take this bug. :)

I'm also for the "All Address Books" in the list which should be protected from deletion and no special button. Then we have also some consistency between AB and contact sidebar.

For a followup bug: a new column could be added which shows the AB where the address is stored. This could also help to find the duplicates and then to delete the correct one. This could also help on copy functions to not copy to the same AB.

Comment 62

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch 2.2 variant 1

Comment 63

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.2 variant 2

Okay, so we have a bit of progress.
But I came up with 2 variants. Variant 2 is my favorite.
Variant 1 is a bit go-go way of doing it.

Please help me select one so that I can proceed.

Thanks.

Comment 64

[Suyash Agarwal (:sshagarwal)]

Attachment: AB Screenshot

Oh and here is the screenshot of AB window after any
of these patches.

Thanks.

Comment 65

[Richard Marti (:Paenglab)]

First, this is really looking good.

I tried both versions and this is what I saw:
- On both versions in "All Address Books" when I move or copy a contact, this isn't updated: on move from "Collected Addresses" to normal AB the contact disappears until I select the AB and go back to all, on copy the new/doubled contact isn't shown until I select again thAB and go back to all.
- In version 1 the tree state (open/closed) isn't persistent.
- On both versions in Prefs/Composition/Addressing/Automatically add outgoing e-mail addresses to: is "All Address Books" also selectable. This shouldn't be. Either be hidden (preferred) or disabled.
- In both versions in message headers when in "Edit Contact" it's possible to select the "All Address Books". Like above this shouldn't be visible here. I tried pressing "Done" but nothing happens, good.

If it's to much to fix in this bug, this can be done in followups.

Code wise I can't way what version is better, but version 2 has a lot less changes. :)

Comment 66

[Wayne Mery (:wsmwk)]

++  But in the search results I'd like to see what AB each contact is in. (sort of like with global message searching)

And Version 2 may also make sense in the future AB, where presumably tags will be the basis for grouping contacts

Comment 67

[Thomas D. (:thomas8)]

Suyash, your delivery speed is amazing! :)

1) Can someone confirm if LDAP ABs are indeed included in directory iterations for "All ABs"?
2) Same question for other "exotic" types of ABs?
3) "All Address Books" must be localizable string (multiple occurences)
4) I wasn't sure about this:
> kAllDirectoryRoot + "?"
We seem to add the questionmark everywhere just to ultimately remove it again. Maybe we could do without it? (don't know, didn't check details)
5) Apart from label localization, could/should this also use constant kAllDirectoryRoot + "?" instead of the literal string?
menulist.appendItem("All Address books", "moz-abdirectory://?");

6) Thomas D (end of Comment 60), Richard (comment 61), and Wayne (comment 66) all concur that in the contacts list pane aka results list we need a new column showing the name of the containing "Address Book". Probably better done in a new bug.

Comment 68

[Suyash Agarwal (:sshagarwal)]

(In reply to Richard Marti (:Paenglab) from comment #65)
> I tried both versions and this is what I saw:
> - On both versions in "All Address Books" when I move or copy a contact,
> this isn't updated: on move from "Collected Addresses" to normal AB the
> contact disappears until I select the AB and go back to all, on copy the
> new/doubled contact isn't shown until I select again thAB and go back to all.
Are we talking about drag&drop? If yes, I'll need to investigate this.

> - In version 1 the tree state (open/closed) isn't persistent.
Ya :( But since so far everyone prefers variant 2, I am proceeding with it :)
Will fall back to variant 1 if someone says that's ultimately the legal way of doing
things.

> - On both versions in Prefs/Composition/Addressing/Automatically add
> outgoing e-mail addresses to: is "All Address Books" also selectable. This
> shouldn't be. Either be hidden (preferred) or disabled.
Done.
> - In both versions in message headers when in "Edit Contact" it's possible
> to select the "All Address Books". Like above this shouldn't be visible
> here. I tried pressing "Done" but nothing happens, good.
Done.

(In reply to Wayne Mery (:wsmwk) from comment #66)
> ++  But in the search results I'd like to see what AB each contact is in.
> (sort of like with global message searching)

(In reply to Thomas D. from comment #67)
> 1) Can someone confirm if LDAP ABs are indeed included in directory
> iterations for "All ABs"?
Yes. So far, LDAP is being iterated over. In fact, all the ab directories
are being iterated over. Since they all are under root AB directory.

> 3) "All Address Books" must be localizable string (multiple occurences)
Ya. I shouldn't have missed this. Done.

> 4) I wasn't sure about this:
> > kAllDirectoryRoot + "?"
> We seem to add the questionmark everywhere just to ultimately remove it
> again. Maybe we could do without it? (don't know, didn't check details)
Ya, you see this is why I said I am exploiting the code. Currently we aren't
asking for a directory with card results for kAllDirectoryRoot. And I can't
do it because there are other places where it is being asked for, so I had to
find a way for differentiating these present calls with my calls for cards under
all ABs as visible in variant 1.
So, we make it a searchQuery so that it is initiated. And then, finally we remove
it since it isn't needed :)
  
> 5) Apart from label localization, could/should this also use constant
> kAllDirectoryRoot + "?" instead of the literal string?
> menulist.appendItem("All Address books", "moz-abdirectory://?");
I removed this line and I have fallen back to adding this directory in InitCommonJS().
Since, keeping this required removing "All ABs" from various places as Paenglab mentioned.

> 6) Thomas D (end of Comment 60), Richard (comment 61), and Wayne (comment
> 66) all concur that in the contacts list pane aka results list we need a new
> column showing the name of the containing "Address Book". Probably better
> done in a new bug.
I think we have to bring it in this bug itself. Otherwise pushing out half featured patch
will again bring 100s of bug reports :P
I'll try to do it today.

So, currently I am sticking to variant 2. If someone thinks variant 1 is better and is the
preferred way of doing things, please let me know.

Thanks.

Comment 69

[Richard Marti (:Paenglab)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #68)
> (In reply to Richard Marti (:Paenglab) from comment #65)
> > I tried both versions and this is what I saw:
> > - On both versions in "All Address Books" when I move or copy a contact,
> > this isn't updated: on move from "Collected Addresses" to normal AB the
> > contact disappears until I select the AB and go back to all, on copy the
> > new/doubled contact isn't shown until I select again thAB and go back to all.
> Are we talking about drag&drop? If yes, I'll need to investigate this.

Yes drag&drop.

Comment 70

[Suyash Agarwal (:sshagarwal)]

Attachment: composition screenshot v2

Okay, here's what can be done.

Comment 71

[Suyash Agarwal (:sshagarwal)]

Attachment: AB Screenshot v2

And this is for AB main window.

Also, previously, double clicking on an addressbook entry
brought up its properties dialog.

Now, should I make it just expand/collapse its state (if it has
sub entries)?

Comment 72

[Richard Marti (:Paenglab)]

I think we shouldn't change the behavior and let double click show the properties dialog. On All ABs it should do nothing as it is not editable. If this isn't doable let it open the dialog with read only field.

Comment 73

[Suyash Agarwal (:sshagarwal)]

No the issue is, double clicking brings up the properties dialog and also toggles
the expanded/collapsed state. Is that okay?
If yes, we're good :)

Comment 74

[Richard Marti (:Paenglab)]

It's already now like this, so it's okay.

Comment 75

[Suyash Agarwal (:sshagarwal)]

Okay :)
And, should this "Address Book" column be visible only with "All Address Books" ?
As, for all other AB rows, same entry will be present in this column.

Comment 76

[Richard Marti (:Paenglab)]

Yes, only on All ABs. On the other it would be redundant.

Comment 77

[Onno Ekker [:nONoNonO UTC+1]]

Fot the "Address Book" column, you can also look in the behavior of the hidden (removed?) pref mail.autoComplete.commentColumn, which shows the address book a card is coming from on some of my computers in the compose window. (On other computers it just shows empty text behind the address book icon, see bug 1081475)...

Comment 78

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.1

Okay so finally I think I have a patch that fulfills
everyone's needs.

Please let me know what needs to be changed, or what else
needs to be incorporated.

Thanks.

Comment 79

[Richard Marti (:Paenglab)]

Comment on attachment 8543317 [details] [diff] [review]
Patch v3.1

This patch doesn't apply cleanly on tip.
I'm not able in "All ABs" to copy contacts to "Collected Addresses", to other ABs it's working. In normal ABs it's possible. Is this a bug?

The other things are really looking good.

Comment 80

[Suyash Agarwal (:sshagarwal)]

(In reply to Richard Marti (:Paenglab) from comment #79)
Thanks for the quick review.
> I'm not able in "All ABs" to copy contacts to "Collected Addresses", to
> other ABs it's working. In normal ABs it's possible. Is this a bug?
Works for me.
Maybe there's some change between my current tip and trunk's tip that's
responsible for this. Will investigate it today.

Thanks.

Comment 81

[Thomas D. (:thomas8)]

Some UX nits:

1) In contacts side bar only, "Address Book" column should be hidden by default (available for customization via column picker).
- space is limited in side bar
- in an unduplicated ABs setup and for most average use cases (including a large group of private users), Names should be enough to identify a contact; the same name usually should not occur in multiple ABs.
- side bar's main function is probably selecting recipients, for which "AB" column is usually not needed hence distracting; address management in side bar is an important, yet somewhat secondary/advanced use case.
- for main AB, showing "Address Book" column by default is OK (more space, address management/de-duplication, ux-consistent with other searches).

2) Double click on "All Address Books" should toggle subtree AND show readonly Properties dialogue with AB name, as we do for all other ABs (readonly for Collected Addresses and PAB). (This might be implemented by current patch already, just ftr).

3) Further points (from Comment 60), either here or new bugs:
a) For contact card properties, now need a display of parent AB
- At the top of card properties dialog, above the tabs, there should be a dropdown list for showing and changing the current parent AB of the card. The full functionality is available (right-click on to-header in mail reader, Edit Contact), but with more and more ABs/directories around, this should now also become part of the properties dialogue for easy and efficient accessability from anywhere. Containing AB looks like an important contact property to me, viewing and changing of which should be easier.

b) For bonus points, search result cards (and cards in All ABs listing) should have "Open containing Address Book" contextual command
- This is useful and efficient for all types of search results.
Can be done in followup bug if necessary.

Comment 82

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.1

After this patch, user will be able to:
1) Search in all address books (both in main AB window and
   contacts panel in composition window).
2) View all contacts in all addressbooks at one place.
3) Copy/Move single/multiple card(s) from same/different addressbooks from the
   listing of "all address books".
4) Delete single/multiple card(s) from same/different address books at once
   (both from main AB window and contacts panel in composition window) using
   right click context menu, delete key and delete toolbar button(main AB window).
5) Open containing addressbook from the results of all address
   books in contacts panel in composition window.
6) View the addressbook that contains the card along with the
   card in "all address books" cards list.
7) Choose whether "Address Book" column will be visible when
   "All Address books" is being shown (both in main AB window
   and contacts side panel in composition window separately)
   and the behavior will persist.

There may be things other than this that are possible.
If any of the aforementioned abilities aren't granted to you
by this patch. Please let me know.

TODO: Tests. This patch must have broken any tests now.
Once the functionality and code is approved, I will look at them.

Thanks.

Comment 83

[Richard Marti (:Paenglab)]

Comment on attachment 8543534 [details] [diff] [review]
Patch v4.1

Looks good.

For a followup bug: from main window the menu Edit/Find/Search Addresses... or in AB window the menu Edit/Search Addresses... opens a window for advanced search which also needs the same functionality. I don't know how often this is used.

Comment 84

[:aceman]

Comment on attachment 8543534 [details] [diff] [review]
Patch v4.1

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

Thanks for taking this, Suyash!
General comments while playing with this:
- this seems to add a ton of features which relate questionably to this bug's description. Maybe e.g. the "Open containing AB" should be split into another bug.
- I tested it on a big AB collection (20000 cards in total) and it seemed OK. Loading the Contacts sidebar took about 2 seconds in a debug build, even with the Addressbook column enabled.
- The "open containing addressbook" menuitem opened just the Addressbook window, it didn't focus the specific AB, IF the "All address books" node was collapsed in the AB manager window.

::: mail/components/addrbook/content/abCommon.js
@@ +67,5 @@
>  
>          if (selectedDir &&
>  	    (selectedDir != kPersonalAddressbookURI) &&
> +	    (selectedDir != kCollectedAddressbookURI) &&
> +            (selectedDir != kAllDirectoryRoot + "?")) {

There are tabs in these lines.

@@ +314,5 @@
> +                               .substring(cards[ix].directoryId.indexOf("&") + 1);
> +        let directory = null;
> +        if (abList) {
> +          directory = GetDirectoryFromURI(abList.getItemAtIndex(
> +                                            getIndexOfLabel(abList, dirName)).value);

Is it good to look it up via directory name (item label)? Could there be duplicates? Why not using some unique internal ID or URI. What if somebody calls his addressbook "All address books"? :)

@@ +521,5 @@
> +    if (Services.prefs.getBoolPref("mail.show_ab_column")) {
> +      document.getElementById("addrbook").setAttribute("hidden", "false");
> +    } else {
> +      document.getElementById("addrbook").setAttribute("hidden", "true");
> +    }

Not sure if we would prefer:
document.getElementById("addrbook").setAttribute("hidden",
  Services.prefs.getBoolPref("mail.show_ab_column") ? "true" : "false");

::: mail/components/addrbook/content/abContactsPanel.js
@@ +128,5 @@
>  
>    parent.addEventListener("compose-window-close", AbPanelOnComposerClose, true);
>    parent.addEventListener("compose-window-reopen", AbPanelOnComposerReOpen, true);
> +
> +  let config = {attributes: true, childList: true};

I think the style is to have space after '{' and before '}'.

::: mail/components/addrbook/content/abTrees.js
@@ +18,5 @@
> +
> +function getDirectoryValue(aDir, aKey) {
> +  if (aKey == "ab_type") {
> +    if (aDir._directory.URI == kAllDirectoryRoot + "?")
> +      return "aab";

Please make rkent see this if the function can be extended from extensions to add new AB types. I am not sure if his Exchange addon has a specific AB type.

@@ +32,5 @@
> +  } else if (aKey == "ab_name") {
> +    return aDir._directory.dirName;
> +  }
> +  // This should never happen.
> +  return null;

Maybe you could do it that any unknown AB type sorts before "cab" ? Just return a special value here that is also placed in AB_ORDER.

::: mail/components/addrbook/content/addressbook.xul
@@ +446,5 @@
>                  <menu id="sortMenu" label="&sortMenu.label;" accesskey="&sortMenu.accesskey;">
>                      <menupopup id="sortMenuPopup" onpopupshowing="InitViewSortByMenu()">
> +                        <menuitem label="&addrbook.label;"
> +                                  id="cmd_SortByaddrbook"
> +                                  accesskey="addrbook.accesskey;"

Missing & and the entity is not defined in abResultsPaneOverlay.dtd. Also, all other entities in that file start with upper case.

::: mailnews/addrbook/content/abResultsPane.js
@@ +335,5 @@
>    if (gAbResultsTree)
>      gAbResultsTree.treeBoxObject.invalidate();
>  }
>  
> +function getIndexOfLabel(aMenuList, aLabel)

Can this function be shared in abCommon.js ? But first see the previous comments if it is actually needed.

@@ +443,5 @@
> +        if (topWindow)
> +          topWindow.focus();
> +        else
> +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");

This looks like duplicating some code. Can this calls the same window (the same code) that is used elsewhere, e.g. when you click Addressbook in Tools? Just pass it an argument which AB should get focused.

@@ +448,5 @@
> +        if (card) {
> +          let dirName = card.directoryId.substring(card.directoryId.indexOf("&") + 1);
> +          topWindow.addEventListener("abWindowLoaded", function() {
> +            let directory = topWindow.gDirectoryTreeView.getIndexForName(dirName);
> +            topWindow.document.getElementById("dirTree").view.selection.select(directory);

This doesn't work when the window is already open (which case you support above).

::: mailnews/addrbook/src/nsAbView.cpp
@@ +453,5 @@
>  
>  nsresult nsAbView::GetCardValue(nsIAbCard *card, const char16_t *colID,
>                                  nsAString &_retval)
>  {
> +  if (!NS_strcmp(colID, MOZ_UTF16("addrbook"))) {

Is there some safer comparison possible? Like nsString(colId).Equals("addrbook"). Or is this the common pattern? Yeah, there seem to be other char16_t comparisions later on.

Comment 85

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #84)
> Comment on attachment 8543534 [details] [diff] [review]
> Patch v4.1
> @@ +314,5 @@
> > +        if (abList) {
> > +          directory = GetDirectoryFromURI(abList.getItemAtIndex(
> > +                                            getIndexOfLabel(abList, dirName)).value);
> 
> Is it good to look it up via directory name (item label)? Could there be
> duplicates? Why not using some unique internal ID or URI. What if somebody
> calls his addressbook "All address books"? :)
Ya but I couldn't find any other way to identify the address book from the card.

Comment 86

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5.1

(In reply to :aceman from comment #84)
> Comment on attachment 8543534 [details] [diff] [review]
> Patch v4.1
> 
> @@ +443,5 @@
> > +        if (topWindow)
> > +          topWindow.focus();
> > +        else
> > +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> > +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> 
> This looks like duplicating some code. Can this calls the same window (the
> same code) that is used elsewhere, e.g. when you click Addressbook in Tools?
> Just pass it an argument which AB should get focused.

The method is in mailCore.js in mail/
I don't think we can use it here in mailnews/

Addressed all other comments.

Thanks.

Comment 87

[:aceman]

Comment on attachment 8550967 [details] [diff] [review]
Patch v5.1

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

Thanks for fixing the comments :)

::: mail/components/addrbook/content/abTrees.js
@@ +27,5 @@
> +    if (aDir._directory instanceof Ci.nsIAbMDBDirectory)
> +      return "mork";
> +    if (aDir._directory instanceof Ci.nsIAbLDAPDirectory)
> +      return "ldap";
> +    return "mapi+other";

I see this tries to solve the new AB types added via extensions. So if we get an unknown AB type, why do you return "mapi+other". Isn't "anyab" intended for that? Shouldn't you return it here instead of end of function?

::: mailnews/addrbook/content/abResultsPane.js
@@ +435,5 @@
> +          topWindow.focus();
> +          abAlreadyOpen = true;
> +        } else {
> +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");

Both TB and SM have a function called 'toAddressBook'. Would it be accessible from here?

@@ +437,5 @@
> +        } else {
> +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> +        }
> +        if (card) {

I think this "if (card)" block of code should be inside addressbook.xul/.js (pass a parameter to the window on which AB/card to focus and this code picks it up and does it.) It should be a centralized feature of the AB window, not of the callers. We do that e.g. in the preferences dialog, or filter list dialog.

But you would need to implement it in both TB and SM AB versions, so I suggest you just file a followup bug for it so we do not drag this one too much.

Comment 88

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #87)
> Comment on attachment 8550967 [details] [diff] [review]
> Patch v5.1
> 
> Review of attachment 8550967 [details] [diff] [review]:

> ::: mail/components/addrbook/content/abTrees.js
> @@ +27,5 @@
> > +    if (aDir._directory instanceof Ci.nsIAbMDBDirectory)
> > +      return "mork";
> > +    if (aDir._directory instanceof Ci.nsIAbLDAPDirectory)
> > +      return "ldap";
> > +    return "mapi+other";
> 
> I see this tries to solve the new AB types added via extensions. So if we
> get an unknown AB type, why do you return "mapi+other". Isn't "anyab"
> intended for that? Shouldn't you return it here instead of end of function?
> 
Oh ya, sorry. So, when should "mapi+other" be returned?

> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +435,5 @@
> > +          topWindow.focus();
> > +          abAlreadyOpen = true;
> > +        } else {
> > +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> > +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> 
> Both TB and SM have a function called 'toAddressBook'. Would it be
> accessible from here?
> 
No, that's the same thing; its in mail/. But I'll try calling it here once and see if it is available.
> @@ +437,5 @@
> > +        } else {
> > +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> > +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> > +        }
> > +        if (card) {
> 
> I think this "if (card)" block of code should be inside addressbook.xul/.js
> (pass a parameter to the window on which AB/card to focus and this code
> picks it up and does it.) It should be a centralized feature of the AB
> window, not of the callers. We do that e.g. in the preferences dialog, or
> filter list dialog.
> 
Ya, I was thinking of it if the function from mailCore.js was available here. But since
this was the only user of the proposed method, I thought to leave it. Will do it now.

> But you would need to implement it in both TB and SM AB versions, so I
> suggest you just file a followup bug for it so we do not drag this one too
> much.
That'll be better :)

Thanks.

Comment 89

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #88)
> (In reply to :aceman from comment #87)
> > Comment on attachment 8550967 [details] [diff] [review]
> > Patch v5.1
> > 
> > Review of attachment 8550967 [details] [diff] [review]:
> 
> > ::: mail/components/addrbook/content/abTrees.js
> > @@ +27,5 @@
> > > +    if (aDir._directory instanceof Ci.nsIAbMDBDirectory)
> > > +      return "mork";
> > > +    if (aDir._directory instanceof Ci.nsIAbLDAPDirectory)
> > > +      return "ldap";
> > > +    return "mapi+other";
> > 
> > I see this tries to solve the new AB types added via extensions. So if we
> > get an unknown AB type, why do you return "mapi+other". Isn't "anyab"
> > intended for that? Shouldn't you return it here instead of end of function?
> > 
> Oh ya, sorry. So, when should "mapi+other" be returned?
Does it have a special class as Ci.nsIAb*Directory ? I see now this code is just copied from original code. So if there is no special class to detect then the "unknown AB" is actually the "other" part and is already covered by this "mapi+other" string. No need for "anyab" then.

> > ::: mailnews/addrbook/content/abResultsPane.js
> > Both TB and SM have a function called 'toAddressBook'. Would it be
> > accessible from here?
> > 
> No, that's the same thing; its in mail/. But I'll try calling it here once
> and see if it is available.
I see it also at http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailTasksOverlay.js#21

> > @@ +437,5 @@
> > > +        } else {
> > > +          topWindow = window.open("chrome://messenger/content/addressbook/addressbook.xul", "_blank",
> > > +                                  "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> > > +        }
> > > +        if (card) {
> > 
> > I think this "if (card)" block of code should be inside addressbook.xul/.js
> > (pass a parameter to the window on which AB/card to focus and this code
> > picks it up and does it.) It should be a centralized feature of the AB
> > window, not of the callers. We do that e.g. in the preferences dialog, or
> > filter list dialog.
> > 
> Ya, I was thinking of it if the function from mailCore.js was available
> here. But since
> this was the only user of the proposed method, I thought to leave it. Will
> do it now.
How does this relate to toAddressBook in mailCore.js? I think this code should be in addressbook.js. But that is for the other bug.

Comment 90

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5.2

Made a slight mistake in the last patch.
Ya, toAddressBook() is unavailable in mailnews/ so I assume toOpenWindowByType() will be unavailable as well.

So, I'll file a followup for this. Till then, I think we should get this in :)

Thanks.

Comment 91

[hopeandhealing]

Can someone show me the SIMPLE step by step on how to apply/input/insert this patch into Thunderbird.
All the language in these posts is Greek to me.

I am not a programmer and have no experience in the internal coding of Thunderbird.

But I need a quick fix for 'Enabling search in multiple/all address books'

Is there and add-on or a simple way to fix this?

Comment 92

[Suyash Agarwal (:sshagarwal)]

(In reply to hopeandhealing from comment #91)
> Can someone show me the SIMPLE step by step on how to apply/input/insert
> this patch into Thunderbird.
Hi, since this fix contains changes that requires building, I don't think you can
use it in existing TB. I suggest you wait just a bit more, it'll not take much
time now. I am active on this. I hope to get this in the major release. But even before that,
once this bug is marked closed, you can get the next daily that'll be having this feature.

Thanks.

Comment 93

[hopeandhealing]

Thanks, and please let me know when that feature is out on TB 3.

Comment 94

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5.4

Okay so there was an issue that on the "first run", the "All ABs" entry was
collapsed. This patch fixes that.

Thanks.

Comment 95

[Magnus Melin [:mkmelin]]

Comment on attachment 8551412 [details] [diff] [review]
Patch v5.2

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

::: mail/components/addrbook/content/abCommon.js
@@ +294,5 @@
> +    if (GetSelectedDirectory() != (kAllDirectoryRoot + "?")) {
> +      gAbView.deleteSelectedCards();
> +    } else {
> +      let cards = GetSelectedAbCards();
> +      for (let ix = 0; ix < cards.length; ix++) {

"i" is shorter ;)

@@ +490,5 @@
> +  // "All address books". Since the value of this column is redundant in
> +  // all other cases.
> +  if (GetSelectedDirectory() == (kAllDirectoryRoot + "?")) {
> +    document.getElementById("addrbook").setAttribute("hidden",
> +      Services.prefs.getBoolPref("mail.show_ab_column") ? "false" : "true");

I don't think you need to spell out the "true" and "false"? (just use the bool pref value)

::: mail/components/addrbook/content/abContactsPanel.js
@@ +66,5 @@
> +  let addrbookColumn = document.getElementById("addrbook");
> +  if (abList.selectedIndex == 0) {
> +    addrbookColumn.setAttribute("hidden",
> +      Services.prefs.getBoolPref("mail.show_ab_column_contactpanel") ?
> +      "false" : "true");

same here

@@ +132,5 @@
> +  });
> +
> +  document.getElementById("addrbook").setAttribute("hidden",
> +    Services.prefs.getBoolPref("mail.show_ab_column_contactpanel") ?
> +    "false" : "true");

just use the boolean value

::: mail/components/addrbook/content/addressbook.js
@@ +103,5 @@
>    MailServices.ab.removeAddressBookListener(gDirectoryTreeView);
>  
> +  obs.disconnect();
> +  document.getElementById("addrbook")
> +          .removeEventListener("ValueChange", updateABColumnState);

huh? surely you don't get mutations passed in here, so it does nothing?

@@ +130,5 @@
>  {
>    return gAddressBookAbViewListener;
>  }
>  
> +function updateABColumnState(aMutations)

i'd just inline this function

::: mailnews/addrbook/content/abDragDrop.js
@@ +282,5 @@
>            directory.dropCard(card, needToCopyCard);
> +
> +          // This if will qualify as true only if srcURI is "All ABs" and
> +          // action is moving.
> +          if (srcDirectory != null) {

if (srcDirectory)

::: mailnews/addrbook/content/abResultsPane.js
@@ +448,5 @@
> +            }
> +
> +            let directory = view.getIndexForId(MailServices
> +                                               .ab
> +                                               .getDirectoryFromId(dirId).URI);

just move MailServices down a line, indent by two and put all of the rest on one line

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +237,5 @@
> +  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {
> +    rv = enumerator->GetNext(getter_AddRefs(support));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    directory = do_QueryInterface(support, &rv);
> +    //NS_ENSURE_SUCCESS(rv, rv);

remove

@@ +251,5 @@
> +  }
> +
> +  return NS_OK;
> +}
> +  

whitespace

::: mailnews/mailnews.js
@@ +370,5 @@
>  pref("mail.collect_email_address_newsgroup", false);
>  #endif
>  pref("mail.collect_email_address_outgoing", true);
> +pref("mail.show_ab_column", true);
> +pref("mail.show_ab_column_contactpanel", false);

please add documentation for these

Comment 96

[Richard Marti (:Paenglab)]

Comment on attachment 8551961 [details] [diff] [review]
Patch v5.4

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

The migration doesn't work here. I get this:

Error: Migrating from UI version 9 to 10 failed. Error message was: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsISafeOutputStream.finish]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource:///modules/mailMigrator.js :: MailMigrator._migrateUI :: line 306"  data: no] -- Will reattempt on next start.
Source file: resource://app/modules/mailMigrator.js
Line: 314

::: mail/base/modules/mailMigrator.js
@@ +268,5 @@
>            Services.prefs.clearUserPref("intl.charset.detector");
>          }
>        }
>  
> +      if (currentUIVersion < 10) {

Please can you add a short comment what this migrates?

Comment 97

[:aceman]

I get a different error:
Error: SyntaxError: JSON.parse: unterminated string at line 1 column 45 of the JSON data
Source File: chrome://messenger/content/addressbook/abTrees.js
Line: 162

Because the file contains wrong data:
["moz-abdirectory://?","moz-abdirectory://?]

The inserting of the string probably cuts off wrongly.

Comment 98

[Suyash Agarwal (:sshagarwal)]

(In reply to Richard Marti (:Paenglab) from comment #96)
> Comment on attachment 8551961 [details] [diff] [review]
> Patch v5.4
> The migration doesn't work here. I get this:
> 
> Error: Migrating from UI version 9 to 10 failed. Error message was:
> [Exception... "Component returned failure code: 0x80520015
> (NS_ERROR_FILE_ACCESS_DENIED) [nsISafeOutputStream.finish]"  nsresult:
> "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame ::
> resource:///modules/mailMigrator.js :: MailMigrator._migrateUI :: line 306" 
> data: no] -- Will reattempt on next start.
> Source file: resource://app/modules/mailMigrator.js
> Line: 314
Weird. I have no idea how to handle access denied issue :) Need to investigate.

(In reply to :aceman from comment #97)
> I get a different error:
> Error: SyntaxError: JSON.parse: unterminated string at line 1 column 45 of
> the JSON data
> Source File: chrome://messenger/content/addressbook/abTrees.js
> Line: 162
> 
> Because the file contains wrong data:
> ["moz-abdirectory://?","moz-abdirectory://?]
> 
> The inserting of the string probably cuts off wrongly.

The duplicate entry can probably be explained with the case that you already had
built once with this patch and so had all ABs in your directoryTree.json, but after
this patch, it added another entry. So, I'll add a case where it checks if the entry
is already there and skips it but adds it otherwise (rare for users?).
But the trimmed quote is again weird. I need to get things right.

Moreover, another discrepancy found.
Adding new AB fails for me. Can you please confirm that you see this as well?

Thanks.

Comment 99

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8551961 [details] [diff] [review]
Patch v5.4

Clearing review requests as the patch has issues. Added behavior isn't experienced by reviewers.
Adding new AB fails. (It is created, you can see the values in All ABs
but the entry isn't made in the dirTree).

Thanks.

Comment 100

[:aceman]

You do not need to worry about the value appearing in the array twice. Yes it is probably a remnant from previous testing of the patch and will not be encountered on normal profile. Just fix the missing quote.

Comment 101

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6

Okay so this is complete from my side now.
- Fixed the missing quote issue.
- Disabled "new mailing list" and "new card" option for "All ABs". This
also fixed the "All ABs" blank entry in "Add new list" and "add new card"
dialog.

- Adding new AB wasn't updating the view. Though everything was working
fine in the backend. That is probably because the "childNodes" code wasn't
updating because of "mInitialized" being true after the list is generated once. The code path for adding mailing lists probably could be used but since
our case is entirely separate (of All ABs), so, instead of going through fixing all the listeners, I added a special case in abTrees.js which seems fine to me
as per the performance measures as well.

- There was just one test failing (just because of the extra entry), fixed it.

So, let me know if there's anything else that needs to be done to get this in.

Thanks.

Comment 102

[Richard Marti (:Paenglab)]

Comment on attachment 8552913 [details] [diff] [review]
Patch v6

The migration works now. Thanks for your hard work!

Comment 103

[:aceman]

Collapsing some rows in the AB window sometimes produces:
Warning: ReferenceError: reference to undefined property this._rowMap[aIndex]._parent
Source File: chrome://messenger/content/jsTreeView.js
Line: 59

Creating a new list under Personal AB produces:
Warning: ReferenceError: reference to undefined property params.idKey
Source File: file:///var/SSD/TB-hg/tbird-bin/dist/bin/components/nsAbAutoCompleteSearch.js
Line: 393

Error: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]
Source File: chrome://messenger/content/addressbook/abCommon.js
Line: 622

Timestamp: 23.01.2015 23:06:40
Error: An error occurred updating the cmd_newlist command: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger/content/addressbook/abCommon.js :: GetDirectoryFromURI :: line 622"  data: no]
Source File: chrome://global/content/globalOverlay.js
Line: 83

Aside from the "params.idKey" error, these are not shown when running without this patch.

Comment 104

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.2

(In reply to :aceman from comment #103)
> Collapsing some rows in the AB window sometimes produces:
> Warning: ReferenceError: reference to undefined property
> this._rowMap[aIndex]._parent
> Source File: chrome://messenger/content/jsTreeView.js
> Line: 59
Just occurred for me once. I think that's common :P We can always
look at it later.

> Creating a new list under Personal AB produces:
> Warning: ReferenceError: reference to undefined property params.idKey
> Source File:
> file:///var/SSD/TB-hg/tbird-bin/dist/bin/components/nsAbAutoCompleteSearch.js
> Line: 393
I am unable to reproduce this error.
 
> Error: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a
> (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]
> Source File: chrome://messenger/content/addressbook/abCommon.js
> Line: 622
> 
> Timestamp: 23.01.2015 23:06:40
> Error: An error occurred updating the cmd_newlist command: [Exception...
> "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI)
> [nsIAbManager.getDirectory]"  nsresult: "0x804b000a
> (NS_ERROR_MALFORMED_URI)"  location: "JS frame ::
> chrome://messenger/content/addressbook/abCommon.js :: GetDirectoryFromURI ::
> line 622"  data: no]
> Source File: chrome://global/content/globalOverlay.js
> Line: 83
> 
Ya, my mistake. I added a check for "All ABs" but removed the check for nullity.
Added it back, no errors now in the console.

> Aside from the "params.idKey" error, these are not shown when running
> without this patch.

Thanks.

Comment 105

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #104)
> (In reply to :aceman from comment #103)
> > Collapsing some rows in the AB window sometimes produces:
> > Warning: ReferenceError: reference to undefined property
> > this._rowMap[aIndex]._parent
> > Source File: chrome://messenger/content/jsTreeView.js
> > Line: 59
> Just occurred for me once. I think that's common :P We can always
> look at it later.
Yes, may be a generic error with collapsing rows in the AB tree.

> > Creating a new list under Personal AB produces:
> > Warning: ReferenceError: reference to undefined property params.idKey
> > Source File:
> > file:///var/SSD/TB-hg/tbird-bin/dist/bin/components/nsAbAutoCompleteSearch.js
> > Line: 393
> I am unable to reproduce this error.
It happens when you type the first address into the maillist. It tries to autocomplete it and gives this warning. I file bug 1125506 for it as it seems to happen also in base code.

> > Error: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a
> > (NS_ERROR_MALFORMED_URI) [nsIAbManager.getDirectory]
> > Source File: chrome://messenger/content/addressbook/abCommon.js
> > Line: 622
> > 
> > Timestamp: 23.01.2015 23:06:40
> > Error: An error occurred updating the cmd_newlist command: [Exception...
> > "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI)
> > [nsIAbManager.getDirectory]"  nsresult: "0x804b000a
> > (NS_ERROR_MALFORMED_URI)"  location: "JS frame ::
> > chrome://messenger/content/addressbook/abCommon.js :: GetDirectoryFromURI ::
> > line 622"  data: no]
> > Source File: chrome://global/content/globalOverlay.js
> > Line: 83
> Ya, my mistake. I added a check for "All ABs" but removed the check for
> nullity.
> Added it back, no errors now in the console.
Seems to work now.

Comment 106

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8554142 [details] [diff] [review]
Patch v6.2

I think we need this to be reviewed quick soon since, in a short time,
we won't be taking string changes in.

So, I request the reviewers to kindly look into this stuff a bit soon.

Thanks.

Comment 107

[Magnus Melin [:mkmelin]]

Comment on attachment 8554142 [details] [diff] [review]
Patch v6.2

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

(not finished reviewing yet)

::: mail/base/modules/mailMigrator.js
@@ +284,5 @@
> +                        .createInstance(Ci.nsIScriptableInputStream);
> +          fstream.init(file, -1, 0, 0);
> +          sstream.init(fstream);
> +          while (sstream.available())
> +            data += sstream.read(4096);

braces for all whiles

::: mail/components/addrbook/content/addressbook.js
@@ +103,5 @@
>    MailServices.ab.removeAddressBookListener(gDirectoryTreeView);
>  
> +  obs.disconnect();
> +  document.getElementById("addrbook")
> +          .removeEventListener("ValueChange", updateABColumnState);

?? see my previous comment about this

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +256,1 @@
>  NS_IMETHODIMP nsAbManager::GetDirectory(const nsACString &aURI,

trailing whitespace

Comment 108

[Magnus Melin [:mkmelin]]

Comment on attachment 8554142 [details] [diff] [review]
Patch v6.2

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

First, see my earlier review, comment 95. 

On a general level: it's a bit awkward to have the "All addressbooks" a top level for all the others. Especially for LDAP abs you probably don't see all addresses listed beneath so the view is misleading. (Didn't verify that though.) Over all I'm not sure why we have that tree entry at all. Search could just search all books and show which one the hit is in. 

There's a bunch of exceptions for Print preview. Also, Search addressbooks (from the menu) doesn't list the "All" option, just a borked empty entry at index 0.

This also needs some basic tests.

::: mail/base/modules/mailMigrator.js
@@ +268,5 @@
>            Services.prefs.clearUserPref("intl.charset.detector");
>          }
>        }
>  
> +      if (currentUIVersion < 10) {

document what's changing

::: mail/components/addrbook/content/abCommon.js
@@ +68,5 @@
>  
>          if (selectedDir &&
> +           (selectedDir != kPersonalAddressbookURI) &&
> +           (selectedDir != kCollectedAddressbookURI) &&
> +           (selectedDir != (kAllDirectoryRoot + "?"))) {

these 3 lines should be indented one more space

@@ +301,5 @@
> +    if (GetSelectedDirectory() != (kAllDirectoryRoot + "?")) {
> +      gAbView.deleteSelectedCards();
> +    } else {
> +      let cards = GetSelectedAbCards();
> +      for (let ix = 0; ix < cards.length; ix++) {

lets just use i

@@ +494,5 @@
>    }
> +
> +  // Hide the addressbook column if the selected addressbook isn't
> +  // "All address books". Since the value of this column is redundant in
> +  // all other cases.

This is probably reduntant, as in other bugs people request the ability to show which ab it's in.

::: mail/components/addrbook/content/abTrees.js
@@ +10,5 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource:///modules/mailServices.js");
>  
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

c-c doesn't define these globally like this

@@ +115,5 @@
>          this._children[this._children.length - 1]._level = this._level + 1;
>          this._children[this._children.length - 1]._parent = this;
>        }
>  
> +      // We sort children;

can remove this comment

@@ +244,5 @@
> +    for (let i = 0; i < this._rowMap.length; i++) {
> +      if (this._rowMap[i].id == aId)
> +        return i;
> +    }
> +  },

this should always return something

::: mail/components/addrbook/content/addressbook.js
@@ +193,5 @@
>    // Initialize the Address Book tree view
>    gDirectoryTreeView.init(gDirTree,
>                            kPersistCollapseMapStorage);
>  
> +  let config = { attributes: true, childList: true };

unused

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +239,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    directory = do_QueryInterface(support, &rv);
> +    //NS_ENSURE_SUCCESS(rv, rv);
> +    if (NS_FAILED(rv))
> +      continue;

probably should just error out. or if not, why continue?

Comment 109

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.5

(In reply to Magnus Melin from comment #108)
> Patch v6.2
> First, see my earlier review, comment 95. 
> 
> On a general level: it's a bit awkward to have the "All addressbooks" a top
> level for all the others. Especially for LDAP abs you probably don't see all
> addresses listed beneath so the view is misleading. (Didn't verify that
> though.) Over all I'm not sure why we have that tree entry at all. Search
> could just search all books and show which one the hit is in. 
Ya but, it was the path to the root and it provides much enhanced functionality like instant duplicate contacts visibility, multiple contacts copy/move from multiple address books at once etc. Overall, a better AB management is provided so, I think we can use this.

> 
> There's a bunch of exceptions for Print preview. Also, Search addressbooks
> (from the menu) doesn't list the "All" option, just a borked empty entry at
> index 0.
There were two exceptions for me but print preview was generated fine. Didn't check without the patch (will do it later today).

Ya, we were holding it for a followup, but since it didn't need much work, did it here. 
Now, we can perform advanced search on All ABs too.

> 
> This also needs some basic tests.
Oh ya, tests. So, what do you want to be tested? I didn't find anything special
being offered with this new entry.

> @@ +494,5 @@
> >    }
> > +
> > +  // Hide the addressbook column if the selected addressbook isn't
> > +  // "All address books". Since the value of this column is redundant in
> > +  // all other cases.
> 
> This is probably reduntant, as in other bugs people request the ability to
> show which ab it's in.
I am unable to understand why people want the AB column for a single AB when it is going to be filled up with the name that is already selected on the left.
But okay, if they want this, we have it. Many comments pertaining to Mutation Observer, pref etc. go away because I have removed this code now.
But I have still kept this in contacts panel in compose window as there is space constraint and the AB name is just right above :)

> ::: mailnews/addrbook/src/nsAbManager.cpp
> @@ +239,5 @@
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    directory = do_QueryInterface(support, &rv);
> > +    //NS_ENSURE_SUCCESS(rv, rv);
> > +    if (NS_FAILED(rv))
> > +      continue;
> 
> probably should just error out. or if not, why continue?
I thought, if we are unable to select a directory, let's just skip it and
continue with listing other ABs. Isn't that right? If not, let me know, I'll error
it out here.

Thanks.

Comment 110

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #109)
> Created attachment 8560318 [details] [diff] [review]
> Patch v6.5
> 
> (In reply to Magnus Melin from comment #108)
> > Patch v6.2
> > First, see my earlier review, comment 95. 
> > 
> > On a general level: it's a bit awkward to have the "All addressbooks" a top
> > level for all the others. Especially for LDAP abs you probably don't see all
> > addresses listed beneath so the view is misleading. (Didn't verify that
> > though.) Over all I'm not sure why we have that tree entry at all. Search
> > could just search all books and show which one the hit is in. 
> Ya but, it was the path to the root and it provides much enhanced
> functionality like instant duplicate contacts visibility, multiple contacts
> copy/move from multiple address books at once etc. Overall, a better AB
> management is provided so, I think we can use this.

+1. Magnus, Richard (comment 61) and I (comment 54) had explicitly requested and approved the "All Address Books" top level tree entry UI because it provides a very powerful and intuitive address management UX. "Just search all books" (always or optionally) would come nowhere near such UX power, i.e. we would lose a lot of really cool use cases like those mentioned by Suyash. Furthermore, this UI is perfectly externally ux-consistent with similar functionality on OS level: In Windows Explorer (Win 7/8), select parent directory "My Computer" or its child "My Documents" respectively and watch the search field in the upper right corner. Whichever container you select on the left will be searched. So actually conceptionally this is more like a *filter* for whichever set of data you have preselected on the left. In this concept, searching siblings on same level or parent directories would be unexpected and confusing, and also less ux-efficient.

Having said which, I agree with Magnus that we need to be correct and ensure that labels and hierarchies suggested by UI actually match what's inside. So as I have repeatedly stated before, if we make "All Address Books" a parent directory which has LDAP ABs as children, then their content must show in "All ABs" view. So we still need someone to test this with LDAP (I tried but didn't get far). If it doesn't work (yet), we still have several (temporary) options which preserve the general concept, e.g.
- rename to "All Local ABs", and make that a /sibling/ of all ABs instead of /parent/
- rename to "All Local ABs", and make that a parent only of local ABs, plus:
- keep LDAP ABs as top-level entries, or have their own "All LDAP ABs" parent

Comment 111

[Thomas D. (:thomas8)]

Suyash (or anyone), can you pls provide an updated try build for testing purposes. Tia.

Here's preliminary UX feedback based on OLD(!) try build dated 4th January (sorry for the lag):

0) This feature is AWESOME!!! :)
1) "Address book" column in main AB view should be last column, not first (ux-consistent with other searches like Global Search from inbox; containing AB is less significant over contact name and, depending on scenario, not always required to use the result set. User can move containing AB column to front at any time if he so wishes by dragging column header, which should be preserved).
2) Theme nit (@Richard), not affected by this bug: Background color of non-editable AB name in AB > Properties dialogue of special ABs should be grey (not white) to indicate that it's read-only.
3) "All ABs" should be expanded after first install (perhaps already fixed?)
4) I had an (initial) state in compose window (right after first install, create some contacts in all the different ABs, then click "Write" only), where "All ABs" was active in contacts side bar, but only contacts from "Collected Addresses" were showing (do we have testcases for this?).
5) Editing properties of All-AB contacts from contacts side bar, change a name property, OK did not close dialogue (perhaps already fixed?)

Comment 112

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

Pushed to try-comm-central as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bebd46b1f954

Comment 113

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #111)

Thanks Suyash & Archaeopteryx for letting me play with the real thing (try build) :)

Issues/UX review of today's try build
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bebd46b1f954

> 0) This feature is AWESOME!!! :)
...and getting better by the day! :)

> 1) "Address book" column in main AB view should be last column, not first
> 2) Theme nit (@Richard), not affected by this bug: Background color of
> non-editable AB name in AB > Properties dialogue of special ABs should be
> grey (not white) to indicate that it's read-only.

1 & 2 are still applicable.

> 3) "All ABs" should be expanded after first install (perhaps already fixed?)
> 4) I had an (initial) state in compose window (right after first install,
> create some contacts in all the different ABs, then click "Write" only),
> where "All ABs" was active in contacts side bar, but only contacts from
> "Collected Addresses" were showing (do we have testcases for this?).

3 & 4 Testcases would be good; didn't recheck.

> 5) Editing properties of All-AB contacts from contacts side bar, change a
> name property, OK did not close dialogue

5 still applicable, after editing contacts, clicking OK to save the modified contact data does nothing, and changes are lost. This fails only from contacts side bar.

6) Have one contact named "foo" in any AB; open contacts side bar with "all abs", verify exactly one "foo" is shown. From main AB (!), edit properties of contact, change display name by adding "bar" so it's now "foobar". Surprisingly (different from 5), this works. BUT: Alt+Tab to go back to the composition, and without clicking anything, look at contents of contacts side bar. There's a ghost duplicate entry of "foobar"; deleting one of them will delete "foobar" contact entirely -> dataloss!

7) I managed to get into a state where in main AB, All ABs was selected, and I could use "File > New > Mailing List", which brought up mailing list dialogue with blank AB name. Creating the list failed somehow. Next restart crashed. I did a lot of "open containing AB" around the time, but I wasn't able to reproduce. Maybe it's on a new profile, first time opening of menu only, or some other odd condition.

8) There are a number of UX problems around "Open containing AB"
8a) Have "John" and "Jane" in PAB. In main AB view, select "John"(!) in PAB. Keep window open. From contacts side bar, "all abs", do "Open containing AB" on "Jane"(!). main AB view is activated, but we wrongly preserve the old selection on "John". Confusing and error-prone, dataloss dangers.

For "open containing AB" command, if only one contact is selected when calling context menu, that same contact should be selected in the source AB. If it's not possible, at least clear old selection from that AB (set index to -1 or something).

IMO there's no reason to ever disable "Open containing AB" command from *contacts side bar*:
8b) for multiple selected contacts from *same* AB, open the AB and blur selection (or even try to select the same contacts). this currently fails and will focus AB window, but not select the correct AB.
8c) for multiple selected contacts from *different* ABs, "Open containing AB" is currently enabled but does not select any AB. Instead of disabling, we should just open "All ABs view" with no selection (or select the same selected contacts if we can). It'll be a consistent and convenient shortcut go get into main AB view regardless of which contact(s) are selected.

If we are in a hurry, we could postpone the details to another bug and for now make it so that "Open containing AB" is only available for single contact selection.

9) In main AB, would like to see "Open containing AB" contextual action on "All ABs" search result contacts, working as described in 8). For ux-efficiency, we should never force the user to manually research and reselect elements which are already right in front of his eyes, and their whereabouts known.

10) "Advanced AB search" dialogue does not have a "Address book" column for "all ABs" results (or any results as we postulated, even for known single ABs), and needs a button for "Open containing AB". Can be done in a new bug. There's another known bug that these results don't have a context menu.

So much for now... ;)

Comment 114

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #110)
> Having said which, I agree with Magnus that we need to be correct and ensure
> that labels and hierarchies suggested by UI actually match what's inside. So
> as I have repeatedly stated before, if we make "All Address Books" a parent
> directory which has LDAP ABs as children, then their content must show in
> "All ABs" view. So we still need someone to test this with LDAP (I tried but
> didn't get far). 

No, that's not how LDAP is meant to be used, and you'd often have quite a lot, like thousands (10 000 is surely not at all uncommon) of entries. It's not directly comparable to your local address book.

FWIW, I often use this public ldap to see if things work: https://howto.adams.edu/index.php/Thunderbird_-_Add_LDAP_to_Address_Book

But even so, I don't think having "All abs" in the tree view just to be able to search all is a good idea. It's don't even think people assume the search works with the ab selected (and some bug i read recently was about exactly that). 

As it is about search, why not put a ab dropdown selector (one including "All") next to the search field?

Comment 115

[Suyash Agarwal (:sshagarwal)]

So, why not we change the title of this bug to "Implement functionality of All ABs"? :)
Or, are you suggesting that we should drop everything that's been implemented in this patch and just
keep the search ability? If yes, it was the very first draft patch on this bug and the very first screenshot shows that.

Thanks.

Comment 116

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #109)

> > This also needs some basic tests.
> Oh ya, tests. So, what do you want to be tested? I didn't find anything
> special
> being offered with this new entry.

Would be nice with something to show that it finds contacts from more than one ab.

> > ::: mailnews/addrbook/src/nsAbManager.cpp
> > @@ +239,5 @@
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > > +    directory = do_QueryInterface(support, &rv);
> > > +    //NS_ENSURE_SUCCESS(rv, rv);
> > > +    if (NS_FAILED(rv))
> > > +      continue;
> > 
> > probably should just error out. or if not, why continue?
> I thought, if we are unable to select a directory, let's just skip it and
> continue with listing other ABs. Isn't that right? If not, let me know, I'll

I'm not sure when that would happen. I guess it can be ok after all but maybe make it a warning, and remove the uncommented ns_ensure_success.

Comment 117

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #115)
Yes I'd suggest to drop the tree part of the patch. So what do you think about something like that initial idea, but not with a changed button but with an ab selector next to it?

Comment 118

[Thomas D. (:thomas8)]

Thanks Magnus for providing the test LDAP directory in comment 114, and good catch!

Indeed the current design exactly as-is is not feasible because LDAP directories work differently from local ABs:
- LDAP directories show NO entries unless there's a filter phrase (this is the main issue)
- LDAP filter results are limited to a default 100 entries (customizable in AB properties)
Hence the current top-level entries (both in main AB and contacts side bar) wrongly suggest (from tree design and/or labeling) that we're showing ALL content of ALL ABs, which is not the case for LDAP until there's a search phrase. So without search phrase, LDAP entries are completely excluded, BUT for added complication, as soon as there IS a search phrase, we'll want LDAP entries to be included again...

So indeed we need to go back to the drawing board and find a better design which reflects that reality.

Tentative ideas:
- Suyash along first patch, see screenshot 8542400 [Global search *push* button next to filter input]
- Thomas along comment 54, 3) [Global search *checkbox* button above AB list, or next to filter input]
- Magnus along comment 117 [dedicated (duplicate?) AB selector for search/filter field]
- (Thomas along the end of comment 110 doesn't work either for the global search/filter scenario ["All Local ABs" top entry, rectify labels/tree hierarchy to indicate that LDAP is excluded; but then we want it *in*cluded for searching...])

Tricky. As we include LDAP, the charming simplicity and functionality of the current approach seems gone...

Comment 119

[Wayne Mery (:wsmwk)]

I haven't followed this realy close because, well, UI is not my thing. But it's time for me to chime in. I'm not sure we whether or not we need to go back to the drawing board. Personally I don't think we need to make the UI more complex nor make the user make more choices. So my thoughts are (and some of them may have already been mentioned) :

* including issues related to contact sidebar so that some sort of consistency can be achieved IMO goes well beyond the scope of this bug

* providing search all addressbooks without showing addressbook name is not useful. not useful at all. let's not go there (i.e. I don't think we want separate bug)

* regarding, ldap and other types of AB that do not fit the model of "search all AB", we can't let that get in the way achieving search the core goal of this bug.  If in the current models you can't exclude them it without making the UI more complex (eg selections and other BS), then I think we simply exclude them by default and make it clear in the search results UI that they were excluded (grey out the addressbook name?)

* Last, and perhaps most important - I suspect the model for the next gen address book is that search all addressbooks should be the *default* and that we allow the user to refine from that default (much like quick filters, eg exclude/include tags, etc). So, in retrospect I suppose, perhaps part of goal of this bug should be to make search all addressbooks the DEFAULT.  If necessary, maybe our first cut no longer allows search within an AB. (If search within only one AB is absolutely needed, someone please give a use case). Anyway, we can if necessary file bug(s) for refining search as future work.

Let's push on :)

Comment 120

[Richard Marti (:Paenglab)]

What about moving out the LDAP AB's of the "All AB" tree and put them to the same level. And the rename the "All Address Books" to "Local Address Books"? The expression "Local Address Books" is already used in the prefs in Composition/Addressing.

Here a try of an ASCII art:

Local Address Books
 ├ Personal Address Book
 ├ Other Address Book
 └ Collected Addresses
LDAP Address Book

The search on all ABs doesn't work with LDAP and the separation with the tree level makes this visible because on opening the AB window the "Local Address Books" line is selected.

Comment 121

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #120)
> What about moving out the LDAP AB's of the "All AB" tree and put them to the
> same level. And the rename the "All Address Books" to "Local Address Books"?
> The expression "Local Address Books" is already used in the prefs in
> Composition/Addressing.
> 
> Here a try of an ASCII art:
> 
> Local Address Books
>  ├ Personal Address Book
>  ├ Other Address Book
>  └ Collected Addresses
> LDAP Address Book

+1, I like that because it enables powerful features for local ABs like cross-ab full listing, de-duplication, cross-AB moving/copying etc.
However, it doesn't provide an UI yet for "Global AB search" including LDAP directories, see below.

> The search on all ABs doesn't work with LDAP and the separation with the
> tree level makes this visible because on opening the AB window the "Local
> Address Books" line is selected.

*Search/Filter* on all ABs works well *including LDAP* (user-definible max # of LDAP results, default 100). It's only *View all contacts* (without filter) where LDAP draws a blank, at least in our current design (I wonder if it's still a problem these days to get and show, say, a list of 10.000 or 20.000 contacts?).

So far I've assumed we'd want "Global AB search/filter" to be truly global, i.e. include results from LDAP (which is already realized in the current patch, see try build comment 112; only the UI representation for the "global listing/no filters(!)" case is a bit odd).

I also assume we're not here to remove existing per-AB filter functionality, which is obviously useful (just like Quick Filter Bar for mail folders).

Comment 122

[:aceman]

Yes, I also support search in one or All ABs and also showing all contacts in the AB window is a useful feature (but users must understand having a duplicate contact in some of personal ABs AND Collected Addresses is expected). I'd like that the current feature set is just refined so that no features needs to be cut from the patch. If that needs separating LDAP out of this I am OK with that. We could have a "Local Address Books" node and a "Remote Address books" node in the AB window.

Comment 123

[Thomas D. (:thomas8)]

Attachment: UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for search results)

Comment 124

[Thomas D. (:thomas8)]

Comment on attachment 8560983 [details]
UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for search results)

(In reply to :aceman from comment #122)
> Yes, I also support search in one or All ABs and also showing all contacts
> in the AB window is a useful feature [...].
> I'd like that the current feature set is just refined so that no
> features needs to be cut from the patch. If that needs separating LDAP out
> of this I am OK with that. We could have a "Local Address Books" node and a
> "Remote Address books" node in the AB window.

So here's a wireframe that seeks to incorporate as much of everyone's feedback as possible, while keeping things simple yet powerful.

This proposal suggests two new UI elements for global AB search:
1) "Search All ABs" iconic toggle button (on the right of AB filter input box)
2) AB scope selector bar (which appears below the filter box when there's a search phrase in the filter box, similar to message quick filter bar behaviour)

Other UI elements:
3) "Local Address Books" top level tree entry (because it's so powerful)
4) "Remote Address Books" top level tree entry (for symmetry, and also allowing powerful cross-"Remote Addres Books" filter/search)

To realize global search per this proposal, 1) is required; 2)-4) are optional but desirable.

Workflow:

Scene 1: "Personal AB" selected, no filter
-> AB filter box with emptyText "Search Personal Address Book"
-> AB contents showing

Scene 2: User enters filter term "Pete"
-> matches from "Personal AB" are shown (2 contacts)
-> number of matching contacts shown on the left of AB filter box (as in msg quick filter bar): 2 contacts [Pete           x ]
-> AB scope selector bar appears between filter box and search results, with several benefits:
- stronger visual indication that there's an active filter
- clear headline for search results showing current scope ("Search in [Personal AB  v]")
- AB scope dropdown selector allows targeted browsing/searching through other ABs on-the-fly with same filter applied (similar to "sticky pin" on msg quick filter bar)

Scene 3: User expands search scope to "Global AB search" by clicking "Search All ABs" iconic checkbox button next to filter box.
-> Global AB search button appears pressed
-> AB scope selector bar now showing "Search in: [All Address Books  v]"
-> temporarily remove selection indicator in AB list, because it's now a cross-AB view
-> matches from all ABs are shown (4 contacts)

Scene 4: User clears search term while in "Global AB search" mode
-> Global AB search button remains pressed (even when changing ABs)
-> AB scope selector bar disappears as there's no filter any more
-> AB filter box with emptyText "Search All Address Books"
-> Last AB which was selected by user is re-selected in AB list and contents shown

Scene 5 (not included in mockup): After scene 2), user changes scope from  AB selector bar, e.g. choose "Collected Addresses"
-> current filter ("Pete") is conveniently applied to "Collected Addreses"
-> "Collected Addresses" is selected in AB list (and remains so until another AB is chosen in scope selector or AB list)
-> "Search All ABs" button is toggled "off" (and remains so until clicked again)

Note the following cool choices available from AB scope selector (in this UX-proposal):
- All Address Books
- Local Address Books
  - ...
- Remote Address Books
  - ...

Comment 125

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #124)
> Comment on attachment 8560983 [details]
> UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for
> search results)

Note how LDAP searches are integrated seamlessly and correctly into this proposal.

When "Remote Address Books" node is selected, where we don't show any contents by design, we could helpfully have a hint in the empty contacts list pane to "Please enter a search phrase in the search box above", or some such.

Comment 126

[Richard Marti (:Paenglab)]

Comment on attachment 8560983 [details]
UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for search results)

f+ for the proposal, but
1. the actual patch is complex enough and we shouldn't add additional functionality except for:
(In reply to Thomas D. from comment #124)
> 3) "Local Address Books" top level tree entry (because it's so powerful)
and if possible
> 4) "Remote Address Books" top level tree entry (for symmetry, and also
> allowing powerful cross-"Remote Address Books" filter/search)

2. we are 2 weeks before uplift with the string freeze and this could make it harder to land before.

I propose, Thomas, you file a new bug for your proposal and make a patch with the needed strings (with consulting Suyash) to land before the uplift and Suyash can, when he has time, finish it in the next cycle and then land and uplift to Aurora. If this patch misses TB 38, then we have already a basic functionality, which already satisfy all users without LDAP, with the patch from this bug.

Comment 127

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #126)

Thanks, I agree.

> Comment on attachment 8560983 [details]
> UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for
> search results)
> 
> f+ for the proposal, but
> 1. the actual patch is complex enough and we shouldn't add additional
> functionality except for:
> (In reply to Thomas D. from comment #124)
> > 3) "Local Address Books" top level tree entry (because it's so powerful)
> and if possible

I just noticed in last try-build, contrary to what I claimed before, that "All Address Books" does NOT yet search LDAP ABs. So it's really a misnamed "Local Address Books" node, with some LDAP cuckoo's eggs which don't belong into that part of the tree. Strange, I really thought I had tested that and LDAP results had occured between the local results. I also keep getting crashes with that try build.

> > 4) "Remote Address Books" top level tree entry (for symmetry, and also
> > allowing powerful cross-"Remote Address Books" filter/search)
> 
> 2. we are 2 weeks before uplift with the string freeze and this could make
> it harder to land before.
> 
> I propose, Thomas, you file a new bug for your proposal and make a patch
> with the needed strings (with consulting Suyash) to land before the uplift
> and Suyash can, when he has time, finish it in the next cycle and then land
> and uplift to Aurora. If this patch misses TB 38, then we have already a
> basic functionality, which already satisfy all users without LDAP, with the
> patch from this bug.

Iow, Richard suggests to morph this bug into
3) "Enable search in all *local* ABs" (realized by "Local Address Books" node)
4) + (optionally) Enable search in all *LDAP* ABs" (realized by "Remote Address Books" node)
and do the full-global stuff (unified global search for Local + LDAP) in another bug.
I think that's wise given the time frames.

However, I'm not doing patches atm, so somebody else would have to land those strings:

Strings required for the UX-proposal 1 of attachment 8560983 [details]:

- "%1 contact" singular for resultsFoundSingular label
- "%1 contacts" for resultsFoundPlural label (forgot how to do those pairs properly, i think a single entry having singular and plural forms is sufficient?)
- EmptyText for filter box: "Search %1" (with a comment explaining that: %1 will be the the name of an AB exactly as seen in main AB list; pls work around potential grammatical oddities in your local language as required using using colon, double quotes etc., e.g. 'Search: "%1"'). Languages like German require object case with verbs like "search", which can't be provided for custom ABs, so we have ["Dieser PC" durchsuchen] in Windows Explorer which is grammatically odd but the quotes provide some alleviation.
- tooltip for global search button: "Search all address books"
- "All Address Books" for the scope selector dropdown list (other strings are always identical with AB and should be taken from there)
- "Remote Address Books" as node label in AB tree if not done in this bug
- "Search in: " label for scope selector (with comment: This string will be followed by a dropdown list displaying the name of any one AB exactly as seen in the AB list; consider grammatical oddities resulting from that in your locale, because AB name might not be object case as expected.)
- "Please enter a search phrase in the search box above" label as hint in the empty contacts list pane

Did I forget anything?

Comment 128

[Magnus Melin [:mkmelin]]

Comment on attachment 8560983 [details]
UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for search results)

In a way I do like having the "Local address books" and "Remote Address Books" there. But, that in itself doesn't really solve the "I just want to find that contact, search everywhere" problem. 

(You're also opening up to some more issues to think about. We also support system address book on mac, and I believe there's some hidden pref to enable it on windows too. Those would just be one though, so you'd get a "System address book" entry somewhere.)

In essence, I think you're making it all harder than it needs to be. 

Was there any real downside to just
 * always have the search field search all +
 * show address book column (so you can sort by adressbook if you want the contacts for just one)

The addressbook quick filter bar could be nice in adddition too but that's out of scope here.

Comment 129

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7

Okay so here we try something different.
Instead of making some drastic changes, now, if the user selects an LDAP entry without having a search term, he'll be presented with a message saying that the entries will be visible only with a search term.

(In reply to Thomas D. from comment #111)
> 5) Editing properties of All-AB contacts from contacts side bar, change a
> name property, OK did not close dialogue (perhaps already fixed?)
Fixed now.

(In reply to Thomas D. from comment #113)
> 6) Have one contact named "foo" in any AB; open contacts side bar with "all
> abs", verify exactly one "foo" is shown. From main AB (!), edit properties
> of contact, change display name by adding "bar" so it's now "foobar".
> Surprisingly (different from 5), this works. BUT: Alt+Tab to go back to the
> composition, and without clicking anything, look at contents of contacts
> side bar. There's a ghost duplicate entry of "foobar"; deleting one of them
> will delete "foobar" contact entirely -> dataloss!
> 
> 7) I managed to get into a state where in main AB, All ABs was selected, and
> I could use "File > New > Mailing List", which brought up mailing list
> dialogue with blank AB name. Creating the list failed somehow. Next restart
> crashed. I did a lot of "open containing AB" around the time, but I wasn't
> able to reproduce. Maybe it's on a new profile, first time opening of menu
> only, or some other odd condition.
> 
> 8) There are a number of UX problems around "Open containing AB"
> 8a) Have "John" and "Jane" in PAB. In main AB view, select "John"(!) in PAB.
> Keep window open. From contacts side bar, "all abs", do "Open containing AB"
> on "Jane"(!). main AB view is activated, but we wrongly preserve the old
> selection on "John". Confusing and error-prone, dataloss dangers.
> 
> For "open containing AB" command, if only one contact is selected when
> calling context menu, that same contact should be selected in the source AB.
> If it's not possible, at least clear old selection from that AB (set index
> to -1 or something).
> 
> IMO there's no reason to ever disable "Open containing AB" command from
> *contacts side bar*:
> 8b) for multiple selected contacts from *same* AB, open the AB and blur
> selection (or even try to select the same contacts). this currently fails
> and will focus AB window, but not select the correct AB.
> 8c) for multiple selected contacts from *different* ABs, "Open containing
> AB" is currently enabled but does not select any AB. Instead of disabling,
> we should just open "All ABs view" with no selection (or select the same
> selected contacts if we can). It'll be a consistent and convenient shortcut
> go get into main AB view regardless of which contact(s) are selected.
> 
> If we are in a hurry, we could postpone the details to another bug and for
> now make it so that "Open containing AB" is only available for single
> contact selection.
> 
> 9) In main AB, would like to see "Open containing AB" contextual action on
> "All ABs" search result contacts, working as described in 8). For
> ux-efficiency, we should never force the user to manually research and
> reselect elements which are already right in front of his eyes, and their
> whereabouts known.
> 
> 10) "Advanced AB search" dialogue does not have a "Address book" column for
> "all ABs" results (or any results as we postulated, even for known single
> ABs), and needs a button for "Open containing AB". Can be done in a new bug.
> There's another known bug that these results don't have a context menu.
6) didn't happen for me and the rename worked perfectly fine.
7) Unable to reproduce.
8-10) Please file bugs for these, they don't require string changes except 10, will do them there.

(In reply to Magnus Melin from comment #116)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #109)
> 
> > > This also needs some basic tests.
> > Oh ya, tests. So, what do you want to be tested? I didn't find anything
> > special
> > being offered with this new entry.
> 
> Would be nice with something to show that it finds contacts from more than
> one ab.

Will add the test soon. Please review this so that *if* there still cycles needed, they can be done before string freeze.

Thanks.

Comment 130

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #127)
> I just noticed in last try-build, contrary to what I claimed before, that
> "All Address Books" does NOT yet search LDAP ABs. So it's really a misnamed
> "Local Address Books" node, with some LDAP cuckoo's eggs which don't belong
> into that part of the tree. Strange, I really thought I had tested that and
> LDAP results had occured between the local results. I also keep getting
> crashes with that try build.

Oh and I forgot to mention. LDAP search works now. Try it!
Crash doesn't happen for me so I assume its not my patch.

Thanks.

Comment 131

[:aceman]

Comment on attachment 8564799 [details] [diff] [review]
Patch v7

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

Works great for me, thanks!

Some nits from testing:
1. When clicking the All ABs row in the AB window, the status bar shows the correct total number of cards, but wrongs name of addressbook. It picks up one of the ABs, not the "All Addressbooks" name. It is also visible in the screenshot you sent. The Properties button shows a non-writable dialog with the name of "All ABs". That seems fine. Other buttons are not enabled, except Write, which opens compose window with no recipients. That seems correct.
2. Now when clicking one card in the results list:
2a) Is Delete button intentionally disabled? Why?
2b) Properties, Write, IM seem to work fine. But "New list" gets confused. It opens the dialog but does not know into which AB it should create the list. The picker is uninitialized. Can the UX guys decide whether to prefill the first local AB there? Or just disable the button? "New contact" is disabled too.
3. Clicking an LDAP AB shows the info message, centered. Great work. The UI guys should just decide if it is fine to be on a grey background, or whether it also should have white background as the normal result pane has. If it should appear as being part of the result data, or part of window chrome.

Otherwise, this patch is a great accomplishment. Please get it in :)

Comment 132

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #131)
> Comment on attachment 8564799 [details] [diff] [review]
> Patch v7
> 
> Review of attachment 8564799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works great for me, thanks!
> 
> Some nits from testing:
> 1. When clicking the All ABs row in the AB window, the status bar shows the
> correct total number of cards, but wrongs name of addressbook. It picks up
> one of the ABs, not the "All Addressbooks" name. It is also visible in the
> screenshot you sent. The Properties button shows a non-writable dialog with
> the name of "All ABs". That seems fine. Other buttons are not enabled,
> except Write, which opens compose window with no recipients. That seems
> correct.

Pls confirm if default identity is used, after trying to get all previous selections or whatever away from the default identity.

> 2. Now when clicking one card in the results list:
> 2a) Is Delete button intentionally disabled? Why?

Delete Button should be enabled for any type of single/multiple/cross-AB selection in results (but not for the "All Address Books" node itself). It would be nice to do that here at least for single selections because duplicate removal is one of our usecases here (we could do multiple/cross-AB selections in another bug).

> 2b) Properties, Write, IM seem to work fine. But "New list" gets confused.
> It opens the dialog but does not know into which AB it should create the
> list. The picker is uninitialized. Can the UX guys decide whether to prefill
> the first local AB there? Or just disable the button? "New contact" is
> disabled too.

The scenario here was about enabling (or not) of the "New list" button when a single contact from "All ABs" search result is selected. Here's the UX decision: Pls just disable the button for that scenario.

To create a mailing list (per current mis-design) we need the name of an existing AB which will be the parent AB for the list. While in theory we could use the parent AB of the selected result entry (as displayed in "Address book" column), that connection is very vague; and for multiple cross-AB selections it wouldn't work anaway. Otoh, creating a new list in an uninteded AB can cause havoc to the content of that AB, because any cross-AB contacts added to the list will be copied to the parent AB of the list. So imo this is clearly a case of UX-error-prevention. There's no need nor particular benefit (per current mis-design) to create lists from global search results. The current use pattern of selecting an AB in the list, then creating a new list corresponds appropriately and sufficiently to the current mis-design.

> 3. Clicking an LDAP AB shows the info message, centered. Great work. The UI
> guys should just decide if it is fine to be on a grey background, or whether
> it also should have white background as the normal result pane has. If it
> should appear as being part of the result data, or part of window chrome.
> 
> Otherwise, this patch is a great accomplishment. Please get it in :)

I have requested a try-build from archaeopteryx.

From the description, I conclude that this patch continues to use the "All ABs" node with LDAP directories being children of that node. I also assume that when "All ABs" node is selected, and no search word has been entered yet, all entries from local ABs are shown, but no entries from LDAP ABs, and there's no message informing about the missing entries from LDAP. If that's correct, than I'm failing to see how this patch addresses the problem of misleading UX design where the parent/child relationship in the UI does not match the actual behaviour, until a search term is entered. My proposal in attachment 8560983 [details] solves that problem by "Local ABs" + "Remote ABs" collective nodes plus "Search All Addresses" toggle button next to filter input box. On the "Remote ABs" node, there'd be no results until you enter a search term, which is transparent and predictable. Showing some entries (from local ABs) but not all entries (remote ABs missing) for a global node is not transparent and predictable.
At least there should be an info bar for the "no search words" case of "All ABs" node, something like:
"Enter a search term above to include contacts from remote address books"

Having said which, I guess the lack of ANY type of global AB search is so glaring a design omission, that we could perhaps live with some temporary design flaws for another year in a trade-off to solve the major cross-AB search problem in time for TB38. Getting some version of this into TB38 is highly desirable.

Comment 133

[Thomas D. (:thomas8)]

Here's another issue seen in last try-build of comment 112:

1) Main AB view, select "PAB" (or any single AB)
2) deselect "Address Book" column
3) select "All ABs" and observe columns shown

Actual result

"Address Book" column is gone for global search results, too, after being disabled in any single AB (where the column might be optionally useful, but not generally required).

Expected result

"Address Book" column in global search results must be toggled independently from other views.
Removing the column for "PAB" must not remove it for "All ABs" view.
Enabling the column for "All ABs" view must not enable it for any other single AB. We can't force users to waste that column space when they don't want it; it must be optional for single ABs without losing the functionality for global search results.

Way forward (to speed up delivery):

If it's too hard to get independent column sets, pls restore the old behaviour where "address book" column is only available to global search results.

---

Another issue:
- I requested that the "Address book" column be moved from first column to the end of the default column set. Has this been addressed?

Comment 134

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #128)
> Comment on attachment 8560983 [details]
> UX-Proposal 1: Global AB search (Toggle button & AB Scope Selector Bar for
> search results)
> 
> In a way I do like having the "Local address books" and "Remote Address
> Books" there. But, that in itself doesn't really solve the "I just want to
> find that contact, search everywhere" problem. 

Yes, the "global search" is solved by the iconic "Search All Address Books" toggle button next to the filter input. So it's a small button which intuitively expands the scope from "per-AB filter" (or local/remote cross-AB filters) to "global search".

> (You're also opening up to some more issues to think about. We also support
> system address book on mac, and I believe there's some hidden pref to enable
> it on windows too. Those would just be one though, so you'd get a "System
> address book" entry somewhere.)

I guess we could address those edge cases in followup bugs.

> Was there any real downside to just
>  * always have the search field search all +
>  * show address book column (so you can sort by adressbook if you want the
> contacts for just one)

Yes, there are major downsides to that approach.
- Complete removal of current design of per-AB filter functionality, i.e. the possibility of searching a pre-selected set of contacts to exclude irrelevant/distracting results from other ABs (which is so obviously useful that I'm unwilling to spell out the reasons in detail to avoid being blamed for rants), would require a new bug spelling out detailed reasons why (I don't see any at all). Why would people maintain dedicated ABs if all they ever want is flat global searches?
- Sorting a lot of irrelevant results by AB severely impedes options for managing the relevant results from the single AB user actually wanted to search:
  - irrelevant/distracting results from unwanted ABs clutter view (on what grounds should TB force me to include results from ABs which I know aren't relevant in the first place?)
  - requirement to sort by AB prevents sorting/managing/acting on relevant results by any other category like recipient's name or emmail

> The addressbook quick filter bar could be nice in adddition too but that's
> out of scope here.

Pls use correct terminology to avoid misrepresentations.
The current (pre-patch) design has a quick filter box which searches whichever limited AB scope is selected in the AB list on the left.

My proposal is clear that the "AB scope selector bar" appearing at the top of search results after entering search words is OPTIONAL, but useful in two ways:
- clear visual indication that there's an active filter
- showing the current scope of that filter
- enabling powerful usecases of changing the filter scope "on-the-fly" to expand/reduce scope or iterate other ABs with same filter

Comment 135

[:aceman]

(In reply to Thomas D. from comment #132)
> > Some nits from testing:
> > 1. When clicking the All ABs row in the AB window, the status bar shows the
> > correct total number of cards, but wrongs name of addressbook. It picks up
> > one of the ABs, not the "All Addressbooks" name. It is also visible in the
> > screenshot you sent. The Properties button shows a non-writable dialog with
> > the name of "All ABs". That seems fine. Other buttons are not enabled,
> > except Write, which opens compose window with no recipients. That seems
> > correct.
> 
> Pls confirm if default identity is used, after trying to get all previous
> selections or whatever away from the default identity.
I do not understand this. What does identity have to do with the display of the AB window?

> From the description, I conclude that this patch continues to use the "All
> ABs" node with LDAP directories being children of that node. I also assume
> that when "All ABs" node is selected, and no search word has been entered
> yet, all entries from local ABs are shown, but no entries from LDAP ABs, and
> there's no message informing about the missing entries from LDAP. If that's
> correct, than I'm failing to see how this patch addresses the problem of
> misleading UX design where the parent/child relationship in the UI does not
> match the actual behaviour, until a search term is entered. My proposal in
> attachment 8560983 [details] solves that problem by "Local ABs" + "Remote
> ABs" collective nodes plus "Search All Addresses" toggle button next to
> filter input box. On the "Remote ABs" node, there'd be no results until you
> enter a search term, which is transparent and predictable. Showing some
> entries (from local ABs) but not all entries (remote ABs missing) for a
> global node is not transparent and predictable.
> At least there should be an info bar for the "no search words" case of "All
> ABs" node, something like:
> "Enter a search term above to include contacts from remote address books"

I am sure now that the info message label is there, it can also be shown if the All ABs is selected and no search term is input. It just shouldn't be centered in this case and will say that the "remote (LDAP) addressbooks are empty because no search query was entered.".

Comment 136

[Richard Marti (:Paenglab)]

Comment on attachment 8564799 [details] [diff] [review]
Patch v7

First, it looks really promising with the message on LDAP ABs and I think this is the right way.

I'm clearing the ui-r because of the not hiding of this message when on a LDAP AB contacts are found and should be displayed.

Comment 137

[Magnus Melin [:mkmelin]]

Comment on attachment 8564799 [details] [diff] [review]
Patch v7

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

You added the message for when an ldap ab is selected, but that does not resolve the original problem: that when you select all you'd expect *all* addresses to be shown since it is for the other abs.

Other issues;

- selected All abs: Status bar shows "Totalt contacts in *Collected Addresses*: 35"
- for ldap pane it never shows addresses, only the "this address book shows contact only after a search"

Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsITreeSelection.select]
Source File: chrome://messenger/content/addressbook/abResultsPane.js
Line: 478

- address book is not a selectable column (forced on)
- export does nothing useful while "all abs" is selected
- not critical but the order is now odd. I get 
- Find -> Search addresses - still can't select All. There's just a broken dropdown

Odd ordering:

all
 - personal
 - another one
 - ldap1
 - ldap2
 - collected

::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
@@ +2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY addressbookWindow.title                        "Address Book">
> +<!ENTITY blankResultsPaneMessage.label                  "This address book shows contacts only after a search">

"Search to find contacts in this address book" maybe?

Comment 138

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.2

While you are at it, please have a look at this as well :)

Thanks.

Comment 139

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #131)
> Comment on attachment 8564799 [details] [diff] [review]
> Patch v7
> Some nits from testing:
> 1. When clicking the All ABs row in the AB window, the status bar shows the
> correct total number of cards, but wrongs name of addressbook. It picks up
> one of the ABs, not the "All Addressbooks" name. It is also visible in the
> screenshot you sent. The Properties button shows a non-writable dialog with
> the name of "All ABs". That seems fine. Other buttons are not enabled,
> except Write, which opens compose window with no recipients. That seems
> correct.
Fixed.

> 2. Now when clicking one card in the results list:
> 2a) Is Delete button intentionally disabled? Why?
Works fine for me. For both, single, multiple (cross AB) selections and deletions.

> 2b) Properties, Write, IM seem to work fine. But "New list" gets confused.
> It opens the dialog but does not know into which AB it should create the
> list. The picker is uninitialized. Can the UX guys decide whether to prefill
> the first local AB there? Or just disable the button? "New contact" is
> disabled too.
Disabled.
> 3. Clicking an LDAP AB shows the info message, centered. Great work. The UI
> guys should just decide if it is fine to be on a grey background, or whether
> it also should have white background as the normal result pane has. If it
> should appear as being part of the result data, or part of window chrome.
Needs input from :Paenglab.

(In reply to Thomas D. from comment #132)
> > Comment on attachment 8564799 [details] [diff] [review]
> > Patch v7

<Trimmed suggestions are implemented in the patch.>

> From the description, I conclude that this patch continues to use the "All
> ABs" node with LDAP directories being children of that node. I also assume
> that when "All ABs" node is selected, and no search word has been entered
> yet, all entries from local ABs are shown, but no entries from LDAP ABs, and
> there's no message informing about the missing entries from LDAP.
We have the message now.
> If that's correct, than I'm failing to see how this patch addresses the problem of
> misleading UX design where the parent/child relationship in the UI does not
> match the actual behaviour, until a search term is entered.
> At least there should be an info bar for the "no search words" case of "All
> ABs" node, something like:
> "Enter a search term above to include contacts from remote address books"
Ya, something like this is implemented now.

(In reply to Thomas D. from comment #133)
> Here's another issue seen in last try-build of comment 112:
> 
> 1) Main AB view, select "PAB" (or any single AB)
> 2) deselect "Address Book" column
> 3) select "All ABs" and observe columns shown
> 
> Actual result
> 
> "Address Book" column is gone for global search results, too, after being
> disabled in any single AB (where the column might be optionally useful, but
> not generally required).
> 
> Expected result
> 
> "Address Book" column in global search results must be toggled independently
> from other views.
No. It was implemented this way but it didn't pass the review and a few bug reports
want it to be present in all ABs. Also, it seems confusing/annoying that you just hid the AB column
and it again shows up.
> Way forward (to speed up delivery):
> 
> If it's too hard to get independent column sets, pls restore the old
> behaviour where "address book" column is only available to global search
> results.
It isn't hard its just confusing. If you still think this is necessary, please file
a follow up bug.
> ---
> 
> Another issue:
> - I requested that the "Address book" column be moved from first column to
> the end of the default column set. Has this been addressed?
Yes. Sorry I didn't mention it with my patch.

(In reply to Magnus Melin from comment #137)
> Comment on attachment 8564799 [details] [diff] [review]
> Patch v7
> You added the message for when an ldap ab is selected, but that does not
> resolve the original problem: that when you select all you'd expect *all*
> addresses to be shown since it is for the other abs.
Fixed in a sense. Has ui-r+ from Paenglab. Please see the new patch. I think
its better to just inform the user of the behavior so that he does not try to predict
and figure out what's happening and sometimes, assume its a bug, rather, he is made
sure that this is the expected behavior.
> 
> Other issues;
> 
> - selected All abs: Status bar shows "Totalt contacts in *Collected
> Addresses*: 35"
Fixed.
> - for ldap pane it never shows addresses, only the "this address book shows
> contact only after a search"
My mistake, fixed.
> 
> Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsITreeSelection.select]
> Source File: chrome://messenger/content/addressbook/abResultsPane.js
> Line: 478
Fixed.
> 
> - address book is not a selectable column (forced on)
What do you mean by "selectable" ? I can sort according to it, move it left and right. What else is needed?
> - export does nothing useful while "all abs" is selected
Ya.. this doesn't work. Can we please fix this in a followup? I don't think there'll be any string changes in it. Moreover, it will not be tough as well. I'll do it soon.
> - not critical but the order is now odd. I get 
> - Find -> Search addresses - still can't select All. There's just a broken
> dropdown
Ya, will fix this as well in a followup.

> Odd ordering:
> 
> all
>  - personal
>  - another one
>  - ldap1
>  - ldap2
>  - collected
Tell me how to arrange this, I'll do it.

> 
> ::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
> @@ +2,5 @@
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <!ENTITY addressbookWindow.title                        "Address Book">
> > +<!ENTITY blankResultsPaneMessage.label                  "This address book shows contacts only after a search">
> 
> "Search to find contacts in this address book" maybe?
We have another string now. Please let me know if you have a suggestion for that as well.
We also need input from Paenglab on this.

Thanks.

Comment 140

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #139)
> > You added the message for when an ldap ab is selected, but that does not
> > resolve the original problem: that when you select all you'd expect *all*
> > addresses to be shown since it is for the other abs.
> Fixed in a sense. Has ui-r+ from Paenglab. Please see the new patch. I think
> its better to just inform the user of the behavior so that he does not try
> to predict
> and figure out what's happening and sometimes, assume its a bug, rather, he
> is made
> sure that this is the expected behavior.

But you do not inform about it in the case where it matters. You only show the message when the user selects an ldap ab, NOT when all is selected. (unless something changed with the last past, no time to try it out anymore today). And that's the problem. It's not easy to display that info in a reasonable way.

> Tell me how to arrange this, I'll do it.

Like it used to be.

Comment 141

[Richard Marti (:Paenglab)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #139)
> (In reply to :aceman from comment #131)
> > Comment on attachment 8564799 [details] [diff] [review]
> > Patch v7
> > 3. Clicking an LDAP AB shows the info message, centered. Great work. The UI
> > guys should just decide if it is fine to be on a grey background, or whether
> > it also should have white background as the normal result pane has. If it
> > should appear as being part of the result data, or part of window chrome.
> Needs input from :Paenglab.

Yes, this needs a change but is easily doable through CSS. Can be done in a followup bug.

> (In reply to Magnus Melin from comment #137)
> > Odd ordering:
> > 
> > all
> >  - personal
> >  - another one
> >  - ldap1
> >  - ldap2
> >  - collected
> Tell me how to arrange this, I'll do it.



> > ::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
> > @@ +2,5 @@
> > >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> > >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > >  
> > >  <!ENTITY addressbookWindow.title                        "Address Book">
> > > +<!ENTITY blankResultsPaneMessage.label                  "This address book shows contacts only after a search">
> > 
> > "Search to find contacts in this address book" maybe?
> We have another string now. Please let me know if you have a suggestion for
> that as well.
> We also need input from Paenglab on this.

I think Suyash's sentence, in agreement with aleth and me, is more descriptive why it's empty and how to fill it.

The other sentence when all ABs is selected is: "Contacts from remote address books are not shown until you search"

Comment 142

[Richard Marti (:Paenglab)]

> (In reply to Magnus Melin from comment #137)
> > Odd ordering:
> > 
> > all
> >  - personal
> >  - another one
> >  - ldap1
> >  - ldap2
> >  - collected
> Tell me how to arrange this, I'll do it.

I checked in a TB without the patch and it has the same ordering.

Comment 143

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #139)
> (In reply to Thomas D. from comment #133)
> > Here's another issue seen in last try-build of comment 112:
> > 
> > 1) Main AB view, select "PAB" (or any single AB)
> > 2) deselect "Address Book" column
> > 3) select "All ABs" and observe columns shown
> > 
> > Actual result
> > 
> > "Address Book" column is gone for global search results, too, after being
> > disabled in any single AB (where the column might be optionally useful, but
> > not generally required).
> > 
> > Expected result
> > 
> > "Address Book" column in global search results must be toggled independently
> > from other views.
> No. It was implemented this way but it didn't pass the review and a few bug
> reports
> want it to be present in all ABs.

No, it was never implemented exactly the way I suggested, and nobody ever suggested hiding/showing the column in single-AB views should also hide/show it for global search results (where the hiding case is the bigger ux problem). Pls list the bug reports.

> Also, it seems confusing/annoying that you
> just hid the AB column
> and it again shows up.

I'm yet to see the latest patch in action, but let's be clear about the "Address book" column:

- "Address book" column is important information for all *cross-AB* views or search results (All ABs, Local ABs, Remote ABs etc.), because results can come from various sources so it matters to know the source.
  -> for search results, it must be *shown* by default; but if user decides he doesn't need it in search results, he should still be free to disable it *for cross-AB search results*

- "Address book" column is optional/redundant information for all *single-AB* views or searches because the source Address book can quite easily be seen from the AB selected in the list of ABs, and all contacts in such views always have the same source.
  -> for single ABs, it must be *hidden* by default because for many/most users it'll just waste space; but we could (imo should) still allow users to show that column if they so wish because they might find it useful anyway (clearer in-situ indication of source AB in multi-AB environments, to avoid errors in fast address management).

So definitely the sane defaults are:
- Show "Address book" column for *cross*-AB views / search results (but allow hiding the column anyway)
- Hide "Address book" column for *single*-AB views / search results (but allow showing the column anyway)

So definitely we cannot/should not force user to see redundant "Address book" column in all of his single-AB views just to preserve it for cross-AB views; that would be really odd and our users hate that sort of wasting their screen real estate without reason.
So that requires that we cannot handle hiding/showing of that column the same way for single vs. cross-AB views. Hiding the column for single-AB views does not imply I want to hide it for cross-AB views / search results and vice versa, showing the column for cross-AB views does not mean I want to see it on single ABs too, where the source is always the same for all contacts seen, and known.

> > Way forward (to speed up delivery):
> > 
> > If it's too hard to get independent column sets, pls restore the old
> > behaviour where "address book" column is only available to global search
> > results.
> It isn't hard its just confusing. If you still think this is necessary,
> please file
> a follow up bug.

Please accept that cross-AB views and single-AB views are different animals with different needs, so there's nothing confusing about showing the "Address book" where it's needed but hiding it where it's generally not needed because it's redundant information. (But, if we can do it without too much effort, we could still allow deliberate redundancy if the user so wishes as long as we don't force that upon everyone by default).

For comparison wrt ux-consistency, please look at "Location" column in
- main message list
VS
- global message search results, list-style ("Open as list")

User can freely hide/show location column in both types of views independently of each other, and the choices will be preserved.
That's exactly what I suggested the different types of AB views, which are exactly the same scenario, so we should be ux-consistent.

Whether it's done in a new bug or here I don't care (to land the feature and strings and do all the rest later), but a corrected version (at least allowing to show the column for cross-AB views only) should land with the first release of the feature.

Comment 144

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #143)
> Whether it's done in a new bug or here I don't care (to land the feature and
> strings and do all the rest later), but a corrected version (at least
> allowing to show the column for cross-AB views only) should land with the
> first release of the feature.

Okay so what I suggest is, please file follow up bug(s) for all such polishing issues and I'll
fix them. But let's just get this in before. Toggling AB column globally (for all ABs at once)
isn't so bad that we wait for it here. Plus, I can't think of any string changes it'll involve.
So, its pure and can be landed whenever we want.

Oh and do you need a try build for the latest patch? If yes, please let me know.

Thanks.

Comment 145

[Thomas D. (:thomas8)]

> Oh and do you need a try build for the latest patch? If yes, please let me
> know.

I had asked Archaeopteryx yesterday 16th so I'm hoping to get one.

Comment 146

[Richard Marti (:Paenglab)]

Comment on attachment 8565132 [details] [diff] [review]
Patch v7.2

I give ui-r+ to let it land in TB 38. Please file the remaining issues in followup bugs (but be sure they will be uplifted to TB 38 ESR then).

Comment 147

[Magnus Melin [:mkmelin]]

Comment on attachment 8565132 [details] [diff] [review]
Patch v7.2

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

::: mail/components/addrbook/content/abContactsPanel.js
@@ +88,5 @@
>    SetAbView(GetSelectedDirectory());
>  }
>  
> +function updateABColumnState(aMutations)
> +{

please just inline this function where it's used

@@ +125,5 @@
>  
>    parent.addEventListener("compose-window-close", AbPanelOnComposerClose, true);
>    parent.addEventListener("compose-window-reopen", AbPanelOnComposerReOpen, true);
> +
> +  let config = { attributes: true, childList: true };

inline this too

::: mail/components/addrbook/content/abContactsPanel.xul
@@ +54,5 @@
>                accesskey="&addrBookCardProperties.accesskey;"
>                command="cmd_properties"/>
> +    <menuitem label="&openContainingAB.label;"
> +              accesskey="&openContainingAB.accesskey;"
> +              command="cmd_openContainingAB"/>

I think this is a very odd thing have a context menu for.

@@ +100,5 @@
>                   persist="hidden ordinal width sortDirection" flex="1" label="&GeneratedName.label;" primary="true"/>
>          <splitter class="tree-splitter"/>
> +        <treecol id="addrbook"
> +                 persist="hidden ordinal width sortDirection" hidden="true"
> +                 flex="1" label="&Addrbook.label;"/>

In the contacts sidebar the address book column needs to be hidden by default. There's barely enough room for name there.

::: mail/components/addrbook/content/abTrees.js
@@ +72,5 @@
> +}
> +
> +// A boolean variable that is true if there is at least
> +// one remote AB under "All Address Books".
> +var gHasRemoteAB = false;

having this global and the member seems hacky. can't you just set it directly on the view once you find out its value?

::: mail/locales/en-US/chrome/messenger/addressbook/abContactsPanel.dtd
@@ +11,5 @@
>  <!ENTITY deleteAddrBookCard.label           "Delete">
>  <!ENTITY deleteAddrBookCard.accesskey       "D">
>  <!ENTITY addrBookCardProperties.label       "Properties">
>  <!ENTITY addrBookCardProperties.accesskey   "P">
> +<!ENTITY openContainingAB.label             "Open Containing AddressBook">

Address Book (with a space!)
But then again, I don't think this menu is called for, see below.

::: mail/themes/linux/mail/addrbook/addressbook.css
@@ +142,5 @@
>  toolbar[iconsize="small"] #button-abdelete[disabled] {
>    list-style-image: url("moz-icon://stock/gtk-delete?size=menu&state=disabled");
>  }
>  
> +#blankResultsPaneMessage {

as these are the same on all platforms, ideally they should be in mail/themes/shared no?

::: mailnews/addrbook/content/abDragDrop.js
@@ +282,3 @@
>            directory.dropCard(card, needToCopyCard);
> +
> +          // This if will qualify as true only if srcURI is "All ABs" and

This is true

::: mailnews/addrbook/content/abResultsPane.js
@@ +85,5 @@
> +  // inform the user of the empty results pane.
> +  if (aURI.indexOf("moz-abldapdirectory") != -1 &&
> +      aURI.indexOf("?") == -1) {
> +    let element = null;
> +    if ((element = document.getElementById("abResultsTree")) != null)

you don't need the != nulls here. (for all the cases)

::: mailnews/addrbook/src/nsAbView.cpp
@@ +201,4 @@
>  
> +  if (Substring(uri, 0, searchBegin).EqualsLiteral(kAllDirectoryRoot)) {
> +    mIsAllDirectoryRootView = true;
> +    // we have special request case to search all addressbooks, so we need

W

@@ +827,5 @@
> +  nsCString uri;
> +  aDir->GetURI(uri);
> +  return (uri.Find("moz-abldapdirectory") != kNotFound);
> +}
> +  

whitespace

::: mailnews/mailnews.js
@@ +369,5 @@
>  pref("mail.collect_email_address_incoming", false);
>  pref("mail.collect_email_address_newsgroup", false);
>  #endif
>  pref("mail.collect_email_address_outgoing", true);
> +pref("mail.show_ab_column_contactpanel", true);

but wait, you don't need this pref for anything! just use a global(ish) variable

Comment 148

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #147)
> Comment on attachment 8565132 [details] [diff] [review]
> Patch v7.2

Thanks for allowing us to proceed with this chagne.
> 
> ::: mail/components/addrbook/content/abContactsPanel.xul
> @@ +54,5 @@
> >                accesskey="&addrBookCardProperties.accesskey;"
> >                command="cmd_properties"/>
> > +    <menuitem label="&openContainingAB.label;"
> > +              accesskey="&openContainingAB.accesskey;"
> > +              command="cmd_openContainingAB"/>
> 
> I think this is a very odd thing have a context menu for.
So, how do we allow "open containing AB"? 

> 
> @@ +100,5 @@
> >                   persist="hidden ordinal width sortDirection" flex="1" label="&GeneratedName.label;" primary="true"/>
> >          <splitter class="tree-splitter"/>
> > +        <treecol id="addrbook"
> > +                 persist="hidden ordinal width sortDirection" hidden="true"
> > +                 flex="1" label="&Addrbook.label;"/>
> 
> In the contacts sidebar the address book column needs to be hidden by
> default. There's barely enough room for name there.
> 
> ::: mailnews/addrbook/content/abDragDrop.js
> @@ +282,3 @@
> >            directory.dropCard(card, needToCopyCard);
> > +
> > +          // This if will qualify as true only if srcURI is "All ABs" and
> 
> This is true
You mean to say this is always true? If yes, I'll remove the check then.
> 
> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +85,5 @@
> > +  // inform the user of the empty results pane.
> > +  if (aURI.indexOf("moz-abldapdirectory") != -1 &&
> > +      aURI.indexOf("?") == -1) {
> > +    let element = null;
> > +    if ((element = document.getElementById("abResultsTree")) != null)
> 
> you don't need the != nulls here. (for all the cases)
> 
I was getting document.getElementById("..")... is null errors in the error console.
> 
> ::: mailnews/mailnews.js
> @@ +369,5 @@
> >  pref("mail.collect_email_address_incoming", false);
> >  pref("mail.collect_email_address_newsgroup", false);
> >  #endif
> >  pref("mail.collect_email_address_outgoing", true);
> > +pref("mail.show_ab_column_contactpanel", true);
> 
> but wait, you don't need this pref for anything! just use a global(ish)
> variable
But how do we persist the behaviour across sessions then?
As, for contacts side bar, we intend to hide address book column permanently for all ABs
except "All ABs" entry. So, I think we need a pref.. 

Thanks.

Comment 149

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #148)
> (In reply to Magnus Melin from comment #147)
> > Comment on attachment 8565132 [details] [diff] [review]
> > Patch v7.2
> 
> Thanks for allowing us to proceed with this chagne.
> > 
> > ::: mail/components/addrbook/content/abContactsPanel.xul
> > @@ +54,5 @@
> > >                accesskey="&addrBookCardProperties.accesskey;"
> > >                command="cmd_properties"/>
> > > +    <menuitem label="&openContainingAB.label;"
> > > +              accesskey="&openContainingAB.accesskey;"
> > > +              command="cmd_openContainingAB"/>
> > 
> > I think this is a very odd thing have a context menu for.
> So, how do we allow "open containing AB"? 

I don't think we need to. You can see which ab it is if you enable the ab column. But i fail to see why anyone would like to do that there.

> 
> > 
> > @@ +100,5 @@
> > >                   persist="hidden ordinal width sortDirection" flex="1" label="&GeneratedName.label;" primary="true"/>
> > >          <splitter class="tree-splitter"/>
> > > +        <treecol id="addrbook"
> > > +                 persist="hidden ordinal width sortDirection" hidden="true"
> > > +                 flex="1" label="&Addrbook.label;"/>
> > 
> > In the contacts sidebar the address book column needs to be hidden by
> > default. There's barely enough room for name there.
> > 
> > ::: mailnews/addrbook/content/abDragDrop.js
> > @@ +282,3 @@
> > >            directory.dropCard(card, needToCopyCard);
> > > +
> > > +          // This if will qualify as true only if srcURI is "All ABs" and
> > 
> > This is true
> You mean to say this is always true? If yes, I'll remove the check then.

I meant to suggest changing the comment wording.

> > 
> > ::: mailnews/addrbook/content/abResultsPane.js
> > @@ +85,5 @@
> > > +  // inform the user of the empty results pane.
> > > +  if (aURI.indexOf("moz-abldapdirectory") != -1 &&
> > > +      aURI.indexOf("?") == -1) {
> > > +    let element = null;
> > > +    if ((element = document.getElementById("abResultsTree")) != null)
> > 
> > you don't need the != nulls here. (for all the cases)
> > 
> I was getting document.getElementById("..")... is null errors in the error
> console.

I don't think those are related to the != null check. YOu can just check if its truthy.

> As, for contacts side bar, we intend to hide address book column permanently
> for all ABs
> except "All ABs" entry. So, I think we need a pref.. 


Like a said, it's way too large to be shown (ever) by default in the sidebar.

Comment 150

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

There are green Try build generated by Suyash at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73f789a36d3a

Comment 151

[Onno Ekker [:nONoNonO UTC+1]]

I don't see any of the strings on comm-central yet and string freeze is happening very soon I guess. I'd hate to see this miss TB38 because of that...

Comment 152

[Suyash Agarwal (:sshagarwal)]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #151)
> I don't see any of the strings on comm-central yet and string freeze is
> happening very soon I guess. I'd hate to see this miss TB38 because of
> that...

Have faith in the developer.

Comment 153

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v8

Okay so I think I have addressed all the review comments.
Except that ui changes aren't reflected (italic text). I am unable to figure
out what's wrong so please just let me know that. Rest, I think everything
works as expected.

Thanks.

Comment 154

[Richard Marti (:Paenglab)]

Suyash, you need to add to all addressbook.css this line:

@import url("chrome://messenger/skin/shared/addressbook.css");

directly after the @import url("chrome://messenger/skin/");. But don't forget to add it also in addressbook-aero.css.

Comment 155

[Richard Marti (:Paenglab)]

Comment on attachment 8567304 [details] [diff] [review]
Patch v8

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

ui-r+ with comment 154 addressed. And when you are on it, please fix also my comments here.

The background color of the blankResultsPaneMessageBox can be done in a separate bug.

::: mail/themes/shared/mail/addressbook.css
@@ +3,5 @@
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ===== addressbook.css ================================================
> + *   == Styles for the main Address Book window.
> + *     ======================================================================= */

It's a weird indentation, but his part isn't needed, you can remove it.

@@ +5,5 @@
> +/* ===== addressbook.css ================================================
> + *   == Styles for the main Address Book window.
> + *     ======================================================================= */
> +
> +@import url("chrome://messenger/skin/");

This import can be removed, the iport in the files in the themes is enough.

@@ +8,5 @@
> +
> +@import url("chrome://messenger/skin/");
> +
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");

The xhtml namespace line can also be removed.

Comment 156

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v8.1

It works now! :) Thanks.

Comment 157

[Magnus Melin [:mkmelin]]

Comment on attachment 8567463 [details] [diff] [review]
Patch v8.1

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

Ok lets land this and file followups! r=mkmelin

::: mail/components/addrbook/content/abTrees.js
@@ +98,5 @@
>    },
>  
>    _children: null,
>    get children() {
> +    const Cc = Components.classes;

unused it seems?

@@ +156,1 @@
>      const Cc = Components.classes;

don't know why you changed this. I think the original order is more common

Comment 158

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Thanks! :)

Comment 159

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

Pushed as http://hg.mozilla.org/comm-central/rev/a65546d1b14e

Comment 160

[Suyash Agarwal (:sshagarwal)]

Attachment: Regression fix.

Please check this in.
This was deleted by mistake and disables "delete" functionality as of now.
The mozmill test failures are also fixed by this.

Thanks.

Comment 161

[Kent James (:rkent)]

Comment on attachment 8567537 [details] [diff] [review]
Regression fix.

Checked in https://hg.mozilla.org/comm-central/rev/b003a9bbb687

Comment 162

[Thomas D. (:thomas8)]

Thanks Suyash & everyone for working hard to get this in before the string-freeze. Good teamwork under time pressure. I've filed most of the immediate followup bugs to polish the rough edges (some of them still need strings, let's be quick!). This looks like one of the coolest features TB 38 will ship with. Adding a lot of search power and other powerful use cases to the current AB layout. Let's do more of that sort, let's be brave and just tackle those age-old shortcomings/needs head-on, for the benefit and happiness of our users.

Comment 163

[Thomas D. (:thomas8)]

Magnus, are we able to delete contacts from LDAP ABs?
- at all?
- for public ABs of public institutions (certainly not)
So I think "Delete" should be disabled IF the selection contains "remote contacts" (at least when they can't be deleted)?

Comment 164

[Thomas D. (:thomas8)]

Who can test this new feature for MAC System AB, or windows system AB if we actually support that?

Comment 165

[Magnus Melin [:mkmelin]]

No we don't have support for writable ldap.

Comment 166

[Suyash Agarwal (:sshagarwal)]

Attachment: Typo fix

A typo was made by me (sorry it slipped in).
This is the fix.

Thanks.

Comment 167

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

Comment on attachment 8569802 [details] [diff] [review]
Typo fix

Looks good to me. Forwarding to Magnus for formal review.

Comment 168

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

Pushed https://hg.mozilla.org/comm-central/rev/a93f0b6de7fc
Still needs to land on comm-aurora.

Comment 169

[Kent James (:rkent)]

Comment on attachment 8569802 [details] [diff] [review]
Typo fix

[Triage Comment]

needed typo correction.

Comment 170

[Kent James (:rkent)]

Just for the record, the typo fix gets lost in searches because it was placed as a second patch in a closed bug that was resolved in a previous gecko. I'll land this, but in the future please use new bugs for these types of issues.

Comment 171

[Kent James (:rkent)]

Comment on attachment 8569802 [details] [diff] [review]
Typo fix

Pushed http://hg.mozilla.org/releases/comm-aurora/rev/6cf68063bffb

Comment 172

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #81)
> 3) Further points (from Comment 60), either here or new bugs:
> a) For contact card properties, now need a display of parent AB

Bug 553647 - "Edit Contact" properties dialog: Implement dropdown field to show and change the name of containing address book (as in Inline Contact Editor from yellow star)

Comment 173

[Wayne Mery (:wsmwk)]

In Moztrap - https://moztrap.mozilla.org/manage/case/16264/

However:
a) I'm not sure I completely described it.
b) Problem - I tested in a new profile, with all addressbooks selected my search results did not have an Address Book column to show the name of the found contacts' addrees book. I thought that was part of the implementation?

Comment 174

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #173)
> In Moztrap - https://moztrap.mozilla.org/manage/case/16264/
> 
> However:
> a) I'm not sure I completely described it.

Incomplete/incorrect in #1:

> 1. Open Tools, Address Books, click the column picker to the right of the column names and enable
> "Address Book" column, type to find contacts which match on a searchable field.

As we might remember the last used AB soon, pls mention to select "All Address Books"

> --> Results should list matches in all address books (except ldap), and in the Address Book column for
> each match provides the address book name for the contact.

*After* typing searchword, LDAP results *are* included ("except ldap" is wrong as it stands). Only *without* search word in "All ABs" or LDAP AB, that's when there are no Ldap results. plus ldap results will be limited in number according to ldap ab properties.

> b) Problem - I tested in a new profile, with all addressbooks selected my
> search results did not have an Address Book column to show the name of the
> found contacts' addrees book. I thought that was part of the implementation?

Well, we currently have bug that showing/hiding the AB column for "all abs" will also show/hide it for every other AB. For single ABs, showing "AB" column is usually undesirable because AB is known (but we should allow user to show it if he so wishes).
Notwithstanding, even with that bug it's probably better if we would show that column for now after first install...

In the long run, we want bug 1167798.

Comment 175

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #174)

> Well, we currently have bug that showing/hiding the AB column for "all abs"
> will also show/hide it for every other AB. 

bug 1133652

> In the long run, we want bug 1167798.

Comment 176

[Suyash Agarwal (:sshagarwal)]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #173)
> In Moztrap - https://moztrap.mozilla.org/manage/case/16264/
> 
> However:
> a) I'm not sure I completely described it.
> b) Problem - I tested in a new profile, with all addressbooks selected my
> search results did not have an Address Book column to show the name of the
> found contacts' addrees book. I thought that was part of the implementation?

Yes, it is part of the implementation and I see the Address Books column in all the cases viz., isting all the contacts, search results, ldap search, single AB search.

So, the only thing I can think of is, is it possible that in your new profile, AB column wasn't selected in the column picker? Because it is by default turned off IIRC.

Comment 177

[Wayne Mery (:wsmwk)]

(In reply to Suyash Agarwal (:sshagarwal) (away till 19th of May) from comment #176)
> Because it is by default turned off IIRC.
Apparently my memory is faultly, I thought it was enabled. I think we should enable it by default, and if necessary deal with the ugliness of it being enabled in single ABs separately


(In reply to Thomas D. from comment #174)
> (In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment
> #173)
> > In Moztrap - https://moztrap.mozilla.org/manage/case/16264/
> > 
> > However:
> > a) I'm not sure I completely described it.
> 
> Incomplete/incorrect in #1:
> 
> > 1. Open Tools, Address Books, click the column picker to the right of the column names and enable
> > "Address Book" column, type to find contacts which match on a searchable field.
> 
> As we might remember the last used AB soon, pls mention to select "All
> Address Books"

I'll deal with that issue when the time comes.  Thanks for all the feedback. I've revised https://moztrap.mozilla.org/manage/case/16264/

Comment 178

[Thomas D. (:thomas8)]

Updating and rectifying dependencies, which unfortunately generates some bugmail...