1312262 - Make the dialog titles in AB consistent and dynamic

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Richard Marti (:Paenglab)]

We are not consistent with the dialog titles. The AB dialog is "Address Book Properties" or "Directory Server Properties" for LDAP, the Mailing list Dialog is only "Mailing List" and the contact dialog "Edit Contact for ...". We should use for all dialogs "Properties" and "New ..." when adding a new AB, Mailing list or contact.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch

This patch adds the "Properties" to the title of mailing lists and contacts and adds "New Mailing List" when we add a new mailing list.

addressBook.properties has also some white space changes because of trailing WS.

Comment 2

[Thomas D. (:thomas8)]

Thank you for looking into this, I'm always in favor of ux-consistency.

I like the "New..." part of this patch. It's ux-consistent and user-friendly because of "what you click is what you get" (changes by :paenglab's patch marked with (*)):

Menu                           Button       Context Menu  Resulting dialogue title
-----------------------------------------------------------------------------------------
New > Address Book Contact...  New Contact  New Contact   New Contact  
New > Mailing List...          New List     New List      New Mailing List (*)
New > Address Book...          ---          ---           New Address Book
New > LDAP Directory...        ---          ---           Directory Server Properties (1)

(1) So in the same line with the fine change of this patch wrt "New...", I think we should have:
New > LDAP Directory...        ---          ---           New LDAP Directory

Of course we are not creating a new LDAP Server, but I think this label is more intuitive and very correct from TB's perspective because we're adding a "NEW LDAP Directory" to the list of address books (and due to the nature of LDAP, the thing itself is already on the server and we're just connecting to it). Otherwise, if "New LDAP Directory" is not acceptable for the dialogue, that would mean it's also wrong in the 'File > New' Menu.

Note that the above are so intuitive and charming because a) we are using natural language terms and b) we are guiding the user and reassuring him by way of label consistency between command label and dialogue caption. Hence e.g. when you click on "New > Mailing List" or "New List", you'll get a dialogue having exactly the same words in the caption: "New Mailing List". Which is an action-oriented caption, describing to the user what he is currently doing in this dialogue, namely creating a "New Mailing List" (and we can safely skip the action verb "create" because "New" has an implied action of "Create New..."). Accordingly for "Edit Mailing List". When done, user confirms his *action* (New.../Edit...) with OK, or dismisses with Cancel.

This is good UX practice:
http://uxmovement.com/forms/best-practices-for-modal-windows/
"Include a clear, visible title that matches the clicked button"

For the same reason, I think we need to find a better way for the other part of the patch, where you suggested using "Properties" for all the dialogue captions.

I think we agreed on the motivation of Bug 1310442, which was to *reduce* our usage of "Properties", in favor of less technical, more natural, more intuitive, more action-oriented *verbs* like "Edit...". Unfortunately, the current patch here kind of reverts that idea the other way round, returning to the notion of "Properties".

Good UX practice suggests:
"2. Match Between Dialog and The Real World
Dialog should speak the users’ language (use words, phrases and concepts familiar to the user), rather than special system terms."
https://uxplanet.org/5-essential-ux-rules-for-dialog-design-4de258c22116#.rgux4ssfb

Here's
*MY PROPOSAL P1:*
******************************************************************************************************
(*) indicating changes proposed by me
[#] indicating icon of edited element (Contact, Mailing List, AB, [LDAP Dir])

Menu                             Button  Context Menu      Resulting dialogue title
Edit > Contact Properties (1)    Edit    Edit Contact      [#] Edit Contact for John Doe (6)  
Edit > Mailing List Properties   Edit    Edit List         [#] Edit Friends and Family (*)(2)
Edit > Address Book Properties   Edit(3) Properties (*)(4) [#] Personal Address Book Properties (*)(5)
******************************************************************************************************

(1) Main Menu labels: I preserve consistency by having "Edit > Contact", but I also appended "Properties" so that it becomes "Edit > Contact Properties". Reasons:
a) Consistent Access Key for all "Edit/Properties" dialogues: i (Note: Cmd+I is keyboard shortcut for Properties on MAC). For l10n, this can only be guaranteed by having a stable term for the various flavors of this command.
b) UI hint that even though it's applied to different elements, this is still essentially the same command: cmd_Properties (to EDIT ELEMENT PROPERTIES).
c) I suspect Main Menu is mostly used by keyboard users, or systematic/deductive types of users who prefer fixed menu systems over contextual action. Both of these user groups benefit from a) and b).

(2) Edit Mailing List dialogue, proposed changes:
a) drop the word "Mailing List" from the caption, and instead:
b) add Mailing list icon to dialogue caption (easy visual pointer that this is about a mailing list)
c) Include and focus on the *name* of the mailing list (edited element), which was created by user and hence he'll know exactly what this is about:

Name of Mailing List   Dialogue Caption
------------------------------------------------
Friends and Family     Edit Friends and Family
Customers              Edit Customers
Customers List         Edit Customers List
Kundenliste            Kundenliste bearbeiten
Kunden                 Kunden bearbeiten
Freunde und Familie    Freunde und Familie bearbeiten
Freundeskreis          Freundeskreis bearbeiten

Note that the term "Mailing List" does not add much content, and the entire dialogue design and labels leave no doubt that you are editing a mailing list (and not, say, a contact or AB):
- List icon in dialogue caption (proposed here)
- List Name as created by user (proposed here)
- "List Name" and "List Nickname" properties captions at top of dialogue
- List of email addresses as main input field of dialogue
Furthermore, as seen above, the term "list" might already be included in the customized name of the Mailing List, and it would be odd to have something like "Edit Mailing List Customers List"/"Mailingliste Kundenliste bearbeiten".
I think we can get away without quotes around the name, because in the long run, that's just clutter, and "Edit" should be a very short and easily identifiable term in l10n, which is clearly inherently differentiated from the list name.

(3) Changing button labels should be avoided, so I keep "Edit" on the button even when an Address Book is selected, which has "Properties" on the context menu (see (4) below). Changing AB properties (the name of an AB) is very rare, and much more likely to be accessed from context menu, so this minimal inconsistency looks acceptable to me, while maintaining full functionality.

(4) I propose maintaining the notion of "Properties" for ABs and, for the time being, also for LDAP directories. Reasons:
a) "Edit Address Book" (as proposed by :paenglab) can mislead the user to believe that you can actually edit the *contents* of the AB. However, we only allow editing a single property, AB name. Hence "Properties" is really the appropriate term for normal ABs.
b) Similarly, "Edit LDAP Directory" wrongly suggests that we allow editing the *contents* of an LDAP AB on the server. That's one of our most popular feature requests with almost 400 votes (bug 86405), but unfortunately it hasn't been implemented yet, and with current manpower, chances are slim. After implementation, there'll still be a big difference between "Properties of LDAP AB" and "Edit LDAP AB". "Properties" has all the settings, whereas "Edit" allows editing the content. So we don't want to create confusion here and make our users even more angry by suggesting that they could now "Edit LDAP AB" whilst really they still cannot...
c) The need for horizontal consistency between different dialogues for different elements is considerably LESS important than the above-mentioned vertical consistency between the command label and the resulting dialogue: Clicking on *Properties* of "Personal Address Book" will consistently trigger "Personal Address Book *Properties*".

(5) As in (2) above, I propose adding the *name* of AB to the AB properties dialogue. So we'll consistently include the name of the edited element for all element types (Contact, Mailing List, AB).

(6) Please change the icon of the "Edit Contact" dialogue to the current icon for contacts. The current icon (contact icon in open book) is both confusing and hard to recognize, and not consistent with our contact icon on toolbar.
I have also considered shortening the caption to "Edit John Doe" (omitting the element type, as I propose for Mailing Lists), but it sounds slightly semantically odd to me because you can't really edit a person, but only the contact of that person in your AB.

Comment 3

[Richard Marti (:Paenglab)]

Thomas, unfortunately it's not so easy to change the icons in the dialog titles. On Windows this needs ICO icons in progdir/chrome/default with the ID name. And this icons are the same for all Windows versions and should be good visible on all (or almost) all window background colors. But if we find a solution, the good news is, this needs no string changes.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v2

This patch adds the LDAP dialog with "New LDAP Directory" and "LDAP Directory Properties". The mailing list dialog titles are not dynamic yet. This patch should be in the state to check-in when the dynamic titles aren't ready in time.

Ian, I had to touch the LDAP dialogs in mailnews which also affects SM. Please can you check if the string changes are okay for you? I could also revert both strings to "Directory Server Properties". Then there would be no visual change for SM. I ask you now to be sure the string changes are ready before the 52 uplift.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v3

It was easier than I thought to implement the dynamic mailing list title.

Unfortunately the new change is in mailnews too. Ian, now there is also a new string in addressBook.properties.

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 8804027 [details] [diff] [review]
ContactProperties.patch v3

Richard, this looks good but let's go all the way while we are here, as proposed in my Comment 2, P1:

Please also make the dialogue titles of Address Book and LDAP Directory Properties be dynamic and include the name.

I've played with this a little, and for all the reasons laid out in comment 2, I think less is more (ux-minimalism), because default elements have telling names (like "Personal Address Book"), and for any user-created elements, user will certainly know what type they are (especially for LDAP which user will never create unless he knows the stuff). Meaning that the type information does not add much value, but the name of the element does, for you can have as many ABs and LDAP Dirs as you please, so it's more about distinguishing them and less about knowing their type (which you already know when you click on it, from the icon of the thing). And yes, please let's try to clone those icons into the dialogue captions, it must be possible (asap but we can use another bug if it's more tricky).

This is also consistent with File Properties e.g. in Windows Explorer which have the same dynamic type of dialogue caption:
Readme.txt Properties
ContactProperties.patch Properties

We already have dynamic label for contacts:

> Edit Contact for John Doe

You implemented dynamic label for lists:

> Edit "Mailing List Name" (without quotes)

Please add dynamic label for ABs and LDAP Dirs:

"%Address Book Name" Properties (without quotes), so that we'll have e.g.
> Personal Address Book Properties
> Collected Addresses Properties
> All Address Books Properties
> Students Address Book Properties
> Friends and Family Properties
> Business Partners Properties

"%LDAP Dir Name" Properties (without quotes), so that we'll have e.g.
> Employees Login Master Properties
> Students Directory Properties
> Eigenschaften von Siemens Mitarbeiterverzeichnis

Now there's another thing which we need to consider.
Please look at "New Contact" or "Edit Contact for John Doe" dialogue and watch the title bar while changing his display name. A preview of the new display name is mirrored back to the dialogue title as you type. Which is especially nice for new elements, to make the name of the new thing explicitly visible while entering the new information.

It looks fine to me for Contacts as it is, to preview the effects of the display name change.
I'd love to see the same feature for all dialogues of the type "New...", so that when you enter the name of the new item, we dynamically update the dialogue title as you type, like this:

[ marks the begin of the dynamic name part appearing

New Contact[ for John Doe
New Mailing List[: Friends and Family
New LDAP Directory[: Mitarbeiter-Verzeichnis
New Address Book[: Business Partners AB

I'm not sure if we want the same dynamic reflection of name changes in title bar also for the other "Edit/Properties" type of dialogues.

pro:
- immediate preview assists to get the new name right (ux-error-prevention), and to have it in view while changing other properties before summarily confirming the changes made

con:
- you lose the old name as soon as starting to type the new name, but often it's still useful for comparison (ux-efficiency). E.g. in bugzilla, it's so annyoing that the old summary immediately disappears when editing it for changes.
- having the new name on the item before confirming with OK wrongly suggests that the name has already been changed, which might mislead into clicking Cancel (ux-error-prevention) and losing the name-change. However that also applies to most other properties where you can't see if they are old or new until closing and reopening the dialogue.

IF we want dynamic name change updates in dialogue title bars, I believe we'd have to change the labels proposed above so that they don't look odd when the old name is cleared to enter a new name:

[ marks the begin of the dynamic name part appearing

We already have name-updating label for contacts:

> Edit Contact[ for John Doe

Name-updating Label for Mailing Lists

> Edit Mailing List: [Mailing List Name

Name-updating label for ABs and LDAP Dirs:

Address Book Properties: [Address Book Name     so that we'll have e.g.
> Address Book Properties: Personal Address Book
> Address Book Properties: Collected Addresses
> Address Book Properties: All Address Books
> Address Book Properties: Students Address Book
> Address Book Properties: Friends and Family
> Address Book Properties: Business Partners

LDAP Directory Properties: [LDAP Dir Name          so that we'll have e.g.
> LDAP Directory Properties: Employees Login Master
> LDAP Directory Properties: Students Directory
> Eigenschaften von LDAP Verzeichnis: Siemens Mitarbeiterverzeichnis

pro:
- that's more generic (even if we don't want dynamic name change reflection), more systematic, and more explicit wrt object type
- with name-updating, avoid gaps and ambiguity about object type when name has just been cleared, as we'd keep the generic label and supplement from there

con:
- dialogue titles are longer hence harder to parse
- clumsy duplicate descriptors might occur, if the item name includes the item type:
> Address Book Properties: Personal Address Book   (duplicating "Address Book")
> Edit Mailing List: [Customers List

I'm not sure which variant I prefer between with or without name updating for Mailing Lists, ABs, and LDAP Dirs.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #6)
> I'm not sure which variant I prefer between with or without name updating
> for Mailing Lists, ABs, and LDAP Dirs.

I'm also not sure which variant I prefer between with or without leading object type descriptor in the dialogue caption.

Comment 8

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (needinfo?me) from comment #6)
> Comment on attachment 8804027 [details] [diff] [review]
> ContactProperties.patch v3
> 
> Richard, this looks good but let's go all the way while we are here, as
> proposed in my Comment 2, P1:

I see no such proposal for AB and LDAP. Or there is too much written in this comment that I can't find it. ;)

> Please also make the dialogue titles of Address Book and LDAP Directory
> Properties be dynamic and include the name.

LDAP ok, but AB is overkill as there is only one field with the name in this dialog.

> Now there's another thing which we need to consider.
> Please look at "New Contact" or "Edit Contact for John Doe" dialogue and
> watch the title bar while changing his display name. A preview of the new
> display name is mirrored back to the dialogue title as you type. Which is
> especially nice for new elements, to make the name of the new thing
> explicitly visible while entering the new information.
> Name-updating label for ABs and LDAP Dirs:

I knew about this and decided to do this in a new bug. We can't fix the whole AB in this bug by expanding the features of this bug. Also have in mind we have only around 10 days left until the freeze.

> LDAP Directory Properties: [LDAP Dir Name          so that we'll have e.g.
> > LDAP Directory Properties: Employees Login Master
> > LDAP Directory Properties: Students Directory
> > Eigenschaften von LDAP Verzeichnis: Siemens Mitarbeiterverzeichnis

> con:
> - dialogue titles are longer hence harder to parse

Way too long for the limited dialog width.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v4

Added the dynamic LDAP title. I used the replicationProgress.properties for the title as this bundle is already in the LDAP dialog mounted.

Comment 10

[Richard Marti (:Paenglab)]

As stated before I don't want to add dynamic title for AB dialogs because in this dialog you can only change the name and repeating in the title the only field in this dialog makes not much sense.

Comment 11

[Ian Neal]

Comment on attachment 8804769 [details] [diff] [review]
ContactProperties.patch v4

>+# %S will be the LDAP directory's display name
>+directoryTitleEdit=Edit %S
My preference would be to have directoryTitleNew in here too rather than in the dtd file.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v5

(In reply to Ian Neal from comment #11)
> Comment on attachment 8804769 [details] [diff] [review]
> ContactProperties.patch v4
> 
> >+# %S will be the LDAP directory's display name
> >+directoryTitleEdit=Edit %S
> My preference would be to have directoryTitleNew in here too rather than in
> the dtd file.

Done.

Comment 13

[:aceman]

Comment on attachment 8805841 [details] [diff] [review]
ContactProperties.patch v5

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

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +5,5 @@
>  #
>  # The following are used by the Mailing list dialog
>  #
> +
> +# %S will be the Mailing List's display name

Please use the proper format for a "localization note" comment.

@@ +11,3 @@
>  emptyListName=You must enter a list name.
>  lastFirstFormat=%S, %S
>  firstLastFormat=%S %S  

Heh, IF really these trailing spaces are required for proper operation, maybe it would use a localization note. Oneone's eager editor may automatically strip trailing space.
But this is not your fault :)

::: mail/locales/en-US/chrome/messenger/addressbook/replicationProgress.properties
@@ +14,5 @@
>  downloadButton.accesskey=D
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory

Maybe we could name this "New connection to LDAP Directory" to be specific we do not create a directory on the server.

@@ +15,5 @@
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory
> +# %S will be the LDAP directory's display name

Localization note format.

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +51,4 @@
>             + ex + "\n");
>      }
>  
> +    gOldListName = gCurrentDirectory.dirName;

Does this need to be a global variable?

Comment 14

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #13)
> Comment on attachment 8805841 [details] [diff] [review]
> ContactProperties.patch v5
> 
> Review of attachment 8805841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
> @@ +5,5 @@
> >  #
> >  # The following are used by the Mailing list dialog
> >  #
> > +
> > +# %S will be the Mailing List's display name
> 
> Please use the proper format for a "localization note" comment.

What is the proper comment format? I copied mine from here https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties#15 and this file is full of this type.

Comment 15

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #14)
> What is the proper comment format? I copied mine from here
> https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> messenger/addressbook/addressBook.properties#15 and this file is full of
> this type.

That particular one is bad. But in that file there are also some that are correct.
See those starting with "## LOCALIZATION NOTE (<string name>):"

Comment 16

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v6

(In reply to :aceman from comment #13)
> Comment on attachment 8805841 [details] [diff] [review]
> ContactProperties.patch v5
> 
> Review of attachment 8805841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
> @@ +5,5 @@
> >  #
> >  # The following are used by the Mailing list dialog
> >  #
> > +
> > +# %S will be the Mailing List's display name
> 
> Please use the proper format for a "localization note" comment.

Fixed, found it myself in other files.

> :::
> mail/locales/en-US/chrome/messenger/addressbook/replicationProgress.
> properties
> @@ +14,5 @@
> >  downloadButton.accesskey=D
> >  cancelDownloadButton=Cancel Download
> >  cancelDownloadButton.accesskey=C
> > +
> > +directoryTitleNew=New LDAP Directory
> 
> Maybe we could name this "New connection to LDAP Directory" to be specific
> we do not create a directory on the server.

I'll leave it to be consistent with the menuitem. Your proposal is technically correct but the shorter form is colloquially more used.

> @@ +15,5 @@
> >  cancelDownloadButton=Cancel Download
> >  cancelDownloadButton.accesskey=C
> > +
> > +directoryTitleNew=New LDAP Directory
> > +# %S will be the LDAP directory's display name
> 
> Localization note format.

Fixed

> ::: mailnews/addrbook/prefs/content/pref-directory-add.js
> @@ +51,4 @@
> >             + ex + "\n");
> >      }
> >  
> > +    gOldListName = gCurrentDirectory.dirName;
> 
> Does this need to be a global variable?

No, fixed

Comment 17

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v6a

previous was not the patch I wanted to dend.

Comment 18

[:aceman]

Comment on attachment 8805853 [details] [diff] [review]
ContactProperties.patch v6a

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

I can accept this. Because the dialog titles are not consistent across TB. I've found the dialog named "Identities for <account name>", but when editing one identity (or one account) there is no name in the title.

Can I ask you guys to make a new bug to sort out the rules for this for all of TB?

::: mail/locales/en-US/chrome/messenger/addressbook/replicationProgress.properties
@@ +15,5 @@
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory
> +# %S will be the LDAP directory's display name

localization note

@@ +16,5 @@
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory
> +# %S will be the LDAP directory's display name
> +directoryTitleEdit=Edit %S

Is this correct that this only shows "Edit <name"? Why not LDAP directory or something?

::: suite/locales/en-US/chrome/mailnews/pref/replicationProgress.properties
@@ +15,5 @@
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory
> +# %S will be the LDAP directory's display name

localization note

Comment 19

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v6b

(In reply to :aceman from comment #18)
> Comment on attachment 8805853 [details] [diff] [review]
> ContactProperties.patch v6a
> 
> Review of attachment 8805853 [details] [diff] [review]:
> -----------------------------------------------------------------
> :::
> mail/locales/en-US/chrome/messenger/addressbook/replicationProgress.
> properties
> @@ +15,5 @@
> >  cancelDownloadButton=Cancel Download
> >  cancelDownloadButton.accesskey=C
> > +
> > +directoryTitleNew=New LDAP Directory
> > +# %S will be the LDAP directory's display name
> 
> localization note

Sorry, not good checked. Now fixed.

> @@ +16,5 @@
> >  cancelDownloadButton.accesskey=C
> > +
> > +directoryTitleNew=New LDAP Directory
> > +# %S will be the LDAP directory's display name
> > +directoryTitleEdit=Edit %S
> 
> Is this correct that this only shows "Edit <name"? Why not LDAP directory or
> something?

This is after a direct interaction of the selected AB (context/main menu or double click) and there is no need to highlight the type. The icon of the AB in the tree is also a type indicator.

> ::: suite/locales/en-US/chrome/mailnews/pref/replicationProgress.properties
> @@ +15,5 @@
> >  cancelDownloadButton=Cancel Download
> >  cancelDownloadButton.accesskey=C
> > +
> > +directoryTitleNew=New LDAP Directory
> > +# %S will be the LDAP directory's display name
> 
> localization note

fixed

Comment 20

[Thomas D. (:thomas8)]

>+# %S will be the LDAP directory's display name
>+directoryTitleEdit=Edit %S

Richard, as I explained before, I think we can't have "Edit {My LDAP Directory}" because that would wrongly suggest that you can edit the *content* of that directory. The truth is that this is only about the properties of an LDAP directory connection. We will even make our users angry because it's one of the most-voted for features in TB, so to pretend we offer it whilst we do not is provocative.

On the context menu from my other bug, I think we agreed to use "Properties" for ABs and LDAP Dirs.
I think the correct caption here is:

>+# %S will be the LDAP directory's display name
>+directoryTitleProperties=%S Properties

Comment 21

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #20)
> I think the correct caption here is:
> 
> >+# %S will be the LDAP directory's display name
> >+directoryTitleProperties=%S Properties

(In reply to Thomas D., comment 2)

> Menu                             Button  Context Menu      Dialogue title 
> ------------------------------------------------------------------------------------------------
> Edit > Address Book Properties   Edit(3) Properties (*)(4) [#] Personal Address Book Properties(5)

So for LDAP, this would be:

> Menu                             Button  Context Menu      Dialogue title 
> ------------------------------------------------------------------------------------------------
> Edit > Address Book Properties   Edit    Properties        [#] My LDAP Dir Properties

Where "My LDAP Dir" is the customized name of an LDAP directory.
In the Edit Menu, I currently use generic "Address Book Properties" for both normal ABs and LDAP ABs, because the presentation of LDAP directory is no different from normal ABs. In fact, I wonder why we use "Directory" at all, but perhaps it's to indicate the different nature of LDAP Data Source which is on remote server.

If you want, we could be more distinctive and use another bug to change the main menu to:
> Edit > LDAP Directory Properties

Comment 22

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v7

Edit LDAP title fixed.

Comment 23

[Thomas D. (:thomas8)]

Introducing some nice dynamic strings to polish the captions of various cmd_properties dialogues in AB.

Comment 24

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #8)
> (In reply to Thomas D. (needinfo?me) from comment #6)
> > Please also make the dialogue titles of Address Book and LDAP Directory
> > Properties be dynamic and include the name.
> 
> LDAP ok, but AB is overkill as there is only one field with the name in this
> dialog.

I hear you, but I think we should be internally and externally consistent, and also have the name of the address book in the dialogue caption:

> Personal Address Book Properties

After bug 1308776, there'll be more than one field :) Having the actual name of the currently edited object in the dialogue title bar is much more easily visible than in some field. There's duplication either way, because right now you have:

[*Address Book* Properties                   ]
|*Address Book* Name: [Personal Address Book]|

So why not have:

[Personal Address Book Properties            ]
|Address Book Name: [Personal Address Book]|

Otherwise it's a matter of principle, no need to have an exception here...
Windows Explorer also has the file name in the title, and just below, there's the input field with the same file name for renaming. I don't think there's anything odd or special about that, it's two different functions, one to have an identifying title, the other for renaming.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v8

Okay Thomas, you persuaded me.

Aceman, the only change is the dynamic Address Book dialog.

Now it's really feature complete.

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 8807288 [details] [diff] [review]
ContactProperties.patch v8

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

Nice. This is getting better with every iteration. I had a look at the code updating the dialogue title when you change the name of a contact, and it's quite simple and straightforward. So I think we can easily do the same for Mailing Lists, ABs, and LDAP Dirs. We can do that here or in another bug, but the most important part is to land the required strings now (for the "New (object): %name" case), which is also really simple and straightforward.

::: mail/components/addrbook/content/abMailListDialog.xul
@@ +9,5 @@
>  <!DOCTYPE dialog SYSTEM "chrome://messenger/locale/addressbook/abMailListDialog.dtd">
>  
>  <dialog xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>          id="ablistWindow"
> +        title="&mailListWindowAdd.title;"

Remove title attribute here

::: mail/locales/en-US/chrome/messenger/addressbook/abMailListDialog.dtd
@@ +2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- Title -->
> +<!ENTITY mailListWindowAdd.title        "New Mailing List">

Remove this string, now in the properties file.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  #
>  # The following are used by the Mailing list dialog
>  #

mailingListTitleNew=New Mailing List
## LOCALIZATION NOTE (mailingListTitleNewWithName): %S will be replaced by the Mailing List's display name
mailingListTitleNewWithName=New Mailing List: %S

Title from dtd file now here; to get the string from here, pls update OnLoadNewMailList() in abMailListDialog.js with something like this (pls verify):

bundle=document.getElementById("bundle_addressBook");
document.title=bundle.getString("mailingListTitleNew");

@@ +11,3 @@
>  emptyListName=You must enter a list name.
>  lastFirstFormat=%S, %S
>  firstLastFormat=%S %S  

As Aceman hinted, we can and should remove the trailing spaces. They have no effect, because %S will simply be replaced by their values:
> displayName = (gEditCard.displayLastNameFirst)
>      ? gAddressBookBundle.getFormattedString("lastFirstFormat", [lastNameValue, firstNameValue])
>      : gAddressBookBundle.getFormattedString("firstLastFormat", [firstNameValue, lastNameValue]);

@@ +164,5 @@
>  headingDescription=Description
>  headingAddresses=Addresses
>  
>  # For address books
> +addressBookTitleNew=New Address Book

## LOCALIZATION NOTE (addressBookTitleNewWithName): %S will be replaced by the the Address Book's name
addressBookTitleNewWithName=New Address Book: %S

::: mail/locales/en-US/chrome/messenger/addressbook/replicationProgress.properties
@@ +14,5 @@
>  downloadButton.accesskey=D
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory

## LOCALIZATION NOTE (directoryTitleNewWithName): %S will be replaced by the LDAP directory's display name
directoryTitleNewWithName=New LDAP Directory: %S

::: mailnews/addrbook/content/abAddressBookNameDialog.js
@@ +30,5 @@
>    // rename).
>    var bundle = document.getElementById("bundle_addressBook");
>  
> +  if (gDirectory) {
> +    let oldListName = gNameInput.value;

This works, but I think this should be consistent with same code elsewhere:
let oldListName = gDirectory.dirName;

::: suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  #
>  # The following are used by the Mailing list dialog
>  #

For Suite, we can also land the new strings now so that they are available when we get round to making the titles update dynamically upon name changes in the dialogue.
mailingListTitleNew=New Mailing List
## LOCALIZATION NOTE (mailingListTitleNewWithName): %S will be replaced by the Mailing List's display name
mailingListTitleNewWithName=New Mailing List: %S

@@ +156,5 @@
>  headingDescription=Description
>  headingAddresses=Addresses
>  
>  # For address books
> +addressBookTitleNew=New Address Book

## LOCALIZATION NOTE (addressBookTitleNewWithName): %S will be replaced by the the Address Book's name
addressBookTitleNewWithName=New Address Book: %S

::: suite/locales/en-US/chrome/mailnews/pref/replicationProgress.properties
@@ +14,5 @@
>  downloadButton.accesskey=D
>  cancelDownloadButton=Cancel Download
>  cancelDownloadButton.accesskey=C
> +
> +directoryTitleNew=New LDAP Directory

## LOCALIZATION NOTE (directoryTitleNewWithName): %S will be replaced by the LDAP directory's display name
directoryTitleNewWithName=New LDAP Directory: %S

Comment 27

[:aceman]

(In reply to Thomas D. (needinfo?me) from comment #26)
> @@ +11,3 @@
> >  emptyListName=You must enter a list name.
> >  lastFirstFormat=%S, %S
> >  firstLastFormat=%S %S  
> 
> As Aceman hinted, we can and should remove the trailing spaces. They have no
> effect, because %S will simply be replaced by their values:

I've done this in a new bug and there needs to be some communication to translators so do not do bother with it in this bug.

Comment 28

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #27)
> > As Aceman hinted, we can and should remove the trailing spaces. 
> I've done this in a new bug and there needs to be some communication to
> translators so do not do bother with it in this bug.

Kindly let us know the bug in which you are doing those good things ;)

Comment 29

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (needinfo?me) from comment #26)
> Comment on attachment 8807288 [details] [diff] [review]
> ContactProperties.patch v8
> 
> Review of attachment 8807288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. This is getting better with every iteration. I had a look at the code
> updating the dialogue title when you change the name of a contact, and it's
> quite simple and straightforward. So I think we can easily do the same for
> Mailing Lists, ABs, and LDAP Dirs. We can do that here or in another bug,
> but the most important part is to land the required strings now (for the
> "New (object): %name" case), which is also really simple and straightforward.

No, I don't want to make the "New" dialog titles dynamic. This isn't a real improvement for the user and as you wrote somewhere above, you are also not sure if this realtime update is really helpful. Only do it because we can do it is also not a good reason.

> ::: mailnews/addrbook/content/abAddressBookNameDialog.js
> @@ +30,5 @@
> >    // rename).
> >    var bundle = document.getElementById("bundle_addressBook");
> >  
> > +  if (gDirectory) {
> > +    let oldListName = gNameInput.value;
> 
> This works, but I think this should be consistent with same code elsewhere:
> let oldListName = gDirectory.dirName;

Let's decide aceman or Ian which we should use as:
gNameInput.value = gDirectory.dirName;

Comment 30

[:aceman]

(In reply to Thomas D. (needinfo?me) from comment #28)
> (In reply to :aceman from comment #27)
> > > As Aceman hinted, we can and should remove the trailing spaces. 
> > I've done this in a new bug and there needs to be some communication to
> > translators so do not do bother with it in this bug.
> 
> Kindly let us know the bug in which you are doing those good things ;)

Bug 1313942 as linked in the Blocks field.

Comment 31

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #29)
> No, I don't want to make the "New" dialog titles dynamic. This isn't a real
> improvement for the user and as you wrote somewhere above, you are also not
> sure if this realtime update is really helpful. Only do it because we can do
> it is also not a good reason.

I agree.

> > ::: mailnews/addrbook/content/abAddressBookNameDialog.js
> > @@ +30,5 @@
> > >    // rename).
> > >    var bundle = document.getElementById("bundle_addressBook");
> > >  
> > > +  if (gDirectory) {
> > > +    let oldListName = gNameInput.value;
> > 
> > This works, but I think this should be consistent with same code elsewhere:
> > let oldListName = gDirectory.dirName;
> 
> Let's decide aceman or Ian which we should use as:
> gNameInput.value = gDirectory.dirName;

Yes they are the same inside that function. I'd vote for gDirectory.dirName so the intention is clearer.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v9

Using gDirectory.dirName

Comment 33

[:aceman]

Comment on attachment 8807852 [details] [diff] [review]
ContactProperties.patch v9

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

Thanks.
Can you NOT make all the unrelated trailing spaces cleanup, especially in comments? It makes history harder to follow.

Comment 34

[Richard Marti (:Paenglab)]

Attachment: ContactProperties.patch v10

White space changes removed.

Comment 35

[Ian Neal]

Comment on attachment 8807877 [details] [diff] [review]
ContactProperties.patch v10

LGTM r/a=me

Comment 36

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/f861478300e63d3c53f4c4054a6f5243f884f627