1143812 - Always allow creating new contacts and lists, eg after opening addressbook window, when 'All Address Books' is selected, or when search field has focus (eg Ctrl+N fails on all of these)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

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

Always allow creating contacts and lists after opening the addressbook ('All Address Books' is selected) by using the first addressbook.

Comment 1

[Thomas D. (:thomas8)]

Good catch! By default, the first AB is "Personal AB" which is reasonable for creating new stuff. Plus, both dialogues have a very clear dropdown at the top showing the currently used AB where new stuff is created and allowing you to pick another target AB if you so wish.

ui-r+ for the idea :)

Comment 2

[:aceman]

Then I think we should explicitly choose Personal AB (it probably has a stable known ID), not just the first AB.

Comment 3

[:aceman]

Suyash, do you want to work on this or is it free to take? :)

Comment 4

[Suyash Agarwal (:sshagarwal)]

If you want you can work on it :)

Thanks.

Comment 5

[Wayne Mery (:wsmwk)]

This is fairly annoying. And a regression of bug 170270 in version 38.

Note - the first address book will not necessarily be PAB. The user may have reodered them.

Comment 6

[Wayne Mery (:wsmwk)]

Probably to late for 38.0, but if we should get delayed, perhaps not.

Comment 7

[Kent James (:rkent)]

We're not going to block on this, but we might take a patch later.

Comment 8

[:aceman]

What in the case if you have an LDAP AB selected? Should the New Contact be disabled, or also create in the Personal AB?

Comment 9

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #8)
> What in the case if you have an LDAP AB selected? Should the New Contact be
> disabled, or also create in the Personal AB?

Looking at bug 86405, LDAP ABs could be editable but we just don't offer that yet. Disabling would convey a clearer idea of that. However, as Wayne says, it's really annoying when you just want to create a new contact and you can't just because you happen to be on the wrong AB node.

So without any strong feelings about this, perhaps it's ok/good if we keep the current behaviour where "New Contact" is enabled even when LDAP dirs are selected. With the resulting, desirable UX that we just allow creating new contacts ALWAYS. As mentioned, there's a clear feedback/choice at the top of the dialogue in which AB the new contact will be created, so there can't be any doubts about that. And LDAP dirs don't feature in that list yet, so it's clear the new contact will NOT be on the LDAP.

(In reply to :aceman from comment #2)
> Then I think we should explicitly choose Personal AB (it probably has a
> stable known ID), not just the first AB.

+1. Without deeper thoughts, that looks reasonable and safe to me and probably should be done to avoid surprises when user has reordered ABs and puts some other AB in first position. In that case, if user wants to easily create entries in whatever is HIS first AB, he can just select that first AB before creating new entries, and it will be preselected for the creation. But in this bug, we want a safe fall-back AB which we know exists and is editable, especially for the frequent scenario when "All ABs" is selected.

Comment 10

[Wayne Mery (:wsmwk)]

I hope I am not conflating too many cases, by further change of the summary, but these all seem related.

Current behavior when ldap AB is selected, is new contact dialog selects PAB (or perhaps some other AB that is considered the default) ... and that should continue.

And more generally, if an address book has been selected but New contact is disabled because search field is focused, then new contact should work, with THAT address book specified.

Comment 11

[:aceman]

Well, if you have a search (filter) enabled and create a new card and it does not appear in the list because it does not match the search then that could confuse the user and make him create the card multiple times.

Comment 12

[Wayne Mery (:wsmwk)]

If I'm thinking of the same scenario, it already behaved that way before search all was implimented, i.e.  search does not automatically refresh the results after updating OR adding a contact.

Comment 13

[Thomas D. (:thomas8)]

I concur with Wayne that we should simply allow creating new contacts or mailing lists ALWAYS without exception, as requested in the bug summary.

(In reply to :aceman from comment #11)
> Well, if you have a search (filter) enabled and create a new card and it
> does not appear in the list because it does not match the search then that
> could confuse the user and make him create the card multiple times.

I see your point... but... if you can't remember and trust your own actions, or you can't trust Thunderbird to actually create a card after you confirm that, then we have bigger problems... Sometimes trying to avoid every little potential of confusion will create other UX failures in the process.

Not seeing everything can ALWAYS potentially irritate whenever filters are set. By analogy, so we shouldn't allow clicking "Get New Messages" when filter is set, because it might irritate the user that new messages do not seem to arrive? I'd argue the other way round:

If it's not clear enough from the UI that a filter is set, then we should change that. In bug 170270 attachment 8560983 [details], I've suggested a secondary filter bar (as we have it for message quick filter), aka "AB Scope Selector Bar", which only appears when there's an active contact filter:

                                                                [John         ]
+-----------------------------------------------------------------------------+
|Search in: [All Address Books v]                                             | 
+-----------------------------------------------------------------------------+
Result list of contacts...

Wrt search, here's another important scenario/workflow which actually *requires* enabling "New contact/list" even when search has focus and there's a filter term:

- search/filter for "John" to find "John Doe"
- realize he's not in your AB, you only have "John Miller", "Ben Johnson", et alii
- want to create a new contact for "John Doe" just now
-> curse TB because "New Contact" is disabled and you have to click around to enable it again

Iow, I'd consider it a frequent scenario that users might want to add new contacts *especially* just after checking via quick search if they already exist or not. So in that case, the filter terms would match the newly created contact, and it should appear in the search results. Not updating search results after creation of new contact is another bug again which needs to be fixed (I can't believe we don't have a bug for that, does anyone know? Otherwise it should be created...)

In a nutshell, "New Contact" (and by extension, "New List") is a crucial everyday action which the user might want to do at any unpredictable time as long as the AB is open for general address management, so it should never be disabled unless there are really compelling reasons. It's like the "New Message" button, a swiss knife tool which should always work no matter what.

Comment 14

[:aceman]

OK, I am working on it. Just that the full version is quite heavy and involves moving command controllers around and when only half of the window code is shared with Seamonkey, breakage is likely.

So I'll create a simple version for TB38 with just the most important fixes.

Comment 15

[:aceman]

Attachment: patch

This works for me in the AB window, unless the focus is inside the search window. That will be solved in the full version for trunk.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8613712 [details] [diff] [review]
patch

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

::: mail/components/addrbook/content/addressbook.js
@@ +213,5 @@
>  
>    // Focus the searchbox as we think the user will want to do that
>    // with the highest probability.
> +  // This is disabled for now to keep the New Contact command enabled.
> +  // QuickSearchFocus();

why is it ever disable?

Comment 17

[:aceman]

What do you mean?

Comment 18

[:aceman]

If you ask why is New Contact disabled when the search box has the focus then it is because the controller governing the command is attached to the AB list and the results pane. If neither of thos 2 is focused, the command gets disabled. I'll shuffle the command controllers in the full patch for trunk.

Comment 19

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #18)
> If you ask why is New Contact disabled when the search box has the focus
> then it is because the controller governing the command is attached to the
> AB list and the results pane. If neither of thos 2 is focused, the command
> gets disabled. I'll shuffle the command controllers in the full patch for
> trunk.

How hard would it be to hack this so that the New Contact is always enabled? Like just never disable it?
As I've demonstrated above, it's quite a nasty regression because there's a high chance users will need the command exactly after performing a failed search for existing contacts...

Comment 20

[:aceman]

(In reply to Thomas D. from comment #19)
> How hard would it be to hack this so that the New Contact is always enabled?
> Like just never disable it?
> As I've demonstrated above, it's quite a nasty regression because there's a
> high chance users will need the command exactly after performing a failed
> search for existing contacts...

As said in comment 14, it is doable, but it also needs some synchronization with Seamonkey. And it isn't exactly trivial so I would push that kind of patch into TB38 as it could introduce problems at some new places. If somebody else decides on pushing it after seeing the patch, that is another matter.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8613712 [details] [diff] [review]
patch

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

So, this is a fairly large branch patch with almost the only substantial change is not to do the quicksearchfocus(), if i understand it correctly?

I don't particularly like moving the checks into a InitWritableDirectory function. Yes, there is a very small reduction of code duplication, but for someone reading the code that's a lot of moving from back and forth to understand what's doing on. Especially as it has an optional parameter to so something slightly different...

Comment 22

[:aceman]

(In reply to Magnus Melin from comment #21)
> So, this is a fairly large branch patch with almost the only substantial
> change is not to do the quicksearchfocus(), if i understand it correctly?
I am not sure you have seen the full patch. I have not attached it yet.

> I don't particularly like moving the checks into a InitWritableDirectory
> function. Yes, there is a very small reduction of code duplication, but for
> someone reading the code that's a lot of moving from back and forth to
> understand what's doing on. Especially as it has an optional parameter to so
> something slightly different...
I do not understand this problem. Yes, it reduces code duplicated 2 times (per app) to a single place. If the function does what it should, there should be no need to "move back and forth" and focus on the unique code in the calling function. Aren't that what libraries are for?
What does the optional parameter do differently? It sets what properties are needed for the returned AB. But the function still returns a usable AB.

Comment 23

[Magnus Melin [:mkmelin]]

What I'm saying is that the code is easier to read inlined, without having to understand what the function does and maybe doesn't. The value of libraries increase if you use them many times, but I don't think it's called for when the callers are only two which also are in different files. For future use there's also an overhead in finding, making sure everything needed is included etc. (It's the human reading the code that have to "move back and forth".)

Comment 24

[:aceman]

So you want me to add the "if AllABs URI is passed, ignore it and choose Personal AB" to all the 3 places, each in slightly different way. And the same in the future at each occasion like this.
Yeah, that is why SM now has to solve some fallout bugs from the AllABs change, as there is not enough sharing and abstracting stuff in libs.

Comment 25

[Ian Neal]

Comment on attachment 8613712 [details] [diff] [review]
patch

Shame there is not a single place for this function to be shared between SM and TB.

> /**
>+ * Returns a usable writable addressbook URI to be used for new cards or mailing
>+ * lists. Tries to use the given URI first but falls back to Personal AB.
>+ *
>+ * @param aPreferredABURI      The AB URI to try first.
>+ * @param aShouldSupportLists  Boolean indicating if the returned AB must allow
>+ *                             creating mailinglists.
>+ */
>+function InitWritableDirectory(aPreferredABURI, aShouldSupportLists = false) {
>+  // It is assumed Personal AB is always writable and also supports mailinglists.
>+
>+  if (aPreferredABURI == kAllDirectoryRoot + "?")
>+    return kPersonalAddressbookURI;
>+
>+  // Check if selected AB is a mailing list.
Nit: This comment belongs just before the if statement.

>+  let directory = GetDirectoryFromURI(aPreferredABURI);
>+  if (directory.isMailList) {
>+    let parentURI = GetParentDirectoryFromMailingListURI(aPreferredABURI);
>+    if (parentURI)
>+      return parentURI;
Might make it more readable if you just have:
return GetParentDirectoryFromMailingListURI(aPreferredABURI) || kPersonalAddressbookURI;
then you can move the next part out of the else.
>+  } else {
>+    // Check the AB isn't readonly (e.g. LDAP AB) and supports list if needed.
>+    if (!directory.readOnly && (!aShouldSupportLists || directory.supportsMailingLists)) {
>+      return aPreferredABURI;
>+    }
>+  }
>+
>+  return kPersonalAddressbookURI;
>+}

f+ for the moment

Comment 26

[Kent James (:rkent)]

This does not look like it will make it for 38.1.0 but it would still be good to try.

Comment 27

[:aceman]

(In reply to Ian Neal from comment #25)
> Comment on attachment 8613712 [details] [diff] [review]
> patch
> 
> Shame there is not a single place for this function to be shared between SM
> and TB.

I'm glad that you like it.

I could put it into /mailnews/addrbook/content/addrbookWidgets.xml into the addrbooks-menupopup binding. There it could be shared.

Mkmelin, would you then see this deduplication worthy of doing?

Comment 28

[:aceman]

Attachment: patch v2

I don't like the solution, but we can do it like mkmelin wants for TB38.

Comment 30

[Thomas D. (:thomas8)]

attachment 8636224 [details] shows an interesting and more challenging variant of this bug:
For users who have their directory pane hidden, as we are defaulting to "All ABs", their "new contact" and "new list" buttons will always be disabled and it might take a multi-step procedure requiring very good knowledge of the application to enable them again (in the case of attachment 8636224 [details], finding how to unhide menu bar, finding view > directory pane to unhide that, then selecting another ab).

Another point in favor of my position that those buttons should always be enabled, also when "All ABs" is selected.

Comment 33

[Kent James (:rkent)]

Can we get this reviewed so there is hope of getting in 38.2.0?

Comment 34

[Ian Neal]

Comment on attachment 8629661 [details] [diff] [review]
patch v2

With the patch from bug 1154481 landed, this doesn't seem to cause any issues on current trunk for SM.

>+++ b/mail/components/addrbook/content/abCardOverlay.js

>-    if ("selectedAB" in window.arguments[0]) {
>+    if (("selectedAB" in window.arguments[0]) &&
Nit: unneeded () round first check.
>+        (window.arguments[0].selectedAB != kAllDirectoryRoot + "?")) {
>       // check if selected ab is a mailing list
>       var abURI = window.arguments[0].selectedAB;
> 
>       var directory = GetDirectoryFromURI(abURI);
>       if (directory.isMailList) {
>         var parentURI = GetParentDirectoryFromMailingListURI(abURI);
>         if (parentURI)
>           gEditCard.selectedAB = parentURI;

>+++ b/mailnews/addrbook/content/abMailListDialog.js

>-  if (window.arguments && window.arguments[0])
>+  if (("arguments" in window) && window.arguments[0] &&
Nit: unneeded () round first check.
>+      ("selectedAB" in window.arguments[0]) &&
The caller always passes a selectedAB argument, so this check is probably not needed.
>+      (window.arguments[0].selectedAB != kAllDirectoryRoot + "?"))
As you are already checking selectedAB in the next if statement why not move the final check to there?
>   {
>     var abURI = window.arguments[0].selectedAB;
>     if (abURI) {
So make this:
if (abURI && abURI != kAllDirectoryRoot + "?")


>+++ b/suite/mailnews/addrbook/abCardOverlay.js

>   if ("arguments" in window && window.arguments[0])
>   {
>     gEditCard.selectedAB = kPersonalAddressbookURI;
> 
>-    if ("selectedAB" in window.arguments[0]) {
>+    if (("selectedAB" in window.arguments[0]) &&
Nit: unneeded () round first check.
>+        (window.arguments[0].selectedAB != kAllDirectoryRoot + "?")) {
r=me with those points addressed.

Comment 35

[[e-mail]]

Iann,

Thanks for the .JS scripts and the Patch V2 script. I am not a programmer or a person familiar with applying Java scripts or patches in java. What do I save the patch V2 script as (.js ?) and how do I apply or execute patch after saving with proper extension. I can read the patch and see what it looks like it is doing but I am unfamiliar with what file name to save the patch or how to execute or apply.

Thanks,
Terry

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 8629661 [details] [diff] [review]
patch v2

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

LGTM, r=mkmelin

Comment 37

[Thomas D. (:thomas8)]

So this looks ready for checkin, and we'd want to land it asap per rkent's comment 33.

Comment 38

[Wayne Mery (:wsmwk)]

still needs to address comment 34 ... then checkin

Comment 39

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #38)
> still needs to address comment 34 ... then checkin

Good catch, thanks.
I might be wrong but I think the aceman's needinfo?mkmelin of comment 27 is still outstanding.

(In reply to :aceman from comment #27)
> (In reply to Ian Neal from comment #25)
> > Comment on attachment 8613712 [details] [diff] [review]
> > patch
> > 
> > Shame there is not a single place for this function to be shared between SM
> > and TB.
> 
> I'm glad that you like it.
> 
> I could put it into /mailnews/addrbook/content/addrbookWidgets.xml into the
> addrbooks-menupopup binding. There it could be shared.
> 
> Mkmelin, would you then see this deduplication worthy of doing?

Comment 40

[[e-mail]]

Comment on attachment 8629661 [details] [diff] [review]
patch v2

I have never done any patching or editing of java script before but from what I see in file "https://bugzilla.mozilla.org/attachment.cgi?id=8629661&action=diff" it looks like the left pane might be the original file and the right pane is the modified script. Would I just go in with a text editor like "notepad" and edit the corresponding files? Am I correct in my assumption ?

Thanks,
Terry

Comment 41

[:aceman]

(In reply to [e-mail] from comment #40)
> Comment on attachment 8629661 [details] [diff] [review]
> patch v2
> 
> I have never done any patching or editing of java script before but from
> what I see in file
> "https://bugzilla.mozilla.org/attachment.cgi?id=8629661&action=diff" it
> looks like the left pane might be the original file and the right pane is
> the modified script. Would I just go in with a text editor like "notepad"
> and edit the corresponding files? Am I correct in my assumption ?

Yes, if you can find the corresponding file in the omni.ja archive, you could do the changes by hand. Then you need to remove files in startupCache folder so that the new code is picked up.

Comment 42

[:aceman]

Yes, mkmelin's answer to comment 27 would be useful for the followup bug but is not required to finish this TB38 version. I'll update the patch soon.

Comment 43

[:aceman]

Attachment: patch v2.1

Addressed the review nits.

Comment 44

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/66e1d27612eae1574cdf1b095e495b78816166e8
changeset:  66e1d27612eae1574cdf1b095e495b78816166e8
user:       aceman
date:       Wed Aug 05 14:26:00 2015 +0200
description:
Bug 1143812 - Always allow creating new contacts and lists after opening addressbook by using Personal Addressbook when a usable AB is not selected. r=mkmelin, r=IanN

Comment 45

[Kent James (:rkent)]

Comment on attachment 8643960 [details] [diff] [review]
patch v2.1

We should let it bake over the weekend, but let's consider this for 38.2.0 after a few days.

Comment 46

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #39)
> > Mkmelin, would you then see this deduplication worthy of doing?

I'd think it's probably not time well spent.

Comment 47

[:aceman]

Thomas, can you verify the fix is working right in current Nightly so that we can push it to 38?

Comment 48

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

Daily 42.0a1 2015-08-10: This only seems to fix the issue with "All Addressbooks" being selected. If the searchbox has focus, the buttons and menuitems are still disabled.

Comment 49

[:aceman]

Yes, that is intentional as fixing the searchbox needs further and more scary rearchitecting of the controllers. I left that for a followup bug for trunk only.

But the searchbox is now not focused automatically when you open the AB window.

Comment 50

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #47)
> Thomas, can you verify the fix is working right in current Nightly so that
> we can push it to 38?

Yes, does what it says correctly.
Like :aryx, I'm also worried about disabling the buttons while in search box.

If we agree that those buttons should never be disabled, then where and how DO they get disabled?
E.g., how come the Write button is always enabled?
If it takes a major rewrite of controllers just to get a button enabled, our current design must be pretty weird...

Would it help to remove the following lines which seem superfluous:
http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCommon.js#498
498   goUpdateCommand('cmd_newlist');
499   goUpdateCommand('cmd_newCard');

goUpdateCommand does nothing but en/disable commands; if we never disable, this shouldn't be needed.

Comment 51

[:aceman]

(In reply to Thomas D. from comment #50)
> Would it help to remove the following lines which seem superfluous:
> http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/
> abCommon.js#498
> 498   goUpdateCommand('cmd_newlist');
> 499   goUpdateCommand('cmd_newCard');
> 
> goUpdateCommand does nothing but en/disable commands; if we never disable,
> this shouldn't be needed.

Even if we remove the explicit updates, something implicit may still update the commands. So I would still consider it risky.

I think we have fixed the worst problems for users unexpectedly being at the All ABs node.
A better help would be to file the followup bugs.

Comment 52

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #51)
> A better help would be to file the followup bugs.

Sure... Done. :)

Comment 53

[Kent James (:rkent)]

We'll land this for 38.2.0

Comment 54

[Kent James (:rkent)]

Comment on attachment 8643960 [details] [diff] [review]
patch v2.1

https://hg.mozilla.org/releases/comm-esr38/rev/e5428a9d4d4e