397194 - Make the local address book properties (new/edit) dialogs stand-alone

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Attachment: SM Patch

Currently the local new/edit address book dialogs (which are based on one js and one xul source) requires additional code from the caller to pass the title, directory etc. They also then require a callback to do the appropriate thing.

This gets in the way of allowing different types of address book to use different dialogs - especially for the properties (rename) option.

So this bug is going to make the abAddressBookNameDialog "stand-alone", so that it just has to be opened from a window.openDialog call, with an optional parameter, and the dialog will do the rest.

The option parameter is "selectedDirectory", and is expected to be a nsIAbDirectory. It is specified if you are intending to rename the address book. This is the same way as the LDAP properties dialog currently works.

A follow-up bug will resolve how we will actually handle allowing different dialogs for different ab types.

I'm attaching the SM patch first (the UI is currently separate), once I've got reviews for that I'll do the TB one as well.

Comment 1

[Karsten Düsterloh]

Comment on attachment 281960 [details] [diff] [review]
SM Patch

>Index: mailnews/addrbook/resources/content/abAddressBookNameDialog.js
>===================================================================

Please add the missing license header.

>+  // look in arguments[0] for parameters to see if we have a directory or not
>+  if ("arguments" in window && window.arguments[0] &&
>+      "selectedDirectory" in window.arguments[0]) {
>+    gDirectory = window.arguments[0].selectedDirectory;
>+
>+    gNameInput.value = gDirectory.dirName;

Superfluous empty line.

>+  // Work out the window title (if we have a directory specified, then its a
>+  // rename)

It's "it's". ;-)
And sentences beginning with capital letters should end with a full stop.

>+  var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+    .getService(Components.interfaces.nsIStringBundleService);
>+
>+  var bundle = strBundleService
>+    .createBundle("chrome://messenger/locale/addressbook/addressBook.properties");

Why don't you use a <stringbundle> in the XUL?

>+  // If we haven't a directory, or we do, but its not one of the special ones,
>+  // allow renaming.

"it's"

>+  if (!gDirectory ||
>+      (gDirectory.URI != kCollectedAddressbookURI &&
>+       gDirectory.URI != kPersonalAddressbookURI)) {
>+    gNameInput.focus();
> 
>-	moveToAlertPosition();
>+    abNameDoOkEnabling()

Superfluous empty line.

>+  }
>+  else {

On one line, if you need to use this awful "{ after code" style at all. :-P

>+    // Address book name is not editable, therefore disable the field and
>+    // only have an ok button that doesn't do anything.
>+    gNameInput.disabled = true;

Hm, this looks kind of weird (although it's the same styling we use in the folderpane properties).
I think we should have a mailnews style for readonly textboxes like we have eg. in the webpage info dialog, where you can copy and such, but not edit, and it looks just like normal dialog text. Then we should just set readonly=true here.

>+    document.documentElement.buttons = "accept";
>+    document.documentElement.removeAttribute("ondialogaccept");

This for sure is ugly. The buttons assignment is not a problem, but removing the handler is not nice behaviour. The handler should check if he needs to update!

>+  moveToAlertPosition();

Is this really needed? It isn't under Linux, but I didn't check Mac...

>+  if (!gDirectory)
>+    Components.classes["@mozilla.org/addressbook;1"]
>+      .createInstance(Components.interfaces.nsIAddressBook)
>+      .newAddressBook(newName, "", kPABDirectory);

Align the . vertically.

>Index: mailnews/addrbook/resources/content/abCommon.js
>===================================================================
>+        window.openDialog(
>+          "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul",
>+          "", "chrome,modal=yes,resizable=no,centerscreen",
>+          {selectedDirectory: directory});

If possible, factor out this call and reuse it in addressbook.js (or vice versa).

>Index: mailnews/addrbook/resources/content/addressbook.js
>===================================================================
>+  window.openDialog(
>     "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul", 
>-     "", "chrome,modal=yes,resizable=no,centerscreen", {title: dialogTitle, canRename: canRename, name: selectedABDirectory.dirName,
>-      okCallback:AbOnRenameAddressBook});

See above.

Comment 2

[Karsten Düsterloh]

> >+  moveToAlertPosition();
> 
> Is this really needed? It isn't under Linux, but I didn't check Mac...

Mac doesn't seem to need it either.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> (From update of attachment 281960 [details] [diff] [review])
> >+  var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]
> >+    .getService(Components.interfaces.nsIStringBundleService);
> >+
> >+  var bundle = strBundleService
> >+    .createBundle("chrome://messenger/locale/addressbook/addressBook.properties");
> 
> Why don't you use a <stringbundle> in the XUL?

Opps too much cut and paste ;-)

> >+    document.documentElement.buttons = "accept";
> >+    document.documentElement.removeAttribute("ondialogaccept");
> 
> This for sure is ugly. The buttons assignment is not a problem, but removing
> the handler is not nice behaviour. The handler should check if he needs to
> update!

I actually prefer it that way as its less code to do the same job. I did this in bug 307056 and I seem to remember Neil suggested that to me then.

> >+  moveToAlertPosition();
> 
> Is this really needed? It isn't under Linux, but I didn't check Mac...

Probably not, especially as its defined in toolkit/obsolete/content/dialogOverlay.js

> >Index: mailnews/addrbook/resources/content/abCommon.js
> >===================================================================
> >+        window.openDialog(
> >+          "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul",
> >+          "", "chrome,modal=yes,resizable=no,centerscreen",
> >+          {selectedDirectory: directory});
> 
> If possible, factor out this call and reuse it in addressbook.js (or vice
> versa).

Like I said in comment 0, in a follow up bug I want to make the two edit ones (ldap & local) into the same call, and I'd like to do that for new dialogs as well. So I'd prefer to do the factoring out in a separate bug and just keep this bug for simplifying the way the local address book properties dialog works.

Comment 4

[[e-mail]]

(In reply to comment #3)
>>>+  moveToAlertPosition();
>>Is this really needed? It isn't under Linux, but I didn't check Mac...
>Probably not, especially as it's defined in toolkit/obsolete/content/dialogOverlay.js
toolkit/content/widgets/dialog.xml actually.

Comment 5

[[e-mail]]

(In reply to comment #1)
>(From update of attachment 281960 [details] [diff] [review])
>>+  moveToAlertPosition();
>Is this really needed? It isn't under Linux, but I didn't check Mac...
For some reason the caller is centring this... if this is a modal dialog, moveToAlertPosition works better for non-full-screen parent windows...

Comment 6

[Mark Banner (:standard8)]

Attachment: SM Patch v2

Fixes the comments. Note also my comment 3. I've based the license block on cvs history and what abAddressBookNameDialog.xul shows as they appear to have been committed at the same time.

Comment 7

[Mark Banner (:standard8)]

Attachment: SM Patch v3 (checked in)

Revised patch to fix bitrot

Comment 8

[[e-mail]]

Comment on attachment 287566 [details] [diff] [review]
SM Patch v3 (checked in)

>+  // If we haven't a directory, or we do, but it's not one of the special ones,
>+  // allow renaming.
>+  if (!gDirectory ||
>+      (gDirectory.URI != kCollectedAddressbookURI &&
>+       gDirectory.URI != kPersonalAddressbookURI)) {
>+    gNameInput.focus();
>+    abNameDoOkEnabling();
>+  } else {
>+    // Address book name is not editable, therefore disable the field and
>+    // only have an ok button that doesn't do anything.
>+    gNameInput.readOnly = true;
>+    document.documentElement.buttons = "accept";
>+    document.documentElement.removeAttribute("ondialogaccept");
>+  }
I think this would make more sense with the condition switched around (it would also allow you to remove the ugly comment before the if).

>+  // Either create a new directory or update an existing one depending on what
>+  // we were given when we started.
>+  if (!gDirectory)
>+    Components.classes["@mozilla.org/addressbook;1"]
>+              .createInstance(Components.interfaces.nsIAddressBook)
>+              .newAddressBook(newName, "", kPABDirectory);
>+  else
>+    gDirectory.dirName = newName;
This would definitely look better reversed i.e.
if (gDirectory)
  gDirectory.dirName = newName;
else
  ...

>+        window.openDialog(
>+          "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul",
>+          "", "chrome,modal=yes,resizable=no,centerscreen",
>+          {selectedDirectory: directory});
Worth dropping centerscreen?

>+    "", "chrome,modal=yes,resizable=no,centerscreen",
>+    null);
Does the null deserve its own line?

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> >+        window.openDialog(
> >+          "chrome://messenger/content/addressbook/abAddressBookNameDialog.xul",
> >+          "", "chrome,modal=yes,resizable=no,centerscreen",
> >+          {selectedDirectory: directory});
> Worth dropping centerscreen?

Let's drop it when I reduce all the new/rename local/ldap calls.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 287566 [details] [diff] [review]
SM Patch v3 (checked in)

Checked in with comments addressed (apart from the exception per my previous comment).

Comment 11

[Mark Banner (:standard8)]

Attachment: SM followup (checked in)

Remove redundant title string as its actually overridden from .properties here:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/resources/content/abAddressBookNameDialog.js&rev=1.10&mark=67-68#47

Comment 12

[Mark Banner (:standard8)]

Attachment: TB patch v1 (checked in)

TB version of patch for this bug.

This makes TB reuse the xul & js for the new ab/ab properties dialogues from /mailnews. If you want me to attach the diffs I can, but I couldn't see anything major.

This also means that TB can use the new ab/ab properties dialogues as "stand alone" which mean we should soon be able to simplify some of the calls which will make it easier for new address book types.