408613 - Migrate SeaMonkey addressing preferences pane to new style and rewrite how the AB prefs UI works.

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: WIP v1

I'm filing this in Core because I'm going to end up affecting TB fairly significantly as well as SM.

The popup menus etc that we currently have in SM operate via RDF. This causes us headaches as it doesn't really like empty values. Additionally, it'd be nice to start moving some areas of address book away from rdf templates. The way the code currently operates is a mess at any rate and needs a rewrite.

The patch I'm attaching is a WIP patch. Currently this only works for SM, and even there possibly not fully, this is roughly what I've done so far:

- drops usage of nsILDAPPrefsService, which basically forms an array of the current directories.
- centralises filling of the menus and list boxes that are pref related and contain ABs into one function which is also generic.
- separates out dialog specific functions from the core ones.
- allows changes in the address book list to work correctly.
- Edit Directories dialog is now "instant": changes occur straight away. Note however, that for add/edit it brings up dialogs that must be accepted, and for delete it asks for confirmation.
- Migrate SM addressing dialog onto the new preferences window.

Like I say, its still WIP and there's some work to fix TB, but I wanted to post it for comment.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1 - Make pref-editDirectories 'standalone'

This patch makes the edit directories dialog work on its own (well, at least separate to pref-directory* and pref-addressing.xul).

Most of the functions that I've moved and altered from pref-directory.js were only used by pref-editdirectories.xul anyway.

Although we could probably use the result of bug 422845 for the list box, that bug seems to have stalled at the moment, and I'd like to get this bug moving. We can always do a further change later.

For now this separates out the pref-editdirectories.xul code, to hopefully make the work on refining pref-addressing.xul and its associated code easier.

The onHelpButton removal was because I realised its actually unnecessary as SM does it in helpMessengerOverlay and TB doesn't need it.

Comment 2

[[e-mail]]

Comment on attachment 329450 [details] [diff] [review]
Part 1 - Make pref-editDirectories 'standalone'

>+
>+
Nit: double

>+  var holdingArray = [];
I'd prefer you make an array of the directories, sort that, and then create listitems.

>+    if (la < lb)
>+      return -1;
>+    if (la > lb)
>+      return 1;
>+    return 0;
Hmm... I wonder whether we're still allowed to use localeCompare...

>+  if (abList && abList.selectedItems && abList.selectedItems.length) {
Nit: I think you can assume abList && abList.selectedItems &&

>+    var abURI = abList.selectedItems[0].getAttribute('value');
Nit: could use .selectedItem (could do above too)

>+  if (abList && abList.selectedItems && abList.selectedItems.length)
>+    AbDeleteDirectory(abList.selectedItems[0].getAttribute('value'));
Same here too.

>+  <label value="&directoriesText.label;"
>+	 accesskey="&directoriesText.accesskey;" control="directoriesList"/>
>+  <hbox flex="1">
>+    <listbox id="directoriesList" flex="1" onselect="selectDirectory();"
>+	     ondblclick="dblClickDirectory(event);"/>
>+    <vbox>
>+      <button id="addButton" label="&addDirectory.label;"
>+	      accesskey="&addDirectory.accesskey;"
>+	      oncommand="AbNewLDAPDirectory();"/>
>+      <button id="editButton" label="&editDirectory.label;"
>+	      accesskey="&editDirectory.accesskey;" disabled="true"
>+	      oncommand="editDirectory();"/>
>+      <button id="removeButton" label="&deleteDirectory.label;"
>+	      accesskey="&deleteDirectory.accesskey;" disabled="true"
>+	      oncommand="removeDirectory();"/>
Lots of tab characters creeping in here.

Comment 3

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1 - Make pref-editDirectories 'standalone' v2

Updated patch to address Neil's comments.

Comment 4

[[e-mail]]

Comment on attachment 330229 [details] [diff] [review]
[checked in] Part 1 - Make pref-editDirectories 'standalone' v2

>+    if (ab instanceof Components.interfaces.nsIAbDirectory && ab.isRemote &&
>+        !ab.isMailList) {
Surely top-level directories are never mailing lists?

>+                       .getDirectory(abList.selectedItems[0].getAttribute('value'))
Sorry, but I must have missed this one last time when I asked you to replace them with abList.selectedItem.getAttribute('value') but thinking about it again I've realised that there's a better option: abList.value should do the trick.

Comment 5

[David :Bienvenu]

Comment on attachment 330229 [details] [diff] [review]
[checked in] Part 1 - Make pref-editDirectories 'standalone' v2

some tiny comment nits:  I think it's it's :-)
+// cases we just rebuild the list as its easier than searching/adding in the

a comma at the end of the first line would make this easier to parse:

+    // If the disable delete button pref for the selected directory is set
+    // disable the delete button for that directory.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 2 - Migrate the pref pane.

I like this patch. It removes a lot. Hint: when reviewing pref-directory.js (now pref-addressing.js) its probably easier to just look at what is left.

What this patch is doing:

- Replacing the address book rdf menus with the new xbl one from addrbookWidgets.xml on both the pref pane and the am-addressing account manager page.
- Dropping most of the supporting code from pref-directory.js
- am-addressing page no longer relies on pref-directory.js so pref-directory.js is now removed from Thunderbird
- renames pref-directory.js to pref-addressing.js to match pref-addressing.xul since that is now all it is used for.
- moves the initialisation code for ensuring the xbl menupopups are correctly linked to their parents to the constructor of the xbl menupopup. This solves a bug whereby sometimes on first showing, the lists in the pref pane would appear with nothing selected, but you could close the dialog and reopen and it would work fine.

There's some more related tidy up I can do after this, but I'm going to do that in follow-up bugs.

Comment 7

[[e-mail]]

Comment on attachment 333758 [details] [diff] [review]
Part 2 - Migrate the pref pane.

In addrbookWidgets.xml I would select the correct menuitem in build() as you're now duplicating code in both of its callers.

git-apply warns about some trailing whitespace:

>+        <checkbox id="emailCollectionOutgoing" 

>+  var directoryServer = 

>+            <treecell label="(Migrated: &address.label;)"/>

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 333758 [details] [diff] [review]
Part 2 - Migrate the pref pane.

This doesn't work. I forgot, the xbl is setting the value as the uri:

moz-abldapdirectory://<prefid>

not just:

<prefid>

:-(

I'll probably have to provide a method of setting a different value for now, until we can get rid of rdf.

Comment 9

[[e-mail]]

(In reply to comment #8)
> (From update of attachment 333758 [details] [diff] [review])
> This doesn't work. I forgot, the xbl is setting the value as the uri:
> 
> moz-abldapdirectory://<prefid>
> 
> not just:
> 
> <prefid>
> 
> :-(
> 
> I'll probably have to provide a method of setting a different value for now,
> until we can get rid of rdf.

Surely that's the non-rdf popup in this case?

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> (In reply to comment #8)
> > I'll probably have to provide a method of setting a different value for now,
> > until we can get rid of rdf.
> 
> Surely that's the non-rdf popup in this case?

Yes its the non-rdf popup, but I also expect that the values (i.e. just pref locations) won't be what we end up with when we switch away from rdf completely.

Comment 11

[Ian Neal]

Comment on attachment 333758 [details] [diff] [review]
Part 2 - Migrate the pref pane.

> function enableAutocomplete()
> {
>+  var autocompleteLDAP =
>+    document.getElementById("ldap_2.autoComplete.useDirectory");
>+  var directoriesList = document.getElementById("directoriesList");
>   var editButton = document.getElementById("editButton");
>+  if (autocompleteLDAP.value) {
>+    // If the default directory preference is locked
>     // disable the list popup
>+    if (document.getElementById("ldap_2.autoComplete.directoryServer").locked)
>       directoriesList.setAttribute("disabled", true);
>+    else
>       directoriesList.removeAttribute("disabled");
>     editButton.removeAttribute("disabled");
>   }
>   else {
>     directoriesList.setAttribute("disabled", true);
>     editButton.setAttribute("disabled", true);
>   }
> }
I know you have cancelled the review request but changing the disabled status of the editButton like this will be ignoring the lock status of its preference. You could use the EnableElementById function from preferences.js so something like:
var acLDAPValue = document.getElementById("ldap_2.autoComplete.useDirectory")
                          .value;
EnableElementById("directoriesList", acLDAPValue, false);
EnableElementById("editButton", acLDAPValue, false);

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 2 - Migrate the pref pane v2

Updated part 2 patch. This needs the updated addrbookWidget.xml from bug 450581 (awaiting final review) and addresses the previous comments on this bug.

Comment 13

[[e-mail]]

Comment on attachment 334683 [details] [diff] [review]
Part 2 - Migrate the pref pane v2

>rename from mailnews/addrbook/prefs/resources/content/pref-directory.js
>rename to mailnews/addrbook/prefs/resources/content/pref-addressing.js
>+ *   Srilatha Moturi <[e-mail]>, original implementor
I think it's stretching a point to suggest that pref-addressing.js is based on pref-directory.js - it's all new code.

>+    <stringbundle id="bundle_addressBook"
>+		  src="chrome://messenger/locale/addressbook/addressBook.properties"/>
The first of at least two tabs that crept into this patch.

>+      <preference id="ldap_2.autoComplete.useDirectory"
>+                  name="ldap_2.autoComplete.useDirectory"
>+                  onchange="enableAutocomplete();" type="bool"/>
We normally put type="bool" before onchange.

>+function setupDirectoriesList()
>+{
>+  var override = document.getElementById("identity.overrideGlobalPref").getAttribute("value");
>+  var autocomplete = document.getElementById("ldapAutocomplete");
>+  // useGlobalFlag is set when user changes the selectedItem on the radio button and switches
>+  // to a different pane and switches back in Mail/news AccountSettings
>+  var useGlobalFlag = document.getElementById("overrideGlobalPref").getAttribute("value");
>+  // directoryServerFlag is set when user changes the server to None and switches
>+  // to a different pane and switches back in Mail/news AccountSettings
>+  var directoryServerFlag = document.getElementById("directoryServer").getAttribute("value");
>+
>+  if(override == "true" && !useGlobalFlag)
>+    autocomplete.selectedItem = document.getElementById("directories");
>+  else
>+    autocomplete.selectedItem = document.getElementById("useGlobalPref");
>+
>+  var directoriesList = document.getElementById("directoriesList");
>+  var directoryServer =
>+        document.getElementById("identity.directoryServer").getAttribute('value');
>+  if (directoryServerFlag) {
>+    document.getElementById("identity.directoryServer").setAttribute("value", "");
>+    directoryServer = "";
>+  }
>+  directoriesList.value = directoryServer;
> }
I know you're only moving the code but this looks somewhat contrived - I wonder how much of it is actually necessary.

One other problem - the old ldap server list used to include a "None" entry.

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> One other problem - the old ldap server list used to include a "None" entry.

None was originally put in to work around the rdf bug that if got rid of an address book (or sometimes even just hadn't set one up I think), the value would go to null, but you'd end up with the rdf menu showing text as if it had selected an address book - totally confusing the user.

I really didn't like None in the first place, especially as we had the options to turn off/on the completion explicitly.

So I would really like to drop it just seems unnecessary.

Comment 15

[[e-mail]]

Comment on attachment 334683 [details] [diff] [review]
Part 2 - Migrate the pref pane v2

OK, we'll look into "None" separately.

Comment 16

[Ian Neal]

Comment on attachment 334683 [details] [diff] [review]
Part 2 - Migrate the pref pane v2

>diff --git a/mailnews/addrbook/prefs/resources/content/pref-addressing.xul b/mailnews/addrbook/prefs/resources/content/pref-addressing.xul
>@@ -1,111 +1,88 @@
> <?xml version="1.0"?>
No boilerplate?
>+    <stringbundle id="bundle_addressBook"
>+		  src="chrome://messenger/locale/addressbook/addressBook.properties"/>
Nit: tabs in line above.
> 
>+    <preferences id="addressing_preferences">
>+      <preference id="mail.collect_email_address_outgoing"
>+		  name="mail.collect_email_address_outgoing"
>+		  type="bool"/>
Nit: tabs in the two lines above.
>+    <groupbox>
>+      <caption label="&emailCollectiontitle.label;"/>
>+      <description>&emailCollectiontext.label;</description>
>+      <hbox align="center">
>+        <checkbox id="emailCollectionOutgoing"
>+                  label="&emailCollectionPicker.label;"
>+                  accesskey="&emailCollectionPicker.accesskey;"
>+                  preference="mail.collect_email_address_outgoing"/>
>+        <menulist id="localDirectoriesList" flex="1"
>+                  aria-labelledby="emailCollectionOutgoing"
>+		  preference="mail.collect_addressbook">
Nit: tabs in line above.

>+        <menulist id="directoriesList" flex="1"
>+                  aria-labelledby="autocompleteLDAP"
>+                  preference="ldap_2.autoComplete.directoryServer">
>+          <menupopup id="directoriesListPopup" class="addrbooksPopup"
>+                     remoteonly="true" value="dirPrefId"/>
>+	</menulist>
Nit: tab in line above.

Comment 17

[Robert Kaiser]

Is there anything else to do here than fixing the nits and check in?

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> Is there anything else to do here than fixing the nits and check in?

Bug 450581 needs to be completed and checked in.

Comment 19

[Mark Banner (:standard8)]

Attachment: Adds back "None" to the account settings/composition ldap options

Supplemental patch to add back the "None" option to the account setting/composition LDAP selection.

Comment 20

[Mark Banner (:standard8)]

Attachment: [checked in] Part 2 - Migrate the pref pane v3

Updated patch that I'm checking in.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 337631 [details] [diff] [review]
[checked in] Part 2 - Migrate the pref pane v3

Checked in, changeset id: 296:9c411f661785

Comment 22

[[e-mail]]

Comment on attachment 337630 [details] [diff] [review]
Adds back "None" to the account settings/composition ldap options

Hmm, do we need this in addressing preferences too?

Comment 23

[Mark Banner (:standard8)]

Attachment: [checked in] Adds back "None" ldap AB menulists

This is what I checked in for the "None" option, includes it in both places where it was before (as the default is "").

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> Created an attachment (id=337633) [details]
> [checked in] Adds back "None" ldap AB menulists
> 
> This is what I checked in for the "None" option, includes it in both places
> where it was before (as the default is "").

Changeset id is 297:dd785a9c1107

This bug is now fixed.

Comment 25

[Mark Banner (:standard8)]

I checked in an additional changeset 299:16fab37cd726, that changes the attribute name from writeable to writable in pref-addressing.xul - Neil corrected it in bug 450581 without telling me.