Thomas D. (:thomas8) Reporter
	Description • 8 years ago

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

STR

Open Main AB window
Observe initial default view

Actual Result

Forced default view of "All Address Books" (cannot be changed by user)
Certainly useful for some, but not all types of users and scenarios

Expected Result

Allow user to set any specific AB or "All ABs" as initial default view

	Thomas D. (:thomas8) Reporter
	Comment 1 • 8 years ago

Judging from user feedback in Bug 1177706, and more negative feedback from user forums (mentioned by Anje's in bug 1177706, comment 17), this should be fixed asap, as it has potential for annoyance each time users open up the main AB window.

I'll propose a simple and straightforward implementation to address this issue, notwithstanding later more advanced solutions.

	Thomas D. (:thomas8) Reporter
	Comment 2 • 8 years ago

P1: Proposed minimal implementation: Checkbox in contextual "Address Book Properties" dialogue

Right-click on "All ABs" or "Any AB" in main AB window > Properties
Add a simple checkbox to "Address book properties" dialogue:

+--------------------------------------+
| Address Book Properties              |
+--------------------------------------+
|                                      |
| Address Book Name: [Any AB Name    ] |
|                                      |
| [ ] Show as default for initial view |
|                                      |
+--------------------------------------+

When checked, this address book will be shown by default the next time the AB is started, until another default is set. For all other ABs, the checkbox will appear unchecked.

Technically, a single string pref:
mail.addr_book.view.defaultURI = moz-abdirectory://

Then, via checkbox UI, user can change it to another AB (e.g. "Personal AB") and we fill the pref with the corresponding URI (e.g. moz-abmdbdirectory://abook.mab).

As simple as that. I think that's sufficient for a minimal solution.
And it doesn't hinder more advanced solutions in any way, e.g. adding a global pref UI with or without "Last used address book" option (which would just need another single boolean pref, mail.addr_book.view.rememberLastUsedURI=true/false, and if true, fill defaultURI with the last used AB URI upon closing of AB window).

Checkbox caption is subject to discussion, but I'm quite happy with the above.
I tried the following
[ ] Initially show this address book
[ ] Initial default view
[ ] Show as initial default
[ ] Default Address Book for initial view   (that's pretty good, too, but not ideal for "All Address Books")

Aceman, what do you think?

	JP
	Comment 3 • 8 years ago

Agree with Thomas D. solution above.
Definitely well thought out.

Look forward to the implementation.

JP

	Mitch in Oakland
	Comment 4 • 8 years ago

How would this work on a Mac? ("Right click"? Duh! Where do I find the "address book properties" dialogue?)

	Thomas D. (:thomas8) Reporter
	Comment 5 • 8 years ago

(In reply to Mitch in Oakland from comment #4)
> How would this work on a Mac? ("Right click"? Duh! Where do I find the
> "address book properties" dialogue?)

Select desired AB, then from main menu, Edit > Properties. Does that exist on MAC?

	Mitch in Oakland
	Comment 6 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #5)

> Select desired AB, then from main menu, Edit > Properties. Does that exist
> on MAC?

Interesting question. The answer is, "Yes... in a way."

When first opening an address book window -- defaulting to "All Address Books" -- "Properties" appears on the drop-down, but it's grayed-out (thus, unavailable). There's also a "Properties" button on the toolbar immediately above the window, but it, too, is grayed-out.

After choosing another address book (e.g., "Personal Address Book"), "Properties" becomes available (appearing in black) on the "Edit" drop-down. (So, too, does the "Properties" button on the toolbar).

If I switch back to "All Address Books" while the window remains open, "Properties" remains available (in black). (This remains true even if I send the Address Book window to the back and re-select it, returning it to the front.) 

However, if I CLOSE the Address Book window and re-open it (even without quitting and re-launching TB), it reverts to the original configuration (defaulting to "All Address Books," with "Properties" grayed-out). If I quit and re-launch TB, the Address Book (which then needs to be re-opened) ALWAYS defaults to "All Address Books" (with "Properties" unavailable), regardless of which Address Book had been selected in the previous session.

FWIW, while the Address Book window is open and selected (in front) -- regardless of which Address Book is in use -- "Use OSX Address Book" appears on the "File" drop-down. Selecting "Use Mac OSX Address Book" (at which point a check mark appears adjacent to the menu item) imports the Address Book from the Apple "Mail" application and adds "Mac OSX Address Book" to the list of available address books in the sidebar, and selects it. If the Address Book window is closed (or if TB is quit and re-launched) while "Use Mac OSX Address Book" remains checked, "Mac OSX Address Book" remains on the list but the new window defaults to "All Address Books" as usual. (If "Use Mac OSX Address Book" is manually un-checked [toggled back off], "Mac OSX Address Book" disappears from the list.) However, this feature seems to operate independently -- with the "Mac OSX Address Book" behaving like any other address book while the feature is active -- so I doubt that it would be negatively impacted by your solution.

It's probably worth noting that I'm still on Mac OS 10.6.8 (Snow Leopard), using TB 45.4.0. I had previously been on the TB Beta channel, but reverted back to TB 45 (to buy time) after the recent release of TB 49 Beta, which is incompatible with OS 10.6.8 (TB 49 won't launch). Over the next few months I'll update the OS, of course; I've resisted doing so thus far in order to avoid having to purchase the necessary (expensive, now often cloud-based) updates to other applications (e.g., Adobe Creative Suite, MS Office, Toast), along with the inevitable, time-consuming hassles involved in installing a new OS.

I'm not sure whether I'll be able to enjoy using your new feature on a maintenance update to TB 45, or whether I'll need to wait until I update the OS (and perhaps even switch back to TB Beta), but thank you in advance, either way. :-)

	Thomas D. (:thomas8) Reporter
	Comment 7 • 8 years ago

Users have demanded a way of defaulting the initial view to an address book other than All Address Books, and here it is - plain and simple, and contextual hence intuitive and easy to use without digging options.

Checking "Show as default when starting the main address book" will do just that. Unchecking the current default AB will remember the last used AB. Without a single option in options... ;)

I also want to convince Richard that "Address Book Properties" dialogue should have dynamic caption including the AB name, to be consistent with other dialogues, e.g. "Personal Address Book Properties", so now there's more than just the AB name on the AB Properties dialogue ;)

This is WIP/Proof of concept patch, so it's not complete yet.

To do:
1) I failed to select the current directory on the left, based on its URL, as I don't know the index in the tree. Please advise!
2) As part of that, we'll also need to hide the currently wrong info bar at the top about remote ABs even when these are not included (the hiding is easy).
3) Small regression: AB name should be selected when dialogue opens, for easy copy and paste (readonly names), otherwise easy rename.
4) The checkbox and same behaviour also needs to be implemented for LDAP dirs, which should be easy, just cloning the patch changes into the respective dialogue.
5) Remove console.logs

	Thomas D. (:thomas8) Reporter
	Comment 8 • 8 years ago

Comment on attachment 8805705 [details] [diff] [review]
WIP Patch V.1 - proof of concept

This patch is on top of my other patch in bug 1312262.

	Thomas D. (:thomas8) Reporter
	Comment 9 • 8 years ago

btw Richard, can you please set the background color of read-only AB names to the background color of the dialogue, so that there's some indication that the name is read-only. Currently that's not obvious from the UI at all.

	Richard Marti (:Paenglab)
	Comment 10 • 8 years ago

Comment on attachment 8805705 [details] [diff] [review]
WIP Patch V.1 - proof of concept

This is the way this can be done.

For the read-only fields, I'll file a independent bug as this affects already the actual build (didn't know this fields where sometimes read-only because I saw never a difference :) ).

	Thomas D. (:thomas8) Reporter
	Comment 11 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #1)
> Judging from user feedback in Bug 1177706, and more negative feedback from
> user forums (mentioned by Anje in bug 1177706, comment 17), this should be
> fixed asap, as it has potential for annoyance each time users open up the
> main AB window.

	Thomas D. (:thomas8) Reporter
	Comment 12 • 8 years ago

Wayne, can you please CC someone with knowledge about XUL trees or more specifically, the Directory Tree of main Address Book? I'm failing to select a directory on the left, based on its URL, under the assumption that iterating every tree item to check its URL until finding a match is NOT the right approach. I'm also failing to access the tree items using Dom Inspector, I only get to the outer tree element and don't see the children.

	Wayne Mery (:wsmwk)
	Comment 13 • 8 years ago

aceman (already NI) or magnus perhaps may know. Sorry I do not.

	Thomas D. (:thomas8) Reporter
	Comment 14 • 8 years ago

Richard, are you OK with the checkbox label?

[ ] Show as default when starting the main address book

I'm quite happy with that because it says exactly what we do, in natural language.
My feeling is that saying less will not say the right thing.

I've implemented the full logic (including the same checkbox for lists and ldap ABs) on my local Daily, but I'm not sure when I'll get round to making that into a patch.
I'm still unable to select a directory by URL in the tree, which is needed for this to work correctly; so I need help on that one.

I think we should land a strings-only patch first to not miss the string freeze.

	Thomas D. (:thomas8) Reporter
	Comment 15 • 8 years ago

Aceman, I'd appreciate a round of review on this... :) The missing piece is minor and clearly located, so the rest is ready for consumption.

Richard, enjoy! Please play with this and let me know if there's any odd or unexpected behaviour. In terms of UI, if we embrace the general idea as you already did, I think choices are limited so this should be fine.
For consistency and convenience, we also allow setting "Show as default..." on newly created items.

Gee! Lots of work for this little wanna-be-clever. It's much more clever now in that it allows user to make any item from the Directory Tree on the left the initial default view when starting up the main AB: his favorite AB, LDAP Directory, or even Mailing List. Removing the existing checkmark from the current default view item (so that there's no default item) gets you into "Remember last selected AB" mode, until you change your mind again and make any specific item the default.

I hope that I succeeded to duplicate all of my local changes into the patch - lots of manual work - any (semi-)automatical intelligent ways of doing that?

This should be fully functional, but unfortunately, there's still this very visible flaw of not selecting user's favorite item in the DirTree on the left (and clearing that LDAP caption, that's easy).

	Richard Marti (:Paenglab)
	Comment 16 • 8 years ago

Comment on attachment 8810203 [details] [diff] [review]
WIP Patch V.2 - fully functional, but missing DirTree Selection

This patch doesn't work as it should. When you set an AB as default and reopen the properties, there is no ticked checkbox. Only in address lists the checkbox is showing. But why give you this possibility to address lists? A address list shouldn't become a AB functionality. Only because it's in the tree is no reason.

Now the AB window remembers the last selected AB no matter what I try to select as default. Maybe we should only implement this functionality ;)

I somehow don't like the sentence: "Show as default when starting the main address book". It' is way too long. Maybe "Show as default Address Book" is enough. We don't need to take care for the contacts sidebar in this place.

I get also this errors:
Sun Nov 13 2016 20:01:13
Error: ERROR: failed to set status text:  [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 644"  data: no]
Source file: chrome://messenger/content/addressbook/addressbook.js
Line: 518
 ----------
Sun Nov 13 2016 20:01:19
Warning: ReferenceError: assignment to undeclared variable gShowAsDefault
Source file: chrome://messenger/content/addressbook/abAddressBookNameDialog.js
Line: 23
 ----------
Sun Nov 13 2016 20:01:19
Error: ReferenceError: showAsDefaultBox is not defined
Source file: chrome://messenger/content/addressbook/abAddressBookNameDialog.js
Line: 33
 ----------
Sun Nov 13 2016 20:01:22
Error: TypeError: gShowAsDefault is undefined
Source file: resource://app/modules/ABQueryUtils.jsm
Line: 166

	Thomas D. (:thomas8) Reporter
	Comment 17 • 8 years ago

(In reply to Richard Marti (:Paenglab) from comment #16)
> Comment on attachment 8810203 [details] [diff] [review]
> WIP Patch V.2 - fully functional, but missing DirTree Selection

Thank you for rapid ui-review...

> This patch doesn't work as it should. When you set an AB as default and
> reopen the properties, there is no ticked checkbox.

Oops, sorry, my mistake, thanks for pointing out. One variable not properly declared; fixed for next iteration.

> Only in address lists
> the checkbox is showing. But why give you this possibility to address lists?
> A address list shouldn't become a AB functionality. Only because it's in the
> tree is no reason.

You should know me better ;) I believe that ux-efficiency and customizability are part of our brand core, so I tend to ask the other way round: Why NOT offer the possibility of making a mailing list the default view when starting the main address book? Different users have different needs...

Conceptually, mailing lists are subsets of their parent AB, as reflected in the directory tree visual hierarchy:
> All ABs
> + Parent AB
>   + Mailing List (data subset of Parent AB)
(Even in a new AB world, mailing lists would still define subsets of /various/ ABs.)

I think there's a reason why mailing lists were included in the main directory tree, as the conceptual difference between address books and mailings lists is very small; apart from one being a data subset of the other, there's practically no difference at all (notwithstanding implementation differences owed to bad design). Open an address book, and you see a set of contacts. Open a mailing list, and you see a reduced (filtered) set of contacts. Which is very useful because it allows selecting and grouping contacts according to user-defined criteria (much like a saved search). It's also very useful to *view* exactly and only that subset of contacts, so that all other non-matching contacts will be out of the way. So if it's a useful view depending on users needs and hierarchical organization of data, why wouldn't it be useful as a default view IF the user so wishes?

For example, a company might have an address book for all of their customers. Therein, mailing lists like the following:

> All Customers AB
> + Active customers list
> + VIP customers list
> + Newsletter customers list
> + Customers of product/service XYZ

Depending on the AB data structure and workflows of the company, I find it VERY plausible that one of these mailing lists might be the focus of attention wrt address book management. E.g., if 99% of all AB lookups involve "Active customers", it makes a lot of sense to set "Active customers list" as the default view for AB startup.

Furthermore, as you have just requested to shorten the label caption, imo having a tiny checkbox on mailing list dialogue is minimally intrusive but potentially very valuable to user workflows, so the cost-benefit ratio looks good.

Also for the "Remember last used" feature, I think the legitimate expectation is that the AB view upon coming back should be exactly as you left it, so it would be surprising and annoying that if you exit with a particular mailing list shown, upon return we'd show the parent AB instead. (And if we remember mailing lists for "last-used", it would be inconsistent to not allow "show-as-default").

> Now the AB window remembers the last selected AB no matter what I try to
> select as default. Maybe we should only implement this functionality ;)

"Remember last used" is nice functionality, but it's not for everyone. As evidenced by user complaints about the status quo, many users want a stable startup view which best serves their default scenarios. So if you want to implement "last-used" only, we'll redirect the resulting user frustration to you ;)

> I somehow don't like the sentence: "Show as default when starting the main
> address book". It' is way too long. Maybe "Show as default Address Book" is
> enough. We don't need to take care for the contacts sidebar in this place.

Oh, I see. OK, I have fixed that. Actually, I wasn't thinking of contacts side bar at all, but I was trying to clarify the meaning of "Show as default" which may not be self-explanatory: default for what exactly? But anyway, maybe less is more so let's go for your shorter proposal, which is also practical for documentation. I suggest that we just have "Show as default" (without "address book"), because it's already clear from the implied context of the dialogue: "Show [this address book] as default" and "Show [this mailing list] as default" respectively. So "Show as default" is generic, short, and simple.

Personally, I would add a tooltip on the checkbox:
"Show this address book as default when starting the main address book"
"Show this mailing list as default when starting the main address book"
So more detailed information will be available exactly and only when needed, without cluttering the main UI. But from experience, I'll only add the tooltips after your explicit prior consent ;)

> I get also this errors:
> Sun Nov 13 2016 20:01:13
> Error: ERROR: failed to set status text:  [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 644"  data: no]
> Source file: chrome://messenger/content/addressbook/addressbook.js
> Line: 518

I'm not sure if my patch is causing this error; if so, then it's probably because of the missing selection in directory tree, which is the missing piece of this patch (as mentioned before).

>  ----------
> Sun Nov 13 2016 20:01:19
> Warning: ReferenceError: assignment to undeclared variable gShowAsDefault

Thanks, fixed.

	Thomas D. (:thomas8) Reporter
	Comment 18 • 8 years ago

New patch per my comment 17, addressing Richard's comment 16.

	Richard Marti (:Paenglab)
	Comment 19 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #18)
> Created attachment 8810746 [details] [diff] [review]
> WIP Patch V.2.1 - fully functional (hopefully), but missing DirTree Selection
> 
> New patch per my comment 17, addressing Richard's comment 16.

Setting a LDAP directory as default doesn't work. The other are working.

(In reply to Thomas D. (needinfo?me) from comment #17)
> So "Show as default" is generic, short, and simple.
> 
> Personally, I would add a tooltip on the checkbox:
> "Show this address book as default when starting the main address book"
> "Show this mailing list as default when starting the main address book"
> So more detailed information will be available exactly and only when needed,
> without cluttering the main UI. But from experience, I'll only add the
> tooltips after your explicit prior consent ;)

This would be the only tooltip in this dialogs. In AB dialog no surprise as it would be the second element in it, but mailing list- and LDAP dialog have a lot more elements without tooltips.

I would go with only "Show as default".

	Thomas D. (:thomas8) Reporter
	Comment 20 • 8 years ago

Fixed LDAP.

	Richard Marti (:Paenglab)
	Comment 21 • 8 years ago

Comment on attachment 8811200 [details] [diff] [review]
WIP Patch V.2.2 - fully functional, but missing DirTree Selection

Looks good now

	Thomas D. (:thomas8) Reporter
	Comment 22 • 8 years ago

Yippiee yeah!!!

Discovered an existing function (getIndexForId) to get the index from directory URI, somewhere in abTrees.js, which holds the implementation of AB's DirTreeView.

So now everything works perfectly as expected for all standard scenarios.

Unfortunately, I wasn't able to expand the default directory in case it's collapsed. But then, it's your own fault if you start collapsing things which you want to see the next time you start your AB... ;) Spent the whole day digging deep in the internals but just could not find any way to even access the collapsed directory. Trees are too deep... So in that case, and in the somewhat unlikely event that you delete your favorite AB, we just dynamically redirect you to the "All Address Books" root directory. I think that's quite acceptable for those edge cases (collapsing "All Address Books" really doesn't make much sense), so this should be ready to land if it passes review.

Aceman, as this comes with strings, rapid review would be appreciated. Tia! :)

	Thomas D. (:thomas8) Reporter
	Comment 23 • 8 years ago

Hello Mike, after digging long and deep into AB's directory treeview without luck, can I ask you for advice as a veteran guru in this field:
In this bug, we enable user to define a default directory which will show up every time when addressbook is started. It works well as long as user doesn't collapse the tree.
How can I access a collapsed directory item from AB's directory treeview (at all!), so as to expand only that item and otherwise keep persisted open states?
The idea is that even if user collapses everything to only have "All Address Books" in the directory tree, we want to expand and select user's default directory for next AB startup. Thanks for any hints.

	Thomas D. (:thomas8) Reporter
	Comment 24 • 8 years ago

This bug might affect Seamonkey. This feature is probably useful for Seamonkey, too. Can you please verify and port as appropriate. Thank you.

	Thomas D. (:thomas8) Reporter
	Comment 25 • 8 years ago

The l10n impact of this bug is minimal, essentially *one string* ("Show as default") with a handful of duplicates.
User impact if declined: Many users have complained that they are not happy with "All Address Books" as the forced view default for Address Book. It annoys them every time they open their main AB.

	Thomas D. (:thomas8) Reporter
	Comment 26 • 8 years ago

Comment on attachment 8812593 [details] [diff] [review]
RC Patch V.3

Hi Jörg,

I'd really like to see this land (has one l10n string with some duplicates), so whoever gets to the review first will be welcome... Works as expected on my local installation, apart from the edgecase described in comment 22 and comment 23.

(In reply to Thomas D. (needinfo?me) from comment #25)
> The l10n impact of this bug is minimal, essentially *one string* ("Show as
> default") with a handful of duplicates.
> User impact if declined: Many users have complained that they are not happy
> with "All Address Books" as the forced view default for Address Book. It
> annoys them every time they open their main AB [see comment 1]

	Jorg K (CEST = GMT+2)
	Comment 27 • 8 years ago

I lightly tested this before reviewing it. With a default, the default AB is chosen when the address book is next opened; with no default, the last opened one is chosen. Nice functionality ;-) Let's see the code next.

	Jorg K (CEST = GMT+2)
	Comment 28 • 8 years ago

Comment on attachment 8812593 [details] [diff] [review]
RC Patch V.3

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

OK, I gave it one pass through. It's a mix of nits, unclear stuff, funny logic (unless I'm hallucinating) and some things that could be made simpler. Let's have another round ;-)

::: mail/components/addrbook/content/abCommon.js
@@ +486,5 @@
> +  let defaultUri = Services.prefs.getCharPref("mail.addr_book.view.defaultURI");
> +  if (!defaultUri) {
> +    // if pref fails, fall back to "All Address Books" root directory
> +    defaultUri = kAllDirectoryRoot + "?"
> +    dump('Error: Services.prefs.getCharPref("mail.addr_book.view.defaultURI") failed!');

You want to return here or keep going? You'll be calling getIndexForId() on an undefined value in the next line.

@@ +489,5 @@
> +    defaultUri = kAllDirectoryRoot + "?"
> +    dump('Error: Services.prefs.getCharPref("mail.addr_book.view.defaultURI") failed!');
> +  }
> +  let defaultDirTreeIndex = gDirectoryTreeView.getIndexForId(defaultUri);
> +  // To do: If directory of defaultURI is collapsed, we fail to find and select

Standard for "to do" is XXX TODO.

@@ +490,5 @@
> +    dump('Error: Services.prefs.getCharPref("mail.addr_book.view.defaultURI") failed!');
> +  }
> +  let defaultDirTreeIndex = gDirectoryTreeView.getIndexForId(defaultUri);
> +  // To do: If directory of defaultURI is collapsed, we fail to find and select
> +  // the thing, so getIndexForId returns -1; for now, fall back to "All Address

s/the thing/it/. Sorry, I'm new here, what does "collapsed" mean in this context?

::: mail/components/addrbook/content/abEditListDialog.xul
@@ +48,4 @@
>      </hbox>
>  
>      <spacer style="height:1em"/>
> +    <label control="addressCol1#1"

I don't understand this change, please explain.

::: mail/components/addrbook/content/addressbook.js
@@ +102,5 @@
>  {
> +  let currDirURI = GetSelectedDirectory();
> +  let rememberURI = Services.prefs.getBoolPref("mail.addr_book.view.rememberLastUsedURI");
> +  if (rememberURI) {
> +    Services.prefs.setCharPref("mail.addr_book.view.defaultURI", currDirURI);

I don't like the naming and the logic. Your setting "default" even if there is no default.

I'd call this startupURI and haveDefault. This then becomes:

// If no AB was chosen as default, remember the current one as startup AB.
if (!haveDefault)
  Services.prefs.setCharPref("mail.addr_book.view.startupURI", currDirURI);

@@ +180,4 @@
>    gDirectoryTreeView.init(gDirTree,
>                            kPersistCollapseMapStorage);
>  
> +  SelectDefaultViewDirectory();

I'd call this SelectStartupDirectory().

::: mailnews/addrbook/content/abAddressBookNameDialog.js
@@ +30,5 @@
>    }
> +  // Set showAsDefaultBox.checked if gDirectory is defined and the current
> +  // user-defined default directory.
> +  initializeShowAsDefault(showAsDefaultBox, gDirectory);
> +  

trailing space. Do you use Notepad++. Do a regexp search for | $| in the patch so you'll see the trailing spaces you've added.

@@ -46,3 @@
>      gNameInput.readOnly = true;
> -    document.documentElement.buttons = "accept";
> -    document.documentElement.removeAttribute("ondialogaccept");

Please explain this change.

@@ +66,5 @@
> +  if (gDirectory) {
> +    // Rename the directory only if it's none of those below with readonly names.
> +    if (gDirectory.URI != kCollectedAddressbookURI ||
> +        gDirectory.URI != kPersonalAddressbookURI ||
> +        gDirectory.URI != kAllDirectoryRoot + "?") {

I don't understand this logic.
If (x != 1 || x != 2 || x != 3) is always true.
Am I missing something? Perhaps I'm hallucinating again.

::: mailnews/base/util/ABQueryUtils.jsm
@@ +133,5 @@
> +
> +/**
> + * For various cmd_properties dialogues, initialize the checkbox "Show as
> + * default when starting the main address book" and remember related settings
> + * 

Trailing space.

@@ +157,5 @@
> + * For various cmd_properties dialogues, when closed with OK, handle the
> + * checkbox "Show as default".
> + * If checked, make that AB the view default for next AB startup.
> + * If unchecked for all ABs, remember the last used AB.
> + * 

And here.

@@ +161,5 @@
> + * 
> + * aDirURI   The URI of an existing item in the AB Directory Tree (AB, LDAP Dir,
> + *           or Mailing List), which may also be a newly-created item.
> + */
> +function handleShowAsDefault(aDirURI) {

Maybe call this handleStartup.

@@ +164,5 @@
> + */
> +function handleShowAsDefault(aDirURI) {
> +  if (gShowAsDefaultOldValue == gShowAsDefault.checked) {
> +    // Do nothing if the checkbox wasn't changed by user.
> +    return true;

No one uses the return value plus, below there is no return value, plus the return value is not documented.
Also, please document parameters following the current standard.

@@ +174,5 @@
> +    Services.prefs.setBoolPref("mail.addr_book.view.rememberLastUsedURI", false);
> +  } else {
> +    // Current explicit default view directory was unchecked by user;
> +    // remember the last used directory when closing AB.
> +    Services.prefs.setBoolPref("mail.addr_book.view.rememberLastUsedURI", true);

This becomes easier to read with my variable names and more orthogonal. Why should this function know that you want to remember something?
Just set "haveDefault" to false and let the "unload" function do the right thing.

::: mailnews/mailnews.js
@@ +155,5 @@
>  pref("mail.showCondensedAddresses", false);
>  #endif
>  
> +pref("mail.addr_book.view.defaultURI", "moz-abdirectory://?");
> +pref("mail.addr_book.view.rememberLastUsedURI", false);

I suggest to change the names.

	Thomas D. (:thomas8) Reporter
	Comment 29 • 8 years ago

This patch addresses Jörg's review of comment 28. Looks better now! :)

This patch must be applied on top of:
Bug 1319409, Attachment 8814669 [details] [diff]
Bug 196135, Attachment 8813181 [details] [diff]
We should really start to land them...

(In reply to Jorg K (GMT+1) from comment #28)
> Comment on attachment 8812593 [details] [diff] [review]
> RC Patch V.3
> 
> Review of attachment 8812593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, I gave it one pass through. It's a mix of nits, unclear stuff, funny
> logic (unless I'm hallucinating) and some things that could be made simpler.
> Let's have another round ;-)

In spite of those nits and one inconsequential logical error, it was working perfectly :)

> ::: mail/components/addrbook/content/abCommon.js
> @@ +486,5 @@
> > +  let defaultUri = Services.prefs.getCharPref("mail.addr_book.view.defaultURI");
> > +  if (!defaultUri) {
> > +    // if pref fails, fall back to "All Address Books" root directory
> > +    defaultUri = kAllDirectoryRoot + "?"
> > +    dump('Error: Services.prefs.getCharPref("mail.addr_book.view.defaultURI") failed!');
> 
> You want to return here or keep going? You'll be calling getIndexForId() on
> an undefined value in the next line.

It's fine to keep going because as explained in code comment, if pref is not found, defaultURI (now startupURI) falls back to kAllDirectoryRoot + "?", so it's defined and we can proceed, a bit of extra safety for AB startup.

> > +  // To do: If directory of defaultURI is collapsed, we fail to find and select
> > +  // the thing, so getIndexForId returns -1; for now, fall back to "All Address
> 
> s/the thing/it/. Sorry, I'm new here, what does "collapsed" mean in this
> context?

In the directory tree, when you use double-click or twisty to collapse either "All ABs" node, or the node of an AB which contains mailing lists.
Upon startup, if the default startup directory is not visible in the tree because it has been collapsed away, I was not able to access that row for expanding so that we can show it at startup. So we select "All ABs" node at startup time instead. But in a way, that behaviour might even make sense, maybe there's a reason why user collapsed things, so we respect that ;)

> ::: mail/components/addrbook/content/abEditListDialog.xul
> @@ +48,4 @@
> >      </hbox>
> >  
> >      <spacer style="height:1em"/>
> > +    <label control="addressCol1#1"
> 
> I don't understand this change, please explain.

Haha, it's a hack! In mailing list dialogues, the actual list of mail addresses had a label, but no accesskey. I added an accesskey and it still didn't work. Label has control="addressingWidget" which looks correct but unfortunately it doesn't work, too many other things nested inside. So I played with it and found that if we use control="addressCol1#1", at least it works to shift the focus into the list, unfortunately with the first address row selected. That's not perfect, but it's better than before because now we have a working accesskey, and it's not worse than before, because the only other existing method of keyboard-access to the widget will /also/ just select the first row, regardless of more rows present.

> ::: mail/components/addrbook/content/addressbook.js
> @@ +102,5 @@
> >  {
> > +  let currDirURI = GetSelectedDirectory();
> > +  let rememberURI = Services.prefs.getBoolPref("mail.addr_book.view.rememberLastUsedURI");
> > +  if (rememberURI) {
> > +    Services.prefs.setCharPref("mail.addr_book.view.defaultURI", currDirURI);
> 
> I don't like the naming and the logic. Your setting "default" even if there
> is no default.
> I'd call this startupURI and haveDefault. 

Great! Indeed, my naming logic was poor. Thanks for suggesting better names and logic! I changed the names and logic along your lines:

mail.addr_book.view.startupURI
mail.addr_book.view.startupURIisDefault

That reflects logical and UI/behavioural realities very well and has the additional advantage of bundling those related prefs together by their names, which makes them easy to search and understand in config editor.

> @@ -46,3 @@
> >      gNameInput.readOnly = true;
> > -    document.documentElement.buttons = "accept";
> > -    document.documentElement.removeAttribute("ondialogaccept");
> 
> Please explain this change.

When AB rename dialogue had only name field, it was de facto renaming dialogue. So for dirs which can't be renamed, it was just showing the name, but there was nothing to change, so just having OK button without Cancel button was enough.
Now there's our new "Show as default" checkbox, which you can change even for dirs which can't be renamed. So we must show both OK and Cancel always.

> @@ +66,5 @@
> > +  if (gDirectory) {
> > +    // Rename the directory only if it's none of those below with readonly names.
> > +    if (gDirectory.URI != kCollectedAddressbookURI ||
> > +        gDirectory.URI != kPersonalAddressbookURI ||
> > +        gDirectory.URI != kAllDirectoryRoot + "?") {
> 
> I don't understand this logic.
> If (x != 1 || x != 2 || x != 3) is always true.
> Am I missing something? Perhaps I'm hallucinating again.

Ah no, thanks for pointing out my logical error, I fixed that.
Although it was inconsequential (and I think we had it before), because with name field disabled, those special dirs can't be renamed, so even if you rename in backend using the name field value, the name won't change. :)

> ::: mailnews/base/util/ABQueryUtils.jsm
> @@ +133,5 @@
> > +
> > +/**
> > + * For various cmd_properties dialogues, initialize the checkbox "Show as
> > + * default when starting the main address book" and remember related settings
> > + * 
> 
> Trailing space.

It really looks much worse and irritating with that single trailing space removed in empty function comment rows, but anyway...

> > +function handleShowAsDefault(aDirURI) {
> 
> Maybe call this handleStartup.

Hmmm, no, there's a "Show as default" checkbox and we first initialize and then handle that checkbox's checked attribute and the associated actions when toggled. So we only handle if the just edited/created directory is shown as default startup dir or not. Handling the startup is done upon loading/unloading AB.

> @@ +164,5 @@
> > + */
> > +function handleShowAsDefault(aDirURI) {
> > +  if (gShowAsDefaultOldValue == gShowAsDefault.checked) {
> > +    // Do nothing if the checkbox wasn't changed by user.
> > +    return true;
> 
> No one uses the return value plus, below there is no return value, plus the
> return value is not documented.

That's incorrect and should be fixed, so I fixed it (no return value). Fortunately again it didn't hurt ;)

> Also, please document parameters following the current standard.

That's a nice and recommendable bonus for perfection.

> > +pref("mail.addr_book.view.defaultURI", "moz-abdirectory://?");
> > +pref("mail.addr_book.view.rememberLastUsedURI", false);
> 
> I suggest to change the names.

Done, thanks!

	Jorg K (CEST = GMT+2)
	Comment 30 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #29)
> In spite of those nits and one inconsequential logical error, it was working
> perfectly :)
It was. So does running electricity through wet braces/suspenders/Hosenträger ;-)
Or this: http://www.bed-and-breakfast-corneilla.com/mcslider/js-image-slider.js

	Jorg K (CEST = GMT+2)
	Comment 31 • 8 years ago

Comment on attachment 8814961 [details] [diff] [review]
Patch V.4: Better pref names, nitfixes

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

Fine. I didn't run it but I expect it works as well as before. Thanks for accommodating my comments. Please add the comment as requested below.

I checked and this patch almost applies without applying the patch in bug 1319409, only one hunk fails which has ChangeDirectoryByURI(getSelectedDirectoryURI()); some where in an unchanged line in the context.

So we could actually land this first, right? It has string changes and bug 1319409 hasn't. Also there is no review requested here for SM. Are you smashing SM with this change? There a changes in mailnews/. Do they affect SM?

::: mail/components/addrbook/content/abEditListDialog.xul
@@ +47,5 @@
>        </hbox>
>      </hbox>
>  
>      <spacer style="height:1em"/>
> +    <label control="addressCol1#1"

Comment here please. Something like:
We associate the label with the first entry in the address list instead of the overall listbox because ...

	Thomas D. (:thomas8) Reporter
	Comment 32 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #31)
> Comment on attachment 8814961 [details] [diff] [review]
> Patch V.4: Better pref names, nitfixes
> 
> Review of attachment 8814961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fine. I didn't run it but I expect it works as well as before. Thanks for
> accommodating my comments. Please add the comment as requested below.
> 
> ::: mail/components/addrbook/content/abEditListDialog.xul
> @@ +47,5 @@
> >        </hbox>
> >      </hbox>
> >  
> >      <spacer style="height:1em"/>
> > +    <label control="addressCol1#1"
> 
> Comment here please. Something like:
> We associate the label with the first entry in the address list instead of
> the overall listbox because ...

Jörg, I honestly don't think this comment is needed or even helpful. We don't usually put comments into XUL unless it's really serious and significant. The truth here is that nobody except me will ever touch this code, and as you say, it is all bound to be dumped one day in the distant future, and even if somebody would actually bother, it's easy to just replace the ID and see for yourself that it doesn't work, and then take it from there. I'm all for perfection, but there's a limit, and there's a time for things to land even with only 98% perfection, or with a trailing space in comments. Let's save our valuable time for more important things...

> 
> I checked and this patch almost applies without applying the patch in bug
> 1319409, only one hunk fails which has
> ChangeDirectoryByURI(getSelectedDirectoryURI()); some where in an unchanged
> line in the context.
> 
> So we could actually land this first, right? It has string changes and bug
> 1319409 hasn't. Also there is no review requested here for SM. Are you
> smashing SM with this change? There a changes in mailnews/. Do they affect
> SM?

Whatever... I assume it will break the big patch in bug 1319409 if we land this first, and I'm not willing nor able to start all over again.
I believe this patch here breaks SeaMonkey. But I can't and I won't spend more time to fix this for SeaMonkey.
I guess for all of my current patches, they work for TB as is, so they are ready to land in the correct order. If anyone needs to do more, feel free to pick up from where I left.

	Jorg K (CEST = GMT+2)
	Comment 33 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #32)
> > So we could actually land this first, right? It has string changes and bug
> > 1319409 hasn't. Also there is no review requested here for SM. Are you
> > smashing SM with this change? There a changes in mailnews/. Do they affect
> > SM?

> Whatever... I assume it will break the big patch in bug 1319409 if we land
> this first, and I'm not willing nor able to start all over again.

I think there is no hope for bug 1319409 to get SM review before the late string date of Dec. 5th, so the only chance to land this would be to land it first. And the rework is minimal to turn the order around, I can do it.

> I believe this patch here breaks SeaMonkey. But I can't and I won't spend
> more time to fix this for SeaMonkey.

That's more severe. We can't go around and smash them.

Ian, Ratty, FRG, Rsx11m, can you please take a look at the Mailnews/ changes and see whether and how badly they affect you.

	Frank-Rainer Grahl (:frg)
	Comment 34 • 8 years ago

Not familiar with the address book code bug doesn't look like it will cause much problems. 

Maybe you could add the new strings to:

suite\locales\en-US\chrome\mailnews\addressbook

The files are more or less 1:1 copies and this way we can deal with any breakages later. Still on some other bugs.

	Jorg K (CEST = GMT+2)
	Comment 35 • 8 years ago

Comment on attachment 8814961 [details] [diff] [review]
Patch V.4: Better pref names, nitfixes

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

I've just been looking at the strings needed for SM and came across this, see below.

While we're waiting for reviews elsewhere, maybe Aceman can look this over as well.

::: mail/locales/en-US/chrome/messenger/addressbook/abMailListDialog.dtd
@@ +15,2 @@
>  <!ENTITY ListDescription.label          "Description: ">
> +<!ENTITY ListDescription.accesskey      "D">

Changed access key from e to D needs new string name, right? Is there a need to change it?

	Thomas D. (:thomas8) Reporter
	Comment 36 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #35)
> Comment on attachment 8814961 [details] [diff] [review]
> Patch V.4: Better pref names, nitfixes
> 
> Review of attachment 8814961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've just been looking at the strings needed for SM

Thank you for looking into this, I appreciate that! :)

> and came across this,
> see below.
> 
> ::: mail/locales/en-US/chrome/messenger/addressbook/abMailListDialog.dtd
> @@ +15,2 @@
> >  <!ENTITY ListDescription.label          "Description: ">
> > +<!ENTITY ListDescription.accesskey      "D">
> 
> Changed access key from e to D needs new string name, right?

No, it doesn't. Accesskeys are l10n-specific, de-DE has "Beschreibung" so they don't care if we change our access key for "Description". And it's unlikely (though not impossible) that they had the same problem which I was solving here for en-US. We only have to notify other l10ns if we change the main label (which might necessitate updating accesskeys in our and other l10ns, so we must change the identifier of the access key, too, to be safe). Say if we change the string from "Description" to "Explanation", we must change our accesskey identifier, even if it was "e" which works on both for us, because German might change from "Beschreibung" to "Erklärung", and if they had "B" as accesskey, it will no longer work, so it's wise to make them revisit their accesskey by changing the identifier.
So, damit wären nun alle Klarheiten beseitigt!

> Is there a need to change it?

Yes, to be more intuitive, more memorable, and more standards-conform (ux-efficiency).
Standard says use the first character whereever possible, and ensure it's memorable.
E.g. "Type email addresses to add them to the mailing list": "T" is first character, but not memorizable, because the widget is really the _m_ailing list (and that label should probably just be "List of email addresses"). So I picked "m" instead (which then ends up on e_m_ail, but well...). Could have picked "e" maybe...
For "Description", "D" is both first character and memorable.

As you can see in string files, there are strings for an "Up" and "Down" button, which however doesn't exist in the dialogue. So they didn't use "D" because it was reserved for "Down". But then, "Down" never came, and never will. So let's adjust to the current realities, and improve accessibility for that.

	Jorg K (CEST = GMT+2)
	Comment 37 • 8 years ago

I've been considering to land this without bug 1319409. Here's the patch to achieve this. The change is minimal (one line change) and bug 1319409 can be easily rebased.

If we decide to go ahead with this plan, I will also add the strings for SM.

Aceman, please review either patch. Oh, btw, I changed "Düllmann" from ANSI to UTF-8 in this patch so you can easily import it.

	Jorg K (CEST = GMT+2)
	Comment 38 • 8 years ago

Comment on attachment 8816211 [details] [diff] [review]
Patch V.4 (version that can be applied without bug 1319409)

Bug 1319409 has landed so we won't need this.

	Jorg K (CEST = GMT+2)
	Comment 39 • 8 years ago

Added strings for SM in pref-directory-add.dtd, abMailListDialog.dtd and abAddressBookNameDialog.dtd as requested by FRG in comment #34.

	:aceman Assignee
	Comment 40 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #2)
> And it doesn't hinder more advanced solutions in any way, e.g. adding a
> global pref UI with or without "Last used address book" option (which would
> just need another single boolean pref,

I would have liked this solution better as it would be much much less code ;)
And we already have something similar in Tools->Options->Composition->Addressing->Add outgoing addresses...

But we do not have any general panel for Addressbook prefs so far.

> Checkbox caption is subject to discussion, but I'm quite happy with the
> above.
> I tried the following
> [ ] Initially show this address book
> Aceman, what do you think?

I like this version if the only purpose is to show this AB as initial selection in the AB window.

But maybe you find some more uses for it in the future. E.g. do you want to select the AB in the compose addressbook sidebar?

	Jorg K (CEST = GMT+2)
	Comment 41 • 8 years ago

(In reply to :aceman from comment #40)
> And we already have something similar in
> Tools->Options->Composition->Addressing->Add outgoing addresses...

So are we saying here that we're tossing the whole solution and go for something else?

Like:
Tools > Options > Composition > Addressing:
Open the address book to: ...
with a list of address books with a first pseudo-one that says: last opened one.

Hmm, perhaps nicer than the default checkbox everywhere.

	:aceman Assignee
	Comment 42 • 8 years ago

I should have told that at the time of comment 2, so I don't want to be so mean:) The current solution also does have UI review.

	Jorg K (CEST = GMT+2)
	Comment 43 • 8 years ago

Maybe I need to be mean. While I like the opening to a defined address book, the extra check boxes labelled "Default" everywhere aren't so crash hot. And it's not a default in any way, it's a start-up setting. How hard can it be to whip up something else? Just scrap the check-boxes and get the input for the preferences from that panel.

Richard, what do you think about this?

	Richard Marti (:Paenglab)
	Comment 44 • 8 years ago

Yeah, not thought about this place. The default AB setting isn't so frequently changed to make the checkbox in dialog eligible.

	Jorg K (CEST = GMT+2)
	Comment 45 • 8 years ago

So that's bad news for Thomas. I think this would be the better solution.

	:aceman Assignee
	Comment 46 • 8 years ago

Comment on attachment 8816476 [details] [diff] [review]
Patch V.4.1 - added strings for SM

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

This works fine.
I looked through the code and have some nits.

But the other solution (somewhere in Tools->Options) should be much simpler.

::: mail/components/addrbook/content/abCommon.js
@@ +493,5 @@
> +  // is not found because it has been deleted; after fixing the collapsed case,
> +  // deletion will be the only case to end up here, then we could reset the pref
> +  // here (somewhat lazy and fuzzy).
> +  if (startupDirTreeIndex == -1) {
> +    startupDirTreeIndex = 0;

You assume index 0 is the All Addressbooks? Can you ensure that? E.g. above you correctly set the special kAllDirectoryRoot.

@@ +495,5 @@
> +  // here (somewhat lazy and fuzzy).
> +  if (startupDirTreeIndex == -1) {
> +    startupDirTreeIndex = 0;
> +  }
> +  gDirectoryTreeView.selection.select(startupDirTreeIndex);

You replace the above function SelectFirstAddressBook() with your new function. Please also copy the other work the old function is doing, like:
     if (gPreviousDirTreeIndex != 0)
      ChangeDirectoryByURI(getSelectedDirectoryURI());
  }
  gAbResultsTree.focus();

Or explain why it is no longer needed.

::: mail/components/addrbook/content/addressbook.js
@@ +103,5 @@
>  {
> +  // If there's no default startupURI, save the last used URI as new startupURI.
> +  let saveLastURIasStartupURI = !Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault");
> +  if (saveLastURIasStartupURI) {
> +    let selectedDirURI = getSelectedDirectoryURI();

You probably do not need the variable.

::: mail/locales/en-US/chrome/messenger/addressbook/abAddressBookNameDialog.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- Labels -->
> +<!ENTITY name.label               "Address Book Name:">
> +<!ENTITY name.accesskey           "A">

Please do not reindent the strings you are not changing. The string values do not need to be vertically aligned.

::: mailnews/addrbook/content/abAddressBookNameDialog.js
@@ +21,3 @@
>    gOkButton = document.documentElement.getButton('accept');
>    gNameInput = document.getElementById('name');
> +  let showAsDefaultBox = document.getElementById('showAsDefaultCheckbox');

Use double quotes for new strings.

@@ +67,5 @@
> +  if (gDirectory) {
> +    // Rename the directory only if it's none of those below with readonly names.
> +    if (gDirectory.URI != kCollectedAddressbookURI &&
> +        gDirectory.URI != kPersonalAddressbookURI &&
> +        gDirectory.URI != kAllDirectoryRoot + "?") {

Why are these new checks needed? And maybe you can just base the check on the name box being readonly.

@@ +76,5 @@
> +    // Create a new directory.
> +    let newAbBasePref = MailServices.ab.newAddressBook(newName, "", kPABDirectory);
> +    // Hack: Reconstruct the new directory's URI for purposes of show-as-default.
> +    let newABFileName = Services.prefs.getCharPref(newAbBasePref + ".filename");
> +    abURI = "moz-abmdbdirectory://" + newABFileName;

What are you trying to do here?

::: mailnews/base/util/ABQueryUtils.jsm
@@ +8,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["getSearchTokens", "getModelQuery",
>                           "modelQueryHasUserValue", "generateQueryURI",
> +                         "encodeABTermValue", "initializeShowAsDefault",
> +                         "handleShowAsDefault"];

I don't think these belong into ABQueryUtils.jsm file. abCommon.js or similar seems proper.

@@ +138,5 @@
> + * @param aCheckBox  The "Show as default" checkbox element
> + * @param aDirURI    The URI of the current directory; if omitted, we're creating
> + *                   a new directory, so the checkbox defaults to false.
> + */
> +function initializeShowAsDefault(aCheckBox, aDirURI) {

Do you really need the checkbox passed in or can you just look up the element in this function?

@@ +168,5 @@
> + *                 (AB, LDAP Dir, or Mailing List), which may also be a
> + *                 newly-created directory.
> + */
> +function handleShowAsDefault(aDirURI) {
> +  if (gShowAsDefaultOldValue == gShowAsDefaultBox.checked) {

I don't like this global variable. Can you maybe compare to the value of Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault") or something?

	Thomas D. (:thomas8) Reporter
	Comment 47 • 8 years ago

(In reply to Richard Marti (:Paenglab) from comment #44)
> Yeah, not thought about this place. The default AB setting isn't so
> frequently changed to make the checkbox in dialog eligible.

I'd really appreciate if we could arrive at UX-decisions in a principled and reflected way and not just willy-nilly and subject to change at any breeze of wind which comes along. Again, you should know me better. No UX without intensive consideration.

Allow me to speak up in defense of my solution, not just because it's my solution and I spend many hours and days to implement it, but for the thing itself.

1) We are trying to solve a pressing UX problem here, which has been reported by a number of users. Users reporting here are just the tip of the iceberg, if you start seeing them here there's definitely more out here. So my first practical question is: suppose we'd think that an option hidden somewhere in the conglomerat of all options is the way forward, who will step forward to re-implement this as a central option?

(In reply to Richard Marti (:Paenglab) from comment #44)
> Yeah, not thought about this place. The default AB setting isn't so
> frequently changed to make the checkbox in dialog eligible.

2) I have several doubts about that logic. AB names and AB properties in general are almost never changed after setting up an AB. Like LDAP - who's ever going to touch that again after setting up? Following that logic we should just eliminate the properties dialogue and try to stuff everything into a centralized options tree. So what's the point of "properties" then? Is it not exactly to hold item-specific information? Let's be honest and admit that "startup AB" qualifies for both UI locations at least equally.

3) So if AB properties in general are NOT so frequently used, at least there's no reason NOT to put our new property there. In fact, contextual AB properties are a very good place for this feature BECAUSE they are rarely used, but still easy to find when need is. But you may still ask, what about putting it into central options? Wouldn't that be good, too?

4) Centralized options might have their purpose for some things, but in general, for the majority of users, it's an area where they never want to go, and you wouldn't even want them to go there because we can't be sure if they know what they are doing. Even though it's well-sorted, it's still a massive conglomerate of things which isn't easy to navigate and understand. Even I as a power user find myself searching for some of the options I already know, and I don't like being in options too much. I have a suspicion that centralized options were "cool" at a certain time in the past, to show how technically advanced your application is. Things have changed, and options have been quite radically reduced, slimmed out, and simplified for a reason. The trend, even in our good old Thunderbird, is going the other way: Away from centralized options, towards contextual, in-place UI. That's why you see we have grown links over time to some of the things hiding in options: From primary tags UI, we (I personally) have added a "Manage tags" link to get to the options section of Tags. Because without that link, most people are never gonna go there, they may not even know that it exists. Same for attachment reminder keywords: Again, a link from primary UI right-down into deeply nested options. And again, nobody would ever gonna find that without the link. Even account settings (what a conglomerate indeed), it's hooked up from primary UI. Plus that massive complaint from users when attachments were suddenly hidden by default, and they couldn't change it: We implemented a contextual menu *right there* on the attachment header bar. Because on a modern application, I'll try context menu anywhere if I want to do something. That's the general progress which applications have made: away from global conglomerates of commands and options, towards contextual UI where you can do all of your things in-place instead of searching for commands and options. Conclusion: Where options can't be avoided, at least linking them from primary UI makes better UX in terms of ux-discovery, and ux-efficiency (it's MUCH easier and faster to reach those nested options with one contextual click from primary UI than navigating trough the whole options tree and trying to figure out, which exact compartment this option's gonna be in). So for our case: Perhaps we would do well to add a link to that global option? But then, we can just as well have the option right there where it belongs, avoid adding to the conglomerate, and allow users to actually find this feature easily.

5) External UX-consistency. I always like to have a look around how one of the most successful softwares of all times, M$ Windows, is doing things. They are not perfect, we don't have to like them, they are proprietary and evil and all, but still, they are successful, and successful for a reason. If you compare Windows 7 or Windows 10 to ancient Windows 3.1, you'll see a LOT of intelligent progress there. If we are complaining about Windows 10, then we are really spoilt because so many things in this windows world have never been easier than this (Yeah, some things get over-simplified to an extent they are complicated again, admittedly, but still). Check out, e.g. Location bar of Windows Explorer. Now each and every part of the path is clickable, and even comes with subfolders as a dropdown. And STILL you can get the good old DOS-style path for copying, but only when you really go for it by clicking right inside. That's progress. And it's IN-PLACE. Just like about almost everything. Taskbar, context menus, etc. you name it. Ok, I get carried away. Here's the real point: Looking for analogies, pls have a look at your windows printers. How do you make one of them the default printer? Oh yes, you just open up your context menu and voila: Set as Default Printer. Easy. And easy to find. OK, you don't change your default printer every day. But for those few times when you do, would you really prefer to dig somewhere into global options (or even registry) and search for the "Windows Default Printer" setting? I definitely wouldn't. Plus, I even see a nice green tick on my default printer, which makes it easy to spot and understand the concept. Oh, that's why. The thing with the green tick keeps coming up BECAUSE of that green tick. Implementing a startUp AB tick might actually be worth exploring. We might think of other ways of marking like a light-yellow background circle behind the icon, or some special light background for the whole row of startup AB, or some such. Even a tooltip saying that "This is your current default startup directory."

6) I also contest the general assumption that the default startup URI will hardly ever be changed. Says who? For a lot of users, maybe yes. But then there are others. Who knows for what reason they might want to change their startup URI more often. Even monthly would be a regular change. Maybe somebody working through different ABs to manage them, one per week. Set your default startup, continue tomorrow from where you left. Why not make that convenient? Digging options is, I repeat, NEVER convenient. More so when we are not even helping anyone by NOT putting it on properties. Properties is more advanced, less used UI already, but at least still reachable from primary UI. Maybe the AB's themselves are dynamic datasets which are archived and replaced by another AB every month, for whatever reason. Why make changing the default startup AB more difficult than necessary? Just because we CAN hide it in options, it's not a good reason do do that.

7) Having said which, I'm not totally against having that option in options. But I would rather have it *in addition* to the primary properties UI checkbox. One advantage of having it in options is that we can make the "Remember last used directory" setting more explicit/discoverable. Why not have it both ways: Contextual primary UI for ux-efficiency and ux-discovery, AND an option in option, for those who prefer ordered conglomerates of complexity, and which is justified for the "Remember last used directory" setting, which may not be easy to integrate into properties (although a simple dropdown box having "Make this your startup directory...", "Show as startup directory" and "Remember the last used directory" might do the trick, we should try that).

Bottom line: This checkbox is correctly and conviently placed in the rarely used properties dialouges, for reasons of ux-minimalism, ux-discovery, ux-consistency, and ux-efficiency.

	Thomas D. (:thomas8) Reporter
	Comment 48 • 8 years ago

8) Plus let's be honest, LDAP is VERY advanced, very unlikely for private users, and even Mailing Lists (broken as they are) are certainly much LESS frequently used than the plain old normal personal AB. Which means that for most users, the properties dialogue where they are going to meet this feature is that properties dialogue which currently has exactly ONE property: AB name. So it's actually a RENAME dialogue, not a properties dialogue. I was actually happy to add it there so that "Properties" (Plural) starts to make sense and keep the promise!

	Thomas D. (:thomas8) Reporter
	Comment 49 • 8 years ago

9) We even have anecdotal evidence how real users react to my proposal of checkbox in properties dialogue:

(In reply to JP from comment #3)
> Agree with Thomas D. solution above.
> Definitely well thought out.
> 
> Look forward to the implementation.
> 
> JP

(In reply to Mitch in Oakland from comment #6)
> (In reply to Thomas D. (needinfo?me) from comment #5)
> ...
> I'm not sure whether I'll be able to enjoy using your new feature on a
> maintenance update to TB 45, or whether I'll need to wait until I update the
> OS (and perhaps even switch back to TB Beta), but thank you in advance,
> either way. :-)

	Thomas D. (:thomas8) Reporter
	Comment 50 • 8 years ago

(In reply to :aceman from comment #46)
> Comment on attachment 8816476 [details] [diff] [review]
> Patch V.4.1 - added strings for SM
> 
> Review of attachment 8816476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This works fine.
> I looked through the code and have some nits.
> 
> But the other solution (somewhere in Tools->Options) should be much simpler.

Much simpler for whom? We're not here to make it simple for coders, but simple for users!

> ::: mail/components/addrbook/content/abCommon.js
> @@ +493,5 @@
> > +  // is not found because it has been deleted; after fixing the collapsed case,
> > +  // deletion will be the only case to end up here, then we could reset the pref
> > +  // here (somewhat lazy and fuzzy).
> > +  if (startupDirTreeIndex == -1) {
> > +    startupDirTreeIndex = 0;
> 
> You assume index 0 is the All Addressbooks? Can you ensure that? E.g. above
> you correctly set the special kAllDirectoryRoot.

I have to check if I can use kAllDirectoryRoot here, but I don't think it's likely that anything else is going to be on top as long as we have "All Address Books" node. But actually, it doesn't even matter. This is just a fallback so choosing whatever is your first directory in the list does seem to make sense for fallback.

> @@ +495,5 @@
> > +  // here (somewhat lazy and fuzzy).
> > +  if (startupDirTreeIndex == -1) {
> > +    startupDirTreeIndex = 0;
> > +  }
> > +  gDirectoryTreeView.selection.select(startupDirTreeIndex);
> 
> You replace the above function SelectFirstAddressBook() with your new
> function. Please also copy the other work the old function is doing, like:
>      if (gPreviousDirTreeIndex != 0)
>       ChangeDirectoryByURI(getSelectedDirectoryURI());
>   }
>   gAbResultsTree.focus();
> 
> Or explain why it is no longer needed.

I think I did some logs on this and found that the ChangeDirectoryByURI is completely redundant here because it gets called anyway so having it here will call it twice which is less performant. Just put a log into the ChangeDirectoryByURI function and then check how often it gets called.
I intentionally moved gAbResultsTree.focus() out of here because the next focus does not have much to do with this function. We actually want to focus the searchbox, we just had to deactivate that because it disabled some buttons whose isEnabled functions are apparently wrongly set up.
Plus, it all works perfectly without this, so why add more loops which seem to do nothing useful?

> ::: mail/components/addrbook/content/addressbook.js
> @@ +103,5 @@
> >  {
> > +  // If there's no default startupURI, save the last used URI as new startupURI.
> > +  let saveLastURIasStartupURI = !Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault");
> > +  if (saveLastURIasStartupURI) {
> > +    let selectedDirURI = getSelectedDirectoryURI();
> 
> You probably do not need the variable.
> 
> :::
> mail/locales/en-US/chrome/messenger/addressbook/abAddressBookNameDialog.dtd
> @@ +3,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <!-- Labels -->
> > +<!ENTITY name.label               "Address Book Name:">
> > +<!ENTITY name.accesskey           "A">
> 
> Please do not reindent the strings you are not changing. The string values
> do not need to be vertically aligned.

Yeah, but does it hurt much to align them?

> ::: mailnews/addrbook/content/abAddressBookNameDialog.js
> @@ +21,3 @@
> >    gOkButton = document.documentElement.getButton('accept');
> >    gNameInput = document.getElementById('name');
> > +  let showAsDefaultBox = document.getElementById('showAsDefaultCheckbox');
> 
> Use double quotes for new strings.
> 
> @@ +67,5 @@
> > +  if (gDirectory) {
> > +    // Rename the directory only if it's none of those below with readonly names.
> > +    if (gDirectory.URI != kCollectedAddressbookURI &&
> > +        gDirectory.URI != kPersonalAddressbookURI &&
> > +        gDirectory.URI != kAllDirectoryRoot + "?") {
> 
> Why are these new checks needed? And maybe you can just base the check on
> the name box being readonly.

Will look into that.

> @@ +76,5 @@
> > +    // Create a new directory.
> > +    let newAbBasePref = MailServices.ab.newAddressBook(newName, "", kPABDirectory);
> > +    // Hack: Reconstruct the new directory's URI for purposes of show-as-default.
> > +    let newABFileName = Services.prefs.getCharPref(newAbBasePref + ".filename");
> > +    abURI = "moz-abmdbdirectory://" + newABFileName;
> 
> What are you trying to do here?

Ehhhm, isn't there a comment explaining exactly what I'm doing here?
If you can propose another, easier way to get the URI which is needed for the handleShowAsDefault function, please go ahead... I found previous comments complaining about exactly the same thing that unfortunately those functions who create new items in AB do NOT return those items as objects...
So here, the function which creates the new Ab just returns the new pref created for that Ab, so that's all I have about the new AB, so that's all I can use to re-create the URI of that directory...

> ::: mailnews/base/util/ABQueryUtils.jsm
> @@ +8,5 @@
> >  
> >  this.EXPORTED_SYMBOLS = ["getSearchTokens", "getModelQuery",
> >                           "modelQueryHasUserValue", "generateQueryURI",
> > +                         "encodeABTermValue", "initializeShowAsDefault",
> > +                         "handleShowAsDefault"];
> 
> I don't think these belong into ABQueryUtils.jsm file. abCommon.js or
> similar seems proper.

I think I tried that and for the way I am constructing things, it does NOT work. I understood that jsm are special creatures especially in that they memorize variables, which is what I used here. I might be wrong. I'm dealing with directory URI's, and quicksearches are also just a variant of directory URIs with query additions. So it looks compatible to me. But you are suggesting to remove one global variable, so we'd remain with one. Maybe it can also be removed, then it might be possible to put it somewhere else. On the other hand, it works as it is... I'm a bit surprised why reviews are so perfectionist here because I'm always hearing from Jörg that the whole AB code is doomed anyway. So my main concern here is to make things WORK for our users, not to have perfect code. Same argument for the checkbox, by the way: I'd much prefer to have an imperfect checkbox than not to solve our users' problem at all and postpone them again and let them suffer more. But of course, otherwise better code is always better, and it's a learning opportunity. Unfortunately it's also deep learning in doomed code... :/

> @@ +138,5 @@
> > + * @param aCheckBox  The "Show as default" checkbox element
> > + * @param aDirURI    The URI of the current directory; if omitted, we're creating
> > + *                   a new directory, so the checkbox defaults to false.
> > + */
> > +function initializeShowAsDefault(aCheckBox, aDirURI) {
> 
> Do you really need the checkbox passed in or can you just look up the
> element in this function?

I've changed this back and forth and this ultimately seemed the most convenient way to do it. Does it matter? But I'll give it a try, if we can eliminate the checkbox argument, that's fine.

> @@ +168,5 @@
> > + *                 (AB, LDAP Dir, or Mailing List), which may also be a
> > + *                 newly-created directory.
> > + */
> > +function handleShowAsDefault(aDirURI) {
> > +  if (gShowAsDefaultOldValue == gShowAsDefaultBox.checked) {
> 
> I don't like this global variable. Can you maybe compare to the value of
> Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault") or
> something?

Oh, I was trying to improve performance. Is remembering a global variable not more performant than looking up the pref again?
Otoh, if we eliminate the two global variables, it'll be easier to place this function somewhere else...

Finally: A general statement: I'm facing massive challenges of time spent on this. I'm not here for perfection (although I'm certainly perfectionist), I'm here to fix things. Those iterations for things which already work, in code which is ultimately doomed, they cost a lot of time which I'd rather spend on improving other things while I'm here. Maybe others can streamline my code in followup bugs...

	Thomas D. (:thomas8) Reporter
	Comment 51 • 8 years ago

In general, please don't trust the current AB code to do the right thing, especially when it comes to testing for things, or trying to update commands, views etc. Most tests which I found are actually redundant, probably from overcautious coding which did not fully understand all the nested functions which are there. If you track them back, you find that the tests are totally redundant and often running the same tests twice or more, same for updating.
A good example was found in my other bug:

You'd certainly expect that THIS code does something useful and special, since the check is different from all other types checks which just check for the type only:
> else if (types == kCardsOnly && gAbView && gAbView.selection) {
But alas, it turns out that the function which actually creates the types variable already does all those checks, so there's no way tht gAbView and gAbView.selection could NOT exist at this time (and if you think about it, to determine the types of selected items in ABView, you really need both the view and the selection otherwise how would you find the types!?)
So it's a completely useless test which just made the code harder.
Such useless code is very common here from what I have sen.

	Mitch in Oakland
	Comment 52 • 8 years ago

Please forgive me: I'm not a power user, nor a programmer, just a longtime TB user on a Mac (going all the way back to Netscape Mail)... longtime on a Mac, too (since System 6!), where there's no such thing as a "right click."

In my experience, contextual menus are essentially HIDDEN menus -- hence, basically of secondary or backup use (a hidden feature!), not the primary place I'd go to change the behavior or appearance of ANY application. If I want to change the behavior of an application -- ANY application -- I know I can go to "Preferences" (under that application's drop-down), and that's ALWAYS where I'm accustomed to going. I don't go there often, but I know these settings are there, safe-and-sound (all in one place, not scattered in "contextual" or window-specific menus) when I need to change them.

In this instance, moreover, there's an obvious (and obviously logical and intuitive) Preference panel that's appropriate for just this sort of setting (via a check-box or pop-up):
THUNDERBIRD>PREFERENCES>COMPOSITION>ADDRESSING

There's plenty of space on that Preference panel to add a line with a check-box and a pop-up listing the various address books (from which the user can drag down and choose a preferred AB).

I find it ironic that Thomas D now quotes me (from comment #6) in support of his proposal for a "contextual" setting. PLEASE READ THAT ENTIRE COMMENT #6 for a description of just how UN-intuitive such a solution (at least on a Mac) actually is!

I'd hate to see this entire process have to start all over again at Square One -- and I appreciate all the work that Thomas D has put into this (despite even his Windows-centric bias). ;-)

Would I settle for the "Properties" ("contextual") interface if that were to get this fix implemented more quickly? Of course! Would I rather see the setting on a Preference panel? You bet!

Either way, let's just get this done already, and stop quibbling!

	Mitch in Oakland
	Comment 53 • 8 years ago

PS: Please note that I'm arguing for placing this setting on a Preference panel -- and/or secondarily (as a last resort) in "Properties."

As for "Tools>Options"? That's even more obscure; I can't even find such a setting or menu item. Enough said?

	Jorg K (CEST = GMT+2)
	Comment 54 • 8 years ago

|Tools > Options| is the Windows menu for Preferences. See comparison here:
http://kb.mozillazine.org/Menu_differences_in_Windows,_Linux,_and_Mac

So if you're in favour of placing this in to Preferences, you're saying "yes" to |Tools > Options| ;-)

	Mitch in Oakland
	Comment 55 • 8 years ago

On a Mac, my UI location of choice would be a Preference panel:
THUNDERBIRD>PREFERENCES>COMPOSITION>ADDRESSING

As you've illustrated, this is evidently called "Tools > Options" in Windows. (On the Mac, there's also a "Tools" dropdown on the top menu bar: the dropdown varies depending on the front window, but there's never anything called "Options" on that menu.)

In any event, although I've indicated which UI location I prefer (and why I prefer it), I'm saying "Yes" to BOTH possibilities -- depending on which implementation is most practical. To be fair, I'll wait for a response from Thomas to my argument before I "cast a vote" either way.

	Mitch in Oakland
	Comment 56 • 8 years ago

PS: You (Jorg K and Thomas D) are evidently both old-timers familiar with the coding of these sorts of patches. I trust you both to work this out expeditiously. We're all adults here. :-)

	Jorg K (CEST = GMT+2)
	Comment 57 • 8 years ago

Let me join the discussion a little. If I understand it correctly, two options are discussed:
1) Put a check box into the address book properties
2) Put a preference somewhere.
I could add a third one:
3) Context-menu: See below, last paragraph.

I think it's wrong putting a pseudo-property onto the properties panel, since the start-up/default address book is a system setting, not a property of the individual address book.

It's correct that the centralised preference is hard to find and that the "switch" should be somewhere closer to the address book itself. That said, I discovered that on the *Address Book* panel (!!) (Mitch, forgive the Windows speak), Tools > Options takes you right to the right options.

So instead of option 1) - Right-click, Properties, you get option 2) - Tools > Options. In both solutions the option is two clicks away.

There is a third option: Put a checkbox into the right-click/context menu. Why? Because that's were we have it in a similar scenario for folders: "Favorite Folder" lets you nominate a pseudo-attribute of the folder. If set, it will show in the "Favorite Folders" view. So for address books you could have a "Select Initially" or some such thing. And if no address book has this setting, you show the last one used.

Anyway, 3) is possibly too much work, 2) is easier. We could land the strings for that and worry about fitting it out later.

Two strings from comment #41:

Show address book: Show last used  <== two strings here, much room for improvement.
                   Private
                   Work
                   Collected Addresses

	Mitch in Oakland
	Comment 58 • 8 years ago

I certainly prefer the Preference ("Tools") panel UI over a contextual menu alone. It's more intuitive and obvious, and less scattered. 

As I said, I could certainly live with putting this setting in a "Properties" dialog instead -- but only if that would streamline or simplify the implementation process.

Meanwhile, as Thomas D implicitly suggests, if we DON'T place the setting in "Properties," perhaps "Properties" should be re-labelled "Rename Address Book." ;-)

	:aceman Assignee
	Comment 59 • 8 years ago

This a quick patch to add the other possibilities to toggle the default flag. In the Preferences dialog and in the context menu of an addressbook. Just to get the feeling of the UI. And it also confirms that it is much less code.

	:aceman Assignee
	Comment 60 • 8 years ago

(In reply to :aceman from comment #59)
> This a quick patch to add the other possibilities to toggle the default
> flag. In the Preferences dialog and in the context menu of an addressbook.
> Just to get the feeling of the UI. And it also confirms that it is much less
> code.

The patch is to be applied on top of the patch from Thomas, it needs the backend handling.

(In reply to Thomas D. (needinfo?me) from comment #50)
> > But the other solution (somewhere in Tools->Options) should be much simpler.
> 
> Much simpler for whom? We're not here to make it simple for coders, but
> simple for users!

Sure, but in the current state of resources, we need to balance simplicity for coders and for users.
We can't always have the perfect solution for users if it means having to duplicate 5 strings and same block of XUL into various dialogs that need to be kept in sync in the future. Sure for a critical property like "name" you can't avoid that.

	Jorg K (CEST = GMT+2)
	Comment 61 • 8 years ago

Comment on attachment 8816729 [details] [diff] [review]
WIP more options

I can't believe it: You implemented both the preference *and* the right-click option at almost 3 AM. You are a *genius*. Where is the button to give f++++++?
That said, there is a minor bug. If you select an address book in the context menu, the options dialogue doesn't update.

Since it was my proposal, I want those two options and I want the pseudo-property scrapped on the properties panel. And I'd like it today since it needs to land tomorrow.

Note that Thomas' patch also contains changed to mailing list access keys:
+<!ENTITY ListDescription.accesskey      "D">
+<!ENTITY AddressTitle.accesskey         "m">
You might want to carry those forward.

	Richard Marti (:Paenglab)
	Comment 62 • 8 years ago

Comment on attachment 8816729 [details] [diff] [review]
WIP more options

I like the "Last opened one" in the prefs window. "Favorite Folder" can't be really compared because it's not an exclusive switch. It's a switch to add to the "Favorite Folder" view.

Three possibilities are too much now. The context menu entry is simpler and more direct than the checkbox in the properties dialog for the context menu users.

For the other users the pref window option is good because of the "Last used" option which isn't obvious in the other cases when none is ticked.

Like this we have for both fractions a solution and should remove the checkbox in the properties dialog.

	:aceman Assignee
	Comment 63 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #61)
> That said, there is a minor bug. If you select an address book in the
> context menu, the options dialogue doesn't update.

Thanks, it was a quick patch, now need to polish it. And add strings.
 
> Note that Thomas' patch also contains changed to mailing list access keys:
> +<!ENTITY ListDescription.accesskey      "D">
> +<!ENTITY AddressTitle.accesskey         "m">
> You might want to carry those forward.

Yes, as said my patch depends on Thomas' so all this stuff well be kept (except cutting out the checkbox in all properties dialogs if we so decide).

	:aceman Assignee
	Comment 64 • 8 years ago

Can you guys propose a good version of the string "Initially open this AB:" in Preferences ?

	Jorg K (CEST = GMT+2)
	Comment 65 • 8 years ago

"Initially show this address book:"
"Start-up address book:"
"Default address book:"

;-)

	Jorg K (CEST = GMT+2)
	Comment 66 • 8 years ago

Of course it should match the context menu, so perhaps: "Default address book" is best.

Which one is shown in the addressing side panel (which opens with F9)? The default one? Then there's a good reason to call this "default".

Wait, currently the side panel already remembers the last one that was opened. How does it do that? It's not opening to the default. Should that change?

	Richard Marti (:Paenglab)
	Comment 67 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #66)
> Of course it should match the context menu, so perhaps: "Default address
> book" is best.
> 
> Which one is shown in the addressing side panel (which opens with F9)? The
> default one? Then there's a good reason to call this "default".

"Default address book:" at this place sounds more like the AB where the new addresses should go. I'm more for the sentence from AB properties dialog and context menu: "Show as default". But in prefs window I think "Show as default Address Book" is more descriptive.

> Wait, currently the side panel already remembers the last one that was
> opened. How does it do that? It's not opening to the default. Should that
> change?

Thomas wrote already in a other bug (Thomas, you remember which?) that this is by design, so the user can access the last used addresses faster. What is the default on a new profile in the prefs for "Show as default Address Book"? I suppose (not tested yet) it's the last used. Then we are initially in par with the sidebar and the user can change the AB window as he likes.

	Thomas D. (:thomas8) Reporter
	Comment 68 • 8 years ago

(In reply to Richard Marti (:Paenglab) from comment #67)
> Thomas wrote already in a other bug (Thomas, you remember which?) that this
> is by design, so the user can access the last used addresses faster.

Yes. I explained in Bug 1142705 Comment 32, why contacts side bar needs a more sophisticated approach including an x-mozilla-draft header, and also why contacts side bar defaults have very little to do with main AB defaults.
So please, can we stop involving contacts side bar here, which has nothing to do with this bug.
As a caveat, in that comment I was not aware of in-place checkboxes, nor mailing lists.
Also, someone on those bugs mentioned that for contacts side bar, we should implement a per-identity way of defaulting this, which is true. And Aceman said it's doable.

> What is
> the default on a new profile in the prefs for "Show as default Address
> Book"? I suppose (not tested yet) it's the last used. Then we are initially
> in par with the sidebar and the user can change the AB window as he likes.

No. There are only two candidates for the factory "Show as default" AB:
- All Address Books
- Personal Address Book
In current TB, as in my patch, All Address Books is the default startup AB for main AB. Unless someone says why we shouldn't, we want to start out with a stable default AB, plus getting the last used from contacts side bar would probably require transition code which is totally unnecessary. It also doesn't make sense for the main AB to start on some random last used from contacts sidebar; again, contacts side bar and main AB are two quite different animals when it comes to defaults.
Between those two, I think keeping the current factory default of "All Address Books" makes sense:
- We don't want to change the default yet again, and have had user comment from Anje on behalf of her users against that.
- All Address Books makes a good, top-level, neutral start for everyone, and allows convenient cross-AB searches. Anyone who is not happy with that will try to change it to what they really need, which is desirable. If we succeed to land this, it must go into release notes.
- There's nothing so special about "Personal Address Book" although it's our second default whenever we want to write new addresses; what about "Collected Addresses"? Unexperienced users may not have sufficient awareness of "Collected Addresses", so it's better to start with a global search view of All Addresses. Which also helps to raise awareness of duplicate entries, which with our flawed general design are much likely to happen.

	Thomas D. (:thomas8) Reporter
	Comment 69 • 8 years ago

I totally dislike the last-minute flip-flopping and hectic redesign of my well-thought out and very versatile solution for this bug, for weak reasons which I think are just personal taste.

Before starting this bug, I examined the possibility of using context menu:
(In reply to Thomas D. (needinfo?me) from Bug 1177706 Comment 24)
> Details and Evaluation of possible solutions
> 
> 1) Allowing users to freely set their personally preferred initial default
> view is obviously the most versatile and sustainable solution. Possible
> implementations (not mutually exclusive):
> - A checkbox in AB properties dialogue (right-click on AB > Properties): E.g.
> [ ] Initially show this address book   OR   [ ] Initial default view   OR  
> [ ] Show as initial default
> (minimal implementation, and imo pretty good!)
> - A context menu entry (tempting, but imo not very good as it's rarely used,
> hence clutter)
> - A dropdown listbox option in Tools > Options (> Composition > Addressing)
> (Full implementation, most versatile)

Context menu: "Tempting, but... " Iow, I was deliberately NOT exposing it on the context menu because I thought it's not used often enough to be there. But I might be wrong. Context menu is nice because it's simple and exposed. We actively offer this feature to our users and when it's there, they can change it at any time, even more often if needed.
I'm surprised at the variation in UX opinions here, Richard saying don't expose it at all, too rarely used to be in Properties (which itself is also rarely used), then now per Jörg's initiative we go for maximal exposure. But I'm fine with that and it's ux-consistent with things like favorite folders and Windows default printer (sounds far-fetched, but isn't).

I have advocated for a central option in the past because I was not yet aware of the in-place ways of doing this. I'm now wondering if we really need the central option.

I'm *against* the implementation of the *central option as currently seen in Aceman's patch*, for these reasons:

1) Ace's patch drops the Mailing Lists support...
...which was fully realized in my patch (to allow mailing lists as startup views just like any other item in the main directory tree). Initially questioned by Richard, it was then successfully established as a worthwhile option with ui-r+ in subsequent discussion. For details, pls see my Comment 17. It's dropped because the ABList-Dropdown in options does not (yet) list the mailing lists, so if we want to keep the central option as-is, mailing lists are out. However, not supporting mailing lists is a needless clipping of wings for this feature, and also violating ux-consistency. Mailing lists are top-level items in Dir-Tree just like any other AB, they were put there for a reason, and there's no reason to treat them like second-class citizens.
Note that without mailing list support you'll actually have to disable the context menu for mailing lists, and it'll be surprising to users that while we remember mailing lists in "remember-last-used" mode, they can't pick them as a stable default. Not remembering mailing lists is also bad because if I exit with a mailing list shown, I'll expect that same view to come back, and not some other parent AB.

2) The central option is in the wrong place (Composition > Addressing)...
..., and it will be the first and only AB option to end up there. AB Options are few (it has not been designed to be customizable), but generally AB options are realized in-place, like First/Last settings, or AB properties. The place is wrong because Composition > Addressing is much more likely to be associated with Contacts Side Bar, so very soon we'll get user complaints why they set their favourite AB in options but it didn't come out in the sidebar. If we want to start AB options in options, then let them have their own dedicated panel. The twist being that the option which could really go here, about contacts side bar, will NOT end up here, but ultimately in account identities.

3) How can a hidden central option help discovery?
Given 2), I don't share the somewhat absurd notion that hiding a central option somewhere deep-down in the wrong place of options conglomerate will actually assist with ux-discovery of the "Remember last-used feature". To have that hidden central option or not imo will not make any difference.

4) Do we really need to duplicate the UI in options?
I wonder if an interested user cannot figure this out by himself, like this:
"I hate it that Thunderbird always returns to "All Address Books" at every AB startup. I'd much prefer to remember my last used AB. How can I stop that annoying behaviour?" Then, user finds our context menu option, now nicely exposed thanks to Jörg's wicked plan ;P "Ahhh, here we are, damn, let me switch this devil off." Yeah, and that's it. Since we don't offer any other options, user will probably test and just realize that it works now as expected, remembering the last used dir. Also if you think about it, if there's exactly ONE "Show as default" dir (and obviously it can't be more than one), and if you disable that as a default, well, that means there's no default, and no default (in the most simple and literal sense), can only mean to remember the last used. I'm not 100% sure about this, but I think it's totally safe to try it and see how it goes, rather than rushing into half-baked central options which are also feature-incomplete (wrt mailing lists).

Bottom line, I think the better solution is to drop the central option and just go with context menus as suggested by Jörg.

	Thomas D. (:thomas8) Reporter
	Comment 70 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #69)
> Bottom line, I think the better solution is to drop the central option and
> just go with context menus as suggested by Jörg.

Otherwise, I maintain there was nothing wrong with the solution which I proposed, checkbox in properties. I don't think there's any maintenance burden as Aceman claims, we just add it once and then it stays around. I'm also surprised again that suddenly we speak of maintenance burden whilst at other times people are saying AB is doomed so who cares...

	Thomas D. (:thomas8) Reporter
	Comment 71 • 8 years ago

Comment on attachment 8816729 [details] [diff] [review]
WIP more options

Feedback- because I think the central option is half-baked and the wrong way to go, also dropping mailing list support. For details, please see my extensive Comment 69.

It's better to only use Jörg's context menus without central option, or to use my existing patch with checkbox in properties. Both approaches will also secure mailing list support.

As for variable and function names, please avoid just using "default" only. We can get into much trouble lateron if we add other types of default, e.g. for creating new Addresses, for contacts side bar etc. This is about "Show as default" or "Startup view default", and sometimes only about "Startup URI", so variables and functions should be named accordingly, with much more precision than in the patch (I'm aware it was just WIP, so no blame).

	Jorg K (CEST = GMT+2)
	Comment 72 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #69)
> I totally dislike the last-minute flip-flopping and hectic redesign of my
> well-thought out and very versatile solution for this bug, for weak reasons
> which I think are just personal taste.
Thomas, we all dislike last-minute changes. You presented a semi-working patch v2.1 on Nov. 15, 2016. One day after the branch date and definite deadline.

Applying standard procedure, this bug and all the other calendar bugs should NOT be uplifted at all. For TB 52 ESR we have a late string date, mainly for a security bug which *must* land on ESR 52, nothing else. That bug should be my main focus, not the address book bugs.

We're all just trying to lift your work over the threshold. If you prefer, we will not try to get this in and then we have all the time of the world to discuss it.

I repeat: You are getting very very very special treatment here. All other contributors are just told: Sorry, you missed the deadline. And here you have two principal developers tied down with your bugs, one working at 3 AM.

T/D from comment #47:
> 7) Having said which, I'm not totally against having that
> option in options. But I would rather have it *in addition*
> to the primary properties UI checkbox.

T/D from comment #69
> I'm *against* the implementation of the *central option as currently
> seen in Aceman's patch*, for these reasons ...

Sorry, I don't have time to read all the details, but this seems to be a contradiction.

	Mitch in Oakland
	Comment 73 • 8 years ago

NONSENSE, THOMAS! 
NEED I REPEAT MYSELF -- AND CORRECT SOME MIS-STATEMENTS IN THE PROCESS?

In my experience (as a longtime Mac user, for whom "right-click" doesn't even exist), CONTEXTUAL menus are essentially HIDDEN menus -- hence, basically of secondary or backup use (a hidden feature!), not the primary place I'd go to change the behavior or appearance of ANY application. 

If I want to change the behavior of an application, I know I can always go to "Preferences" (under that application's drop-down), and that's always where I'm accustomed to going. I don't go there often, but I know these settings are there, safe-and-sound (all in one place, not scattered in "contextual" or window-specific menus) when I need to change them.

In this instance, moreover, there's an obvious (and obviously logical and intuitive) Preference panel that's appropriate for just this sort of setting (via a check-box or pop-up):
THUNDERBIRD>PREFERENCES>COMPOSITION>ADDRESSING

There's plenty of space on that Preference panel to add a line with a check-box and a pop-up listing the various address books (from which the user can drag down and choose a preferred AB).

THIS IS THE PERFECT SPOT FOR CHOOSING AN ADDRESS BOOK PRIORITY. THOMAS'S POINT (2) IS SIMPLY FALSE.

There are currently two other items on that "addressing" panel, and as it happens, they both DO involve Address books: one allows the user to choose the source of address autocompletion (with "Personal Address Book" a specified option).

The other setting on this panel (if checked) allows the user to choose an Address Book to which outgoing addresses will be automatically added (chosen from a pop-up list).

THIS NEW OPTION, IN FACT, WOULD LOOK AND ACT EXACTLY THE SAME WAY! HOW MUCH MORE APPROPRIATE COULD ONE BE?



Again, I appreciate all the work that Thomas D has put into this (despite even his Windows-centric bias) -- but now that I see his response (insisting on his initial proposal as having been so "well thought-out"), there appears to be some stubbornness or ego involved here. I don't need to be an expert coder to recognize that! ;-)

Again, let's just get this done already, and stop quibbling!

	Mitch in Oakland
	Comment 74 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #72)
> 
> We're all just trying to lift your work over the threshold. If you prefer,
> we will not try to get this in and then we have all the time of the world to
> discuss it.
> 
> I repeat: You are getting very very very special treatment here. All other
> contributors are just told: Sorry, you missed the deadline. And here you
> have two principal developers tied down with your bugs, one working at 3 AM.
> 
> T/D from comment #47:
> > 7) Having said which, I'm not totally against having that
> > option in options. But I would rather have it *in addition*
> > to the primary properties UI checkbox.
> 
> T/D from comment #69
> > I'm *against* the implementation of the *central option as currently
> > seen in Aceman's patch*, for these reasons ...

Jorg,

Please, just go with Thomas's earlier comment #47 (and Aceman's patch), and please just get this over with and in!

	Mitch in Oakland
	Comment 75 • 8 years ago

Hope I haven't spoken out of turn. BtW, this is why I hate politics!

	:aceman Assignee
	Comment 76 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #69)
> 1) Ace's patch drops the Mailing Lists support...

OK, I missed mailing lists, I'll try adding them to the list.

> 2) The central option is in the wrong place (Composition > Addressing)...
> ..., and it will be the first and only AB option to end up there. AB Options
> are few (it has not been designed to be customizable), but generally AB
> options are realized in-place, like First/Last settings, or AB properties.

Agreed, I also thought that this option does not belong into "composition". But I didn't want to create a new panel just for this single option. But if we agree on it, we can do that for TB53, I'd suggest a tab under "Display".

	Jorg K (CEST = GMT+2)
	Comment 77 • 8 years ago

(In reply to :aceman from comment #76)
> (In reply to Thomas D. (needinfo?me) from comment #69)
> > 1) Ace's patch drops the Mailing Lists support...
> OK, I missed mailing lists, I'll try adding them to the list.
You want a mailing list as initial item? Please DON'T!!!!
This is about opening an address book, not any of its content.

Please merge the patches, drop the stuff off the properties panel, fix the nits from your own comment #46 and fix the update bug from comment #61 and get this ready for review.

	Thomas D. (:thomas8) Reporter
	Comment 78 • 8 years ago

Sorry, too much nonsense, noise, and personal attacks here, so for now I need to move out of this. Also because it uses too much of my time and energy, plus some don't have time for reading and appreciating my inputs so we're starting to move in circles (e.g. wrt mailing lists).
Late or not, at least I've been the only one to start fixing this for our users, and my last patch was working correctly except for SeaMonkey. I'm not a SeaMonkey programmer, and it's easy for others to port. So now Jörg is complaining that he is tied with "my" bug while he himself has advocated to change it all over in the last minute, imo without a real need. Btw Aceman is not the only one to code at night, I haven't been doing anything else for the last couple of weeks day in day out, and nights, too, and surely, Jörg himself, too.

@Jörg, pls stop creating impressions about me as if I'm just keeping people busy for the sake of it, and stop downplaying my work. There's a difference between a "semi-working" patch and a single missing line which accidentally didn't get copied over. Not only am I working "very, very, very" hard to improve the poor UX in this area at least in some corners, I'm also delivering "very very very" special and good quality patches where the hardest part of the work, finding the rights spots to fix and viable ways of fixing, has already been done with great care and accuracy. More so given the fact that I'm not a professional programmer. It's always easy to come up with something better when a working prototype is already there, and a way into the jungle has been made, but a very very big chunk of the work is actually in the prototype. Don't get me wrong, I know that all of us are working very very very hard, but we're in this together...

@Mitch: Pls read this:
http://www.wikihow.com/Right-Click-on-a-Mac (4 ways to right-click on a MAC)
https://developer.apple.com/library/prerelease/content/documentation/UserExperience/Conceptual/OSXHIGuidelines/ContextualMenus.html
Contextual menus form a regular part of OSXHI good practice, providing shortcuts in context.
For right-click on Mac, just hold Ctrl - no problem.
But you are right that a contextual menu should never be the *only* way of access. Which doesn't mean it must be central options, but there must be another, regular way of access. So in that respect, I was wrong in my comment 69, point 4).

@Aceman, pls ensure my authorship/co-authorship of this patch gets correctly acknowledged upon landing. Use Duellmann if needed. Thanks everyone for their valuable time to cooperate on this.

	Mitch in Oakland
	Comment 79 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #78)
>
> @Mitch: Pls read this:
> http://www.wikihow.com/Right-Click-on-a-Mac (4 ways to right-click on a MAC)
> https://developer.apple.com/library/prerelease/content/documentation/
> UserExperience/Conceptual/OSXHIGuidelines/ContextualMenus.html
> Contextual menus form a regular part of OSXHI good practice, providing
> shortcuts in context.
> For right-click on Mac, just hold Ctrl - no problem.

Perhaps I exaggerated - or maybe I'm just old-fashioned. I know contextual menus exist on a Mac - but one has to know they're there in order to find them; in that sense, they're obscure compared to the Preference panels, which collate all controls in one place rather than scattering them throughout the UI.

(Then again - I'm old-fashioned enough to still be using OS 10.6.8, since I don't want to lose Adobe Creative Suite v5 or be forced to "update" [at a stiff fee] and become beholden to Adobe's cloud; ditto for MS Office and Toast, among others. I realize I'll need to bite the bullet for security's sake, but IMO some "improvements" merely complicate life as they add unnecessary bells and whistles. And don't get me started about the degradation of Google Maps!) ;-)

I still take issue with your comment 69 point 2, as I explained (regarding the appropriatness of the "addressing" panel, which could plausibly be renamed "addresses" if need be). Nonetheless, I'm reluctant to place further demands on your time, and I seem outnumbered here by others far more knowledgeable and experienced than myself.

Again, I appreciate your work and intended no offense by promoting my concerns. I can live with the fix as you've crafted it, and at this point will be happiest to see it implemented soon.

	Mitch in Oakland
	Comment 80 • 8 years ago

(In reply to Thomas D. (needinfo?me) from comment #78)
>
> @Mitch: Pls read this:
> https://developer.apple.com/library/prerelease/content/documentation/

"Always ensure that contextual menu items are also available as menu commands. A contextual menu is hidden by default and a user might not know it exists, so it should never be the only way to access a command. In particular, you want to avoid using a contextual menu as the only way to access an advanced or power-user feature."

Couldn't have said it better myself!

	:aceman Assignee
	Comment 81 • 8 years ago

This is my proposal. This reduces Thomas' patch to the backend handling of the prefs and displaying the right item when opening the AB window. Also keeps the accesskey fixes in the maillist dialog (for SM and TB). The UI will be in my separate patch.

Jorg, please just check if all still works and I addressed my own review comments ;)

FRG, please check the SM parts.

	:aceman Assignee
	Comment 82 • 8 years ago

This is the UI part for the feature. Adds the AB/ML selection to the Options/Preferences window and to the context menu in the AB window AB tree.

Jorg's hawk-eye-induced feature: If you have the Options window open and you change the default AB via the context menu the menulist in Options window updates too.

	Jorg K (CEST = GMT+2)
	Comment 83 • 8 years ago

Comment on attachment 8816814 [details] [diff] [review]
patch v5 - backend

Still seems to work ;-)
Please remove the unneeded "Show as default" from SM.

	:aceman Assignee
	Comment 84 • 8 years ago

Thanks.

	:aceman Assignee
	Comment 85 • 8 years ago

The setting in Options->Composition->Addressing is also requested in bug 117706, comment 21 and comment 23.

	Jorg K (CEST = GMT+2)
	Comment 86 • 8 years ago

Comment on attachment 8816815 [details] [diff] [review]
patch v5 - UI

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

Works for me.

::: mail/locales/en-US/chrome/messenger/preferences/compose.dtd
@@ +53,5 @@
>  <!ENTITY directories.accesskey                 "D">
>  <!ENTITY directoriesNone.label                 "None">
>  <!ENTITY editDirectories.label                 "Edit Directories…">
>  <!ENTITY editDirectories.accesskey             "E">
> +<!ENTITY makeDefault.label                     "Show as default address book/list:">

Richard, this is the label for the preference. Perhaps make it:
Show as default address book or mailing list:
What do you think?

::: mailnews/addrbook/content/addrbookWidgets.xml
@@ +31,2 @@
>                this._directories.push(ab);
> +              if (this.getAttribute("mailinglists") == "true") {

Aha, new magic here.

	:aceman Assignee
	Comment 87 • 8 years ago

Fixes the case that I forgot to set 'inited' member to true anywhere. And also handles the case where the startupURI pref contains an AB that is no longer valid. Was showing a blank menulist, now shows the 'last opened one' item.

	Thomas D. (:thomas8) Reporter
	Comment 88 • 8 years ago

Comment on attachment 8816827 [details] [diff] [review]
patch v5.1 - UI

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

Thank you for working on this.

Interesting twist to realize this as a command, but doesn't this update itself far more often than necessary? Why not have it in the onPopupShowing function of the context menu?

In the future, we might have other types of "default", so using generic default for a specific default isn't wise. This is about the default startup view directory (not addressbook, because we have mailing lists, too), so variables, functions, and IDs should somehow reflect that.
I propose using more specific variable and function names and IDs.

I also offer alternative strings:
> Context Menu: "Default Startup Directory"
> Options:      "Default startup directory for the main address book"

::: mail/components/addrbook/content/abCommon.js
@@ +44,5 @@
>        case "cmd_selectAll":
>        case "cmd_delete":
>        case "button_delete":
>        case "cmd_properties":
> +      case "cmd_abMakeDefault":

Cmd name and functions derived are misleading. Toggle, not make, because make == set to true. Default what? Will confuse later if we add other defaults.

cmd_abToggleStartupDirDefault

@@ +136,5 @@
>          goSetLabelAccesskeyTooltiptext("cmd_properties-menu",
>            labelAttr, accKeyAttr);
>          return (selectedDir != null);
>        }
> +      case "cmd_abMakeDefault": {

change cmd name, see above.

@@ +137,5 @@
>            labelAttr, accKeyAttr);
>          return (selectedDir != null);
>        }
> +      case "cmd_abMakeDefault": {
> +        let defaultItem = document.getElementById("dirTreeContext-default");

As a favor to posterity, when we might add other types of defaults, pls be more specific about "default".

let startupDefaultItem = document.getElementById("dirTreeContext-startupDirDefault");

@@ +140,5 @@
> +      case "cmd_abMakeDefault": {
> +        let defaultItem = document.getElementById("dirTreeContext-default");
> +        if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +          let selectedDirURI = getSelectedDirectoryURI();
> +          defaultItem.setAttribute("checked", (Services.prefs.getCharPref("mail.addr_book.view.startupURI") == selectedDirURI));

change item var again

this is hard to read; why only one varible and not for both sides of comparison, and why variable for the less complicated call and not for the more complicated call?

let startupURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
let selectedDirURI = getSelectedDirectoryURI();
startupDefaultItem.setAttribute("checked", startupURI == selectedDirURI);

Or with one variable, much more readable than before:
let startupURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
startupDefaultItem.setAttribute("checked", startupURI == getSelectedDirURI());

As a sidenote, can't we use this to shorten those giant pref calls:
https://dxr.mozilla.org/comm-central/source/mozilla/devtools/client/shared/shim/Services.js#84

@@ +142,5 @@
> +        if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +          let selectedDirURI = getSelectedDirectoryURI();
> +          defaultItem.setAttribute("checked", (Services.prefs.getCharPref("mail.addr_book.view.startupURI") == selectedDirURI));
> +        } else {
> +          defaultItem.setAttribute("checked", "false");

item name, dito

@@ +171,5 @@
>        case "cmd_properties":
>          AbEditSelectedDirectory();
>          break;
> +      case "cmd_abMakeDefault":
> +        AbMakeSelectedDefault();

change cmd name, and function name:
abToggleSelectedDirStartupDefault()

@@ +231,5 @@
>                        {selectedDirectory: selectedDir});
>    }
>  }
>  
> +function AbMakeSelectedDefault()

function abToggleSelectedDirStartupDefault()

fyi I understand the new style is to start any function and var names with small letter, so capitalized function names in AB are legacy

@@ +237,5 @@
> +  let selectedDirURI = getSelectedDirectoryURI();
> +  if (!selectedDirURI)
> +    return;
> +
> +  let haveDefault = Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault");

isDefault sounds better to me, but anyway...When we have a default at all, it means that the current startupURI IS the default, so it's just the same.

@@ +238,5 @@
> +  if (!selectedDirURI)
> +    return;
> +
> +  let haveDefault = Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault");
> +  let defaultURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");

This by itself is NOT the defaultURI, it's startupURI, which becomes defaultURI only if combined with "isDefault".

let startupURI = ...

@@ +240,5 @@
> +
> +  let haveDefault = Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault");
> +  let defaultURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
> +
> +  if (haveDefault && defaultURI == selectedDirURI) {

if (isDefault && startupURI == selectedDirURI) {

@@ +242,5 @@
> +  let defaultURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
> +
> +  if (haveDefault && defaultURI == selectedDirURI) {
> +    // The current AB was the default, now turn it off.
> +    // So here's no default startup view directory any more.

// The current directory has been the default startup view directory;
// toggle that off now. So there's no default startup view directory any more.

@@ +246,5 @@
> +    // So here's no default startup view directory any more.
> +    Services.prefs.setBoolPref("mail.addr_book.view.startupURIisDefault", false);
> +  } else {
> +      // The current addressbook will now be the default view
> +      // when starting up the main AB window.

directory, not addressbook (wrt mailing lists)

@@ +252,5 @@
> +      Services.prefs.setBoolPref("mail.addr_book.view.startupURIisDefault", true);
> +  }
> +
> +  // Update the checkbox in the menuitem.
> +  goUpdateCommand('cmd_abMakeDefault');

cmd name

::: mail/components/addrbook/content/addressbook.js
@@ +291,5 @@
>  {
>    goUpdateCommand('cmd_delete');
>    goUpdateCommand('button_delete');
>    goUpdateCommand('cmd_properties');
> +  goUpdateCommand('cmd_abMakeDefault');

cmd name

::: mail/components/addrbook/content/addressbook.xul
@@ +209,5 @@
>    <menuitem id="dirTreeContext-newlist"
>              label="&newlistButton.label;"
>              accesskey="&newlistButton.accesskey;"
>              command="cmd_newlist"/>
> +  <menuitem id="dirTreeContext-default"

change id, dito

@@ +210,5 @@
>              label="&newlistButton.label;"
>              accesskey="&newlistButton.accesskey;"
>              command="cmd_newlist"/>
> +  <menuitem id="dirTreeContext-default"
> +            label="&makeDefault.label;"

Label names should normally represent their string, or at least precise function:

label="&showAsDefault.label;"

::: mail/components/preferences/compose.js
@@ +25,5 @@
>      this.updateAttachmentCheck();
>  
>      this.updateEmailCollection();
>  
> +    this.initDefaultAB();

In one years time, nobody will know what type of default this was...

this.initAbDefaultStartupDir()

@@ +117,5 @@
>                          "editDirectories", "chrome,modal=yes,resizable=no", null);
>      }
>    },
>  
> +  initDefaultAB: function() {

initAbDefaultStartupDir: function () {

@@ +121,5 @@
> +  initDefaultAB: function() {
> +    if (!this.defaultABListener.inited)
> +      this.defaultABListener.load();
> +
> +    let abList = document.getElementById("defaultAbList");

ABs and mailing lists are directories, so:

let dirList = document.getElementById("defaultStartupDirList");

@@ +123,5 @@
> +      this.defaultABListener.load();
> +
> +    let abList = document.getElementById("defaultAbList");
> +    if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +      // Some addressbook is the default

directory, not AB

@@ +124,5 @@
> +
> +    let abList = document.getElementById("defaultAbList");
> +    if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +      // Some addressbook is the default
> +      let defaultURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");

let defaultStartupURI =

@@ +125,5 @@
> +    let abList = document.getElementById("defaultAbList");
> +    if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +      // Some addressbook is the default
> +      let defaultURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
> +      let abItem = abList.menupopup.querySelector('[value="' + defaultURI + '"]');

let defaultDirListItem =

@@ +129,5 @@
> +      let abItem = abList.menupopup.querySelector('[value="' + defaultURI + '"]');
> +      // It may happen the stored URI is not in the list.
> +      // In that case select the "none" value and let the AB code clear out
> +      // the invalid value. Unless the user selects something here.
> +      if (abItem)

defaultDirListItem

@@ +130,5 @@
> +      // It may happen the stored URI is not in the list.
> +      // In that case select the "none" value and let the AB code clear out
> +      // the invalid value. Unless the user selects something here.
> +      if (abItem)
> +        abList.selectedItem = abItem;

dirList 3x going down

@@ +134,5 @@
> +        abList.selectedItem = abItem;
> +      else
> +        abList.value = "";
> +    } else {
> +      // Set pref that there's no default startup view directory any more.

You're not setting the pref here, so where and how does it get set?

// No default startup view directory any more, we'll update the respective pref later

@@ +139,5 @@
> +      abList.value = "";
> +    }
> +  },
> +
> +  setDefaultAB: function(aDirURI) {

setDefaultStartupDir: function(aDirURI) {

@@ +141,5 @@
> +  },
> +
> +  setDefaultAB: function(aDirURI) {
> +    if (aDirURI) {
> +      // Some addressbook was selected. Set prefs to make this directory

// Some AB directory was selected. ...

@@ +233,5 @@
>         document.getElementById('msgcompose.background_color').reset();
>       } catch (ex) {}
> +  },
> +
> +  defaultABListener: {

This is NOT the general default AB. It's the default startup view directory.

defaultStartupDirListener: {

@@ +242,5 @@
> +        return;
> +
> +      // If the default AB prefs have changed,
> +      // reinitialize the default AB picker to see the new value.
> +      gComposePane.initDefaultAB();

gComposePane.initAbDefaultStartupDir();

// If the default startup directory prefs have changed,
// reinitialize the default startup dir picker to show the new value.

::: mail/components/preferences/compose.xul
@@ +220,5 @@
>                </menulist>
>              </hbox>
> +
> +           <hbox align="center" pack="start">
> +             <label value="&makeDefault.label;" accesskey="&makeDefault.accesskey;"/>

&showAsDefault.label;      &showAsDefault.accesskey;

@@ +221,5 @@
>              </hbox>
> +
> +           <hbox align="center" pack="start">
> +             <label value="&makeDefault.label;" accesskey="&makeDefault.accesskey;"/>
> +             <menulist id="defaultAbList" flex="1"

id="defaultStartupDirList"

@@ +222,5 @@
> +
> +           <hbox align="center" pack="start">
> +             <label value="&makeDefault.label;" accesskey="&makeDefault.accesskey;"/>
> +             <menulist id="defaultAbList" flex="1"
> +                       oncommand="gComposePane.setDefaultAB(this.value);">

.setDefaultStartupDir(this.value);">

::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
@@ +139,5 @@
>  <!ENTITY newContactButton.label                         "New Contact">
>  <!ENTITY newContactButton.accesskey                     "C">
>  <!ENTITY newlistButton.label                            "New List">
>  <!ENTITY newlistButton.accesskey                        "L">
> +<!ENTITY makeDefault.label                              "Show as default">

This is short and verbal, which is good, but maybe still not very clear. Show as default for what, and when? "Als Standard anzeigen": Wann und wo?

How about this?

Default Startup Directory
Standard-Startverzeichnis

Directorio initiale de defaulto (wild guessing how it could look in other languages...)

I think that's quite concise and unambiguous.
It's about which directory gets shown upon AB startup, by default.
So here we have all three aspects:
Default + Startup + Directory
We're in the address book, so I think it can be understood that this is a default startup directory for address book and not some other entity.
Also in the title of this bug, we have "set a *directory* as *default startup* view", so it seems to be a term which keeps coming naturally.

I will also suggest a similar label for the main central option, see below. It would be good to keep them in sync, so they can be identified as the same thing.

::: mail/locales/en-US/chrome/messenger/preferences/compose.dtd
@@ +53,5 @@
>  <!ENTITY directories.accesskey                 "D">
>  <!ENTITY directoriesNone.label                 "None">
>  <!ENTITY editDirectories.label                 "Edit Directories…">
>  <!ENTITY editDirectories.accesskey             "E">
> +<!ENTITY makeDefault.label                     "Show as default address book/list:">

Label names:
abStartupDir.label
abStartupDir.accesskey

This could be shortened to
"Show as default directory:"
"Als Standardverzeichnis anzeigen:"

However, this label is highly misleading. Remember we are in
Options > Composition > Addressing (or aren't we?)
And in composition UI, contacts side bar is the only directory around. If we just talk about a default Ab here, users can expect to change contacts sidebar default here. As this pref is in the wrong place, at least we must make it clear what this is about. Which is not easy.

We could go back to the beginning of this bug, and try:

> Show as default when starting the main address book:
> Beim Starten des Adddressbuchs als Standard anzeigen:

... or variants thereof:

> When starting the main address book, show:
> Beim Starten des Addressbuchs anzeigen:

But those verbal variants are basically phrases which include the selected item in the list as an object. Which may not work well for other languages, because it might require object case/declination or articles on the following items in selector list, even in German it's already odd in terms of word order...

So I think maybe next is better, using noun style...

> Default startup directory for the main address book:
> [Default Startup Dir List       v]

Standard-Startverzeichnis für das Addressbuch:
[Auswahlliste                   v]

Nouns are less likely to sound odd if combined with the selected item name, because it's now a label, not a phrase including the selected item, so this sounds right.
There's enough space, so we can use two rows, one for the label, one for the selector.
Plus we would keep in sync with my suggested context menuitem label,

> Context Menu: "Default Startup Directory"
> Options:      "Default startup directory for the main address book"

@@ +55,5 @@
>  <!ENTITY editDirectories.label                 "Edit Directories…">
>  <!ENTITY editDirectories.accesskey             "E">
> +<!ENTITY makeDefault.label                     "Show as default address book/list:">
> +<!ENTITY makeDefault.accesskey                 "S">
> +<!ENTITY makeDefaultLast.label                 "last opened one">

Isn't this the classical MRU?

"Most recently used directory"
"Zuletzt verwendetes Verzeichnis"

::: mailnews/addrbook/content/addrbookWidgets.xml
@@ +194,5 @@
>  
>            if (b.URI == kCollectedAddressbookURI)
>              return -1;
>  
> +          // Mailinglists do not have their own type, sort them to the bottom.

Hmmm. Does that really work?
Mailing Lists are children of their parent ABs.
So there can be mailing lists with same name in different ABs.
If you sort all mailing lists at the bottom, context is lost, then they are indistinguishable.
Can't you sort mailing lists under the containing AB?
Even to append something in front to show that this is a mailing list inside the parent AB?

Address Book 1
⤷ ListName1
⤷ ListName2
Address Book 2
⤷ ListName1
⤷ ListName2

	Thomas D. (:thomas8) Reporter
	Comment 89 • 8 years ago

Correction:

> This is NOT the general default AB. It's the default startup view directory.
> defaultStartupDirListener: {

Better: defaultAbStartupDirListener: {

	Jorg K (CEST = GMT+2)
	Comment 90 • 8 years ago

Comment on attachment 8816827 [details] [diff] [review]
patch v5.1 - UI

Aceman, thanks for all your hard work on this. It seems that there will be more changes coming, so I'm creating the review for now (since the red indicator is irritating me ;-)).

	Jorg K (CEST = GMT+2)
	Comment 91 • 8 years ago

Clearly missed all deadlines. Not tracking any more.

	Richard Marti (:Paenglab)
	Comment 92 • 8 years ago

Comment on attachment 8816827 [details] [diff] [review]
patch v5.1 - UI

This looks good.

I think, the mailing list should have the possibility to set as default. This only because unfortunately the AB window treats the mailing lists like a normal AB.

Could you add a "Show as default" entry in AB main menu at "Edit" directly above the Properties like we have for the Favorite folders? Then also users which are used to use this part have access to it.

The "Last used" entry (I think this works too because this is synonymous to MRU and reads less technical) is important at the place in central prefs because with the menus it is not obvious what happens when no AB is ticked.

Already multiple times reported in this bug and I thought it exists already a bug but couldn't find one. The problem is, default ABs aren't shown when they are in a collapsed tree. Ace, you're such a genius, maybe can solve this in this bug.

	Jorg K (CEST = GMT+2)
	Comment 93 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #86)
> > +<!ENTITY makeDefault.label                     "Show as default address book/list:">
> Richard, this is the label for the preference. Perhaps make it:
> Show as default address book or mailing list:
> What do you think?

Thomas suggested: "Show as default directory:" but a m/l is not a directory, or is it? At least I don't think it's intended as one.

I'm OK with "Last used", good idea.

	Richard Marti (:Paenglab)
	Comment 94 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #93)
> (In reply to Jorg K (GMT+1) from comment #86)
> > > +<!ENTITY makeDefault.label                     "Show as default address book/list:">
> > Richard, this is the label for the preference. Perhaps make it:
> > Show as default address book or mailing list:
> > What do you think?
> 
> Thomas suggested: "Show as default directory:" but a m/l is not a directory,
> or is it? At least I don't think it's intended as one.

We use normally directory only for LDAP. LDAP is also a AB, but the protocol names it a directory. So "Show as default address book/list:" would be okay for me.

	Thomas D. (:thomas8) Reporter
	Comment 95 • 8 years ago

(In reply to Jorg K (GMT+1) from comment #93)
> (In reply to Jorg K (GMT+1) from comment #86)
> > > +<!ENTITY makeDefault.label                     "Show as default address book/list:">
> > Richard, this is the label for the preference. Perhaps make it:
> > Show as default address book or mailing list:
> > What do you think?
> 
> Thomas suggested: "Show as default directory:" but a m/l is not a directory,
> or is it? At least I don't think it's intended as one.
> 
> I'm OK with "Last used", good idea.

"Last used" sounds good, "Last used directory" maybe more correct.
To my ears and notion so far, both ABs and mailing lists are directories.
For one, they both live in the directory tree. Also, the word directory (Verzeichnis) is much closer to mailing list than the word AB, as directories can be considered lists.
Plus, mailing lists are nothing but a selected data subset of address books, hence filtered directories. I do think we can get away with "directories" for both types...

More importantly, I did NOT ultimately suggest "Show as default directory:", I said that label is grossly misleading if placed on a "Composition" tab without further specification. Because Composition has contacts sidebar, so users can think this is about choosing a default directory for contacts sidebar. Hence my real suggestion was and is:

> Context Menu: "Default Startup Directory"
> Options:      "Default startup directory for the main address book"

If we remain with "Show as default", we must still add the target of the default:
"Show as default for the main address book"
It's not required to have the words "address book" or "mailing list" in the label, because the dropdown items are obviously address books and mailing lists, so what is shown is still clear:

Show as default for the main address book: Personal Address Book
Show as default for the main address book: My private mailing list
Als Standard im Haupt-Addressbuch anzeigen: Persönliches Addressbuch
Als Standard im Haupt-Addressbuch anzeige: Meine privater Verteiler

	Frank-Rainer Grahl (:frg)
	Comment 96 • 8 years ago

Comment on attachment 8816816 [details] [diff] [review]
patch v5.1 - backend

Tested and works for SeaMonkey.

	:aceman Assignee
	Comment 97 • 8 years ago

What is "main address book" ?

	Thomas D. (:thomas8) Reporter
	Comment 98 • 8 years ago

(In reply to :aceman from comment #97)
> What is "main address book" ?

The address book window called "Address Book", as opposed to any particular directory/AB in the tree, and as opposed to composition's contacts sidebar.

	:aceman Assignee
	Comment 99 • 8 years ago

Accommodated Thomas' proposals for variable/function/string renaming. The mailinglists in the directory picker in the Options can have the arrows preceding them. But I think that needs to be localizable, which would need a new .properties file to be created and imported into the mailWidgets.xml file. I'm not sure that is worth it. Any other proposals to mark the mailing lists? Is it maybe possible to put their name in italics?

	Richard Marti (:Paenglab)
	Comment 100 • 8 years ago

Comment on attachment 8817912 [details] [diff] [review]
patch v5.2 - UI

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

(In reply to :aceman from comment #99)
> Created attachment 8817912 [details] [diff] [review]
> patch v5.2 - UI
> 
> Accommodated Thomas' proposals for variable/function/string renaming. The
> mailinglists in the directory picker in the Options can have the arrows
> preceding them. But I think that needs to be localizable,

What about using the same icons we already use in AB window? The arrow isn't very obvious.

::: mail/locales/en-US/chrome/messenger/preferences/compose.dtd
@@ +55,5 @@
>  <!ENTITY editDirectories.label                 "Edit Directories…">
>  <!ENTITY editDirectories.accesskey             "E">
> +<!ENTITY showAsDefault.label                   "Default startup directory in the address book window:">
> +<!ENTITY showAsDefault.accesskey               "S">
> +<!ENTITY showAsDefaultLast.label               "most recently used directory">

Please use "Last Used Directory" like Thomas wrote in comment 95.

	Mitch in Oakland
	Comment 101 • 8 years ago

(In reply to :aceman from comment #99)
> Created attachment 8817912 [details] [diff] [review]
> patch v5.2 - UI
> 
> Any other proposals to mark the mailing
> lists? Is it maybe possible to put their name in italics?

I haven't seen the rest of the implementation yet, but would it be practical for the choice in the Preference (Options) Panel to be made from a pop-up window, where the selected mailing list or directory is the one that's visible in the window?:

"[ ] Show as default for address book window: < pop-up >"

This would replicate the appearance and functionality of the line that appears immediately below the window where I'm currently entering this comment:

"[ ] Need more information from < pop-up >"

It would also be consistent with the line

"[ ] Automatically add email addresses to my < pop-up >"

that already appears on the same Preference panel (Preferences>Composition>Addressing) on a Mac.

	Mitch in Oakland
	Comment 102 • 8 years ago

(As an amateur, I hope I've understood Aceman's question properly, and that the above suggestion proves helpful and relevant.)

	Jorg K (CEST = GMT+2)
	Comment 103 • 8 years ago

I'll review this after Thomas and Richard.

	:aceman Assignee
	Comment 104 • 8 years ago

Would this suffice? Or do we need some more involved css games? In the main addressbook the icons as put via css, but it is a tree and the items have properties (see abTrees.js). But the icons I use are the same as linked in the css. Except on OS X where some 2x versions are used.

	:aceman Assignee
	Comment 105 • 8 years ago

OK, another bit missing here. Do we want to be able to select "All addressbooks" as the default? It is NOT possible right now in the picker in Options, but it is in the addressbook window via the context menu.

	Jorg K (CEST = GMT+2)
	Comment 106 • 8 years ago

(In reply to :aceman from comment #105)
> OK, another bit missing here. Do we want to be able to select "All
> addressbooks" as the default?
Yes, we do, in fact, that should be the initial default.

	Richard Marti (:Paenglab)
	Comment 107 • 8 years ago

Your patch looks already good. Instead of hard coding the icons this patch uses classes and CSS. A issue, also with your patch, is that "Last used directory" doesn't get the menuitem-iconic class and is misaligned. It should also get it. My patch doesn't give the menulist a icon. But this is maybe better because they are not really needed in the prefs.

Aceman, when you like it more you can also use like aMenuNode.setAttribute("IsServer", aFolder.isServer);

I also chenged the "last used directory" to "Last used directory".

	Richard Marti (:Paenglab)
	Comment 108 • 8 years ago

Comment on attachment 8817921 [details] [diff] [review]
patch v5.3 - UI

I cancel the ui-r? to do it on your next version with css icons.

	:aceman Assignee
	Comment 109 • 8 years ago

Paenglab, thanks for the examples.
I have rewritten the css rules to use attributes, in the same way our folder picker (folderWidgets.xml) works.

This took a while as adding support for mailinglists and the icons required the rearchitecting of half of the addressbook picker (addrbookWidgets.xml). But I like it, more features in shared widgets are always useful :)

Frg, the UI is not implemented in SM, but please check the changes in mailnews/addrbook/content/addrbookWidgets.xml didn't break the AB picker in SM preferences.

	Thomas D. (:thomas8) Reporter
	Comment 110 • 8 years ago

Comment on attachment 8819624 [details] [diff] [review]
patch v5.4 - UI

I can neither test this nor review in detail, but from a quick glance I think this is going in the right direction. Great work, thank you.

A screenshot of options with the startup dir dropdown selector open would be nice.

Nit:
> <!ENTITY showAsDefaultLast.label               "last used directory">
> <!ENTITY showAsDefaultAll.label                "All addressbooks">

"Last used directory" must start with capital L.
"All Address Books" (that's a proper name, in a way, and that's what we use elsewhere)

	:aceman Assignee
	Comment 112 • 8 years ago

Ok, we can actually take the real "all address books" string from the addressbook.properties (which is used in the AB tree).

	Thomas D. (:thomas8) Reporter
	Comment 113 • 8 years ago

Comment on attachment 8820043 [details]
screenshot of the AB picker in action (linux)

Nice!!

	Jorg K (CEST = GMT+2)
	Comment 114 • 8 years ago

(In reply to :aceman from comment #111)
> Created attachment 8820043 [details]
> screenshot of the AB picker in action (linux)
This is a little confusing since the header "Last used directory" and the first entry "All Address Books" have the same style. No separator and no icon in front of "All Address Books".

I tested it and it works nicely, but would there be a chance to improve this part of the UI? An icon that symbolises multiple books would be ideal. Sorry, being pedantic again.

	Richard Marti (:Paenglab)
	Comment 115 • 8 years ago

Comment on attachment 8820044 [details] [diff] [review]
patch v5.5 - UI

Looks good

(In reply to Jorg K (GMT+1) from comment #114)
> (In reply to :aceman from comment #111)
> > Created attachment 8820043 [details]
> > screenshot of the AB picker in action (linux)
> This is a little confusing since the header "Last used directory" and the
> first entry "All Address Books" have the same style. No separator and no
> icon in front of "All Address Books".
> 
> I tested it and it works nicely, but would there be a chance to improve this
> part of the UI? An icon that symbolises multiple books would be ideal.
> Sorry, being pedantic again.

Agreed, but this can be done in a followup bug. "All Address Books" has already his own attribute, IsAllAB="true".

	:aceman Assignee
	Comment 116 • 8 years ago

Well, in the addressbook window, the All Address Book item has the same icon as a normal addressbook (one book). Should I do that?

Or in a followup bug you find some better icon (e.g. multiple books) for both places?

	Richard Marti (:Paenglab)
	Comment 117 • 8 years ago

I think we should try to give him  his own icon. Now also in addressbook window to be better remarkable. Maybe I need you in this followup bug to give the "All Address Book" also the IsAllAB attribute in addressbook window. Or does it have it already?

	:aceman Assignee
	Comment 118 • 8 years ago

I think there is no such property yet on the tree node, but I can add that when you make the bug.

	Frank-Rainer Grahl (:frg)
	Comment 119 • 8 years ago

Comment on attachment 8820044 [details] [diff] [review]
patch v5.5 - UI

Seeing no regression in SeaMonkey and something suite pick up later too. Thanks and r+

	Jorg K (CEST = GMT+2)
	Comment 120 • 8 years ago

(In reply to :aceman from comment #116)
> Well, in the addressbook window, the All Address Book item has the same icon
> as a normal addressbook (one book). Should I do that?
IMHO, yes, for now (before review).

> Or in a followup bug you find some better icon (e.g. multiple books) for
> both places?
Yes, that as well.

	Jorg K (CEST = GMT+2)
	Comment 122 • 8 years ago

Comment on attachment 8820821 [details] [diff] [review]
patch v5.6 - UI

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

Looks OK to me, but sadly for the widget stuff I'm not the right reviewer. I hope Magnus can add more value. I found some nits and I tested it.

Thanks a lot for picking this up and carrying it through to completion. Maybe we get it as a Christmas present ;-)

::: mail/components/preferences/compose.js
@@ +123,5 @@
> +      this.startupDirListener.load();
> +
> +    let dirList = document.getElementById("defaultStartupDirList");
> +    if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +      // Some directory is the default

Nit. Full stop.

@@ +126,5 @@
> +    if (Services.prefs.getBoolPref("mail.addr_book.view.startupURIisDefault")) {
> +      // Some directory is the default
> +      let startupURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
> +      let dirItem = dirList.menupopup.querySelector('[value="' + startupURI + '"]');
> +      // It may happen the stored URI is not in the list.

Nit: It can happen _that_ the ...

@@ +128,5 @@
> +      let startupURI = Services.prefs.getCharPref("mail.addr_book.view.startupURI");
> +      let dirItem = dirList.menupopup.querySelector('[value="' + startupURI + '"]');
> +      // It may happen the stored URI is not in the list.
> +      // In that case select the "none" value and let the AB code clear out
> +      // the invalid value. Unless the user selects something here.

Nit: ... value, unless ...

@@ +248,5 @@
> +    load: function() {
> +      // Observe changes of our prefs.
> +      Services.prefs.addObserver(this.domain, this, false);
> +      // Unload the pref observer when preferences window is closed.
> +      window.window.addEventListener("unload", this.unload, true);

This looks really odd: windows.window. ... although it seems to work. I added a dump before and after and no JS complaint ;-)
Can you explain what's going on here and maybe add a comment.

::: mailnews/addrbook/content/addrbookWidgets.xml
@@ +87,5 @@
> +              listItem.setAttribute("AddrBook", "true");
> +            if (ab.isRemote)
> +              listItem.setAttribute("IsRemote", "true");
> +            if (ab.isSecure)
> +              listItem.setAttribute("IsSecure", "true");

This is really hard to read. Can you use empty lines to separate the if/else from the independent if's. Or braces.

	:aceman Assignee
	Comment 123 • 8 years ago

Thanks, done. The double 'window' was redundant. It seems to point back to window. So I reduced it to single 'window' and it still works. Good catch.

	Jorg K (CEST = GMT+2)
	Comment 124 • 8 years ago

Comment on attachment 8821318 [details] [diff] [review]
patch v5.7 - UI

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

::: mail/components/preferences/compose.js
@@ +253,5 @@
> +      this.inited = true;
> +    },
> +
> +    unload: function(event) {
> +Components.utils.reportError("unload!");

Umm, did I ask for this being added?

	:aceman Assignee
	Comment 125 • 8 years ago

No, it is a bonus :)

	Jorg K (CEST = GMT+2)
	Comment 126 • 8 years ago

Comment on attachment 8821318 [details] [diff] [review]
patch v5.7 - UI

Please remember to remove the |Components.utils.reportError("unload!");|

	:aceman Assignee
	Comment 127 • 8 years ago

Drops the leftover reporting line.

	:aceman Assignee
	Comment 128 • 8 years ago

Thanks to all involved. This seems ready now.

	Jorg K (CEST = GMT+2)
	Comment 129 • 8 years ago

I'll land it after the next M-C merge which usually happens around 1-2 AM GMT, certainly ready for the next Daily so we can all enjoy the new goodness ;-)

	Jorg K (CEST = GMT+2)
	Comment 130 • 8 years ago

Taking off "checkin-needed" so I can take care of this personally in the morning, just to get the "ü" right, etc.

	Jorg K (CEST = GMT+2)
	Comment 131 • 8 years ago

https://hg.mozilla.org/comm-central/rev/0b370e34182fc18c65d098a50b35b76803d08203
https://hg.mozilla.org/comm-central/rev/68809f0c899a4bdd1dfeca59367b562759263ab2