+++ This bug was initially created as a clone of Bug #1319409 +++ Confirmation prompts for deleting things in the AB are pretty inconsistent. Deleting a mailing list from directory tree has "Delete Mailing List". Deleting a mailing list from ABView results pane has "Confirm". Same for all other deletable items in results pane and combinations thereof, just "Confirm". Let's tidy up here and add the missing explicit titles to make the prompts consistent, regardless which pane is used for selection. While we are here, fix the misleading message for removing contacts from mailing lists, which currently (wrongly) claims to "Delete Contact(s)". Yet for mailing lists, it's really "Remove Contact" in the most literal sense, not "Delete". Contact will remain in containing AB even after removing from mailing list. So let's be precise and less scary: [Remove Contact|s] Are you sure you want to remove the selected contact|s from this mailing list? Again while we are here, make code more readable by reducing needless reduncancy, which also helps us to add the titles easily without clutter.

Jörg, fyi, more l10n strings here, rounding off our smart dialogue renovation spree in Address Book, for TB 52. See patch in my next comment.

Let's have smarter delete prompts! Patch to be applied on top of my other patches: Bug 1308776 attachment 8812593 [details] [diff] [review] Bug 196135 attachment 8813181 [details] [diff] [review] Aceman, this is a straightforward 5 minutes review (really...) Richard, this blends in nicely with the other smart dialogue titles. More details in comment 0.

Comment on attachment 8813352 [details] [diff] [review] Patch V.1 - Smart titles logic for delete confirmation prompts Looks good, but two questions. What about adding the AB/Mailing list/Contact name into the dialog text? But only when one of them is selected. This makes it easier to control what will be deleted. Also shouldn't we note in AB delete dialog that all containing contacts will be deleted too? This should be to prevent data loss. However when deleting a AB the contacts stay in the contacts list until you select a other AB. This seems to be a other bug and not related to your patches, when deleting a AB there should be selected a other entry in the AB list.

(In reply to Richard Marti (:Paenglab) from comment #3) > Comment on attachment 8813352 [details] [diff] [review] > Patch V.1 - Smart titles logic for delete confirmation prompts > > Looks good, but two questions. > > What about adding the AB/Mailing list/Contact name into the dialog text? But > only when one of them is selected. This makes it easier to control what will > be deleted. Nice! I had already thought of that, but shied away for reasons of time... But after all, I couldn't resist... but it took hours to get everything right... So yes, it's implemented. > Also shouldn't we note in AB delete dialog that all containing contacts will > be deleted too? This should be to prevent data loss. Done. Short and simple. In anticipation of your next question, and looking at the file deletion prompt on Windows 10, I also included the number of selected items in the message text. So when you intend to delete two, but accidentally select three, at least you can't blame Thunderbird because we'll tell you EXACTLY what you're about to do... (ux-error-prevention). Having "Multiple" in the titles for multiple selected items like "Delete Multiple Contacts" is also an active contribution to ux-error-prevention, so that the titles of single vs. multiple items become more distinguishable. Windows 10 also has that, and it definitely makes sense. So this works perfectly according to specs, and smarter than ever, so I'm expecting big smiles from Richard... Jörg, if you could kindly check the code for correctness. It's enough if this code does what it should. We're not here for perfection, and I'm not willing to spend more time on purely cosmetical changes. Others may feel free to optimize the code even more than I did.

Comment on attachment 8815314 [details] [diff] [review] Patch V.2: Extra smart and context-sensitive deletion prompts! I can't apply the patch. Also the patches from Bug 1308776 and Bug 196135 don't apply. Is there any other patch I need?

I think you need bug 1319409 first. Then bug 196135 and then bug 1308776 (or vice-versa) and then this.

Comment on attachment 8815314 [details] [diff] [review] Patch V.2: Extra smart and context-sensitive deletion prompts! Yes bug 1319409 was missing (it needed small fixes to apply). This looks good. Only one thing, deleting a LDAP says also it would delete the contacts, which isn't correct. Maybe we need a different text here because we don't delete the directory and it's contacts but only the connection to it.

Comment on attachment 8815314 [details] [diff] [review] Patch V.2: Extra smart and context-sensitive deletion prompts! Review of attachment 8815314 [details] [diff] [review]: ----------------------------------------------------------------- Aceman, can you please take a look at the plurals here. After all the discussion we had in bug 1320570 (caused by bug 1313950). ::: mail/components/addrbook/content/abCommon.js @@ +343,5 @@ > + "confirmRemove" > + : "confirmDelete"; > + confirmDeleteMessageID = (numSelectedItems == 1) ? > + confirmDeleteMessageIDstub + "ThisContact" > + : confirmDeleteMessageIDstub + "TheseContacts"; Is this the right way to do the plurals?

Comment on attachment 8815314 [details] [diff] [review] Patch V.2: Extra smart and context-sensitive deletion prompts! Review of attachment 8815314 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, this is incorrect. ::: mail/components/addrbook/content/abCommon.js @@ +247,5 @@ > if (Services.prefs.getCharPref("mail.collect_addressbook") == aURI && > Services.prefs.getBoolPref("mail.collect_email_address_outgoing")) { > let brandShortName = document.getElementById("bundle_brand").getString("brandShortName"); > + confirmDeleteMessageID = "confirmDeleteCollectionAddressbook"; > + placeholderValue = brandShortName; Why is this called placeholderValue when you only ever put app name into it? @@ +260,5 @@ > + confirmDeleteMessage = gAddressBookBundle.getFormattedString(confirmDeleteMessageID, [placeholderValue]); > +} else { > + // No need for l10n placeholders here because we can construct this ourselves. > + confirmDeleteMessage = gAddressBookBundle.getString(confirmDeleteMessageID) + > + "\n\n" + directory.dirName; You can't construct this like this. And why is using the string differently based on non-empty placeholderValue ? @@ +343,5 @@ > + "confirmRemove" > + : "confirmDelete"; > + confirmDeleteMessageID = (numSelectedItems == 1) ? > + confirmDeleteMessageIDstub + "ThisContact" > + : confirmDeleteMessageIDstub + "TheseContacts"; No, this is incorrect, the patch in attachment 8815049 [details] [diff] [review] contains the whole pattern that needs to be done. Also then you do not need this hackery of building the string ID (which should be avoided anyway as then you can't grep for the string ID in code if you are looking for where it is used). @@ +362,5 @@ > + : [numSelectedItems]; > + } else { // numSelectedItems == 1 > + placeholderArray = selectedDir.isMailList? > + [placeholderListName] > + : null; What is this trying to do? @@ +374,5 @@ > + > + // Get the confirmation prompt strings > + confirmDeleteMessage = placeholderArray ? > + gAddressBookBundle.getFormattedString(confirmDeleteMessageID, placeholderArray) > + : gAddressBookBundle.getString(confirmDeleteMessageID); This also looks strange. Using the same string in different ways? Based on what? @@ +378,5 @@ > + : gAddressBookBundle.getString(confirmDeleteMessageID); > + // For single items, inlude the item name in the confirmation message. > + // No need for l10n placeholders here because we can construct this ourselves. > + confirmDeleteMessage += (numSelectedItems == 1) ? > + "\n\n" + itemName No, I don't think you can construct this 'ourselves'. Some language may want some other separator than \n\n.

Comment on attachment 8815314 [details] [diff] [review] Patch V.2: Extra smart and context-sensitive deletion prompts! Not ready yet, see previous comment.

After another 9 hours of work... 8| Including about 2 hours to write l10n comments... 8/ ...Code has been completely rewritten to use correct l10n plural forms!!! Because English and German are so boring in terms of Plural, just one or plenty! Other languages are much more differentiating in the most adventurous patterns... My favourite is Plural Rule #16 for Celtic (Breton), with as many as 6 different patterns for plural, and you won't guess how they do it, hehe... :p https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Plural_rule_.2316_.283_forms.29 Actually, PluralForm.jsm rocks and makes coding a lot easier!!! Just pass in the number of items, and get the correct string in return. Cool. Makes very slim code, and I also exercised my favorite hobby, to simplify or eliminate nested if structures... I also implemented context-sensitive prompt for LDAP, as proposed by Richard's comment 7, and I even touched up that unlikely "Delete Collection Address Book" prompt... (to test, create custom AB, and set pref mail.collect_addressbook=moz-abmdbdirectory://abook-1.mab if it's the first). So this looks much better, perfectly readable, and addresses all of Aceman's critical review in comment 9. It also works flawlessly on my local installation. Thanks a lot! Enjoy. Needless to say I want this landed with all those strings!!!

And charmingly, this also works in contacts sidebar! :)

Comment on attachment 8815740 [details] [diff] [review] Patch V.3: 100% localized, extra smart and extra context-sensitive deletion prompts! Looks good now. Thank you for this patch. One question. How do you manage to delete the collecting AB? Also with disabled collecting the deletion is disabled.

(In reply to Richard Marti (:Paenglab) from comment #13) > Comment on attachment 8815740 [details] [diff] [review] > Patch V.3: 100% localized, extra smart and extra context-sensitive deletion > prompts! > > Looks good now. Thank you for this patch. > > One question. How do you manage to delete the collecting AB? Also with > disabled collecting the deletion is disabled. I said you can test it on a custom AB which you declare as collection AB, then you'll see the collection AB delete message. Otherwise you are right, "Collected Addresses" can't be deleted even when it's probably no longer the collection AB because we picked another one. But I would not trust any of that without testing. After all, this is Thunderbird... It's a bit strange that even after changing your collection AB, we don't allow deleting the original one. I also wonder how we determine that quality if NOT by the pref. Or maybe we did but only once. Whatever...

Comment on attachment 8815740 [details] [diff] [review] Patch V.3: 100% localized, extra smart and extra context-sensitive deletion prompts! Review of attachment 8815740 [details] [diff] [review]: ----------------------------------------------------------------- Don't you add too many comments here? And the strings and their comments look quite over-engineered. You can't have semantically different strings depending on number of items (once an item name, once the item count). You need to simplify this more. ::: mail/components/addrbook/content/abCommon.js @@ +241,5 @@ > > + if (directory.isMailList) { > + // It's a mailing list. > + confirmDeleteMessageID = "confirmDelete1orMoreMailingLists"; > + confirmDeleteTitleID = "confirmDelete1orMoreMailingListsTitle"; How can you delete multiple mailinglists if only one aURI was passed in? @@ +284,5 @@ > + confirmDeleteMessage = confirmDeleteMessage.replace("#2", brandShortName); > + > + // Bail out if we couldn't set the confirmation prompt strings. > + // This should never happen... > + if (!confirmDeleteMessage || !confirmDeleteTitle) Do we check for string existence like this anywhere else? @@ +295,1 @@ > return; Please add {} around this 2 line block now (yes, technically not needed in JS but it looks strange now). @@ +348,5 @@ > + let confirmDeleteMessageSet; > + let confirmDeleteMessage; > + let itemName; > + let containingListName; > + let selectedDir=getSelectedDirectory(); spaces around = @@ +354,5 @@ > > + switch(types) { > + case kListsAndCards: > + // L10n caveat: This is a plural-only string; singular slot in the list > + // of plural forms must exist, but should be empty or "#noSingular". I think this is incorrect. @@ +403,5 @@ > + // If single selected item, substitute itemName. > + confirmDeleteMessage = confirmDeleteMessage.replace("#1", itemName); > + else > + // If multiple selected items, substitute numSelectedItems. > + confirmDeleteMessage = confirmDeleteMessage.replace("#2", numSelectedItems); This will not work, see my comment at the strings. @@ +419,5 @@ > + if (!Services.prompt.confirm(window, > + confirmDeleteTitle, > + confirmDeleteMessage)) > + // Deletion cancelled by user. > + return; Braces. ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties @@ +51,5 @@ > +# Example Singular: Are you sure you want to delete this contact? > +# <blank line> > +# John Doe > +# Example Plural: Are you sure you want to delete these 3 contacts? > +confirmDelete1orMoreContacts=Are you sure you want to delete this contact?\n\n#1;Are you sure you want to delete these #2 contacts? You can't have different strings and placeholders here. See e.g. Icelandic, where you have the same form for any number ending in 1 (1,21,31...). So your "one string for 1 and a different one for anything more" is not possible. @@ +74,5 @@ > +# Example Singular: Are you sure you want to remove this contact from the mailing list 'Customers List'? > +# <blank line> > +# John Doe > +# Example Plural: Are you sure you want to remove these 3 contacts from the mailing list 'Customers List'? > +confirmRemove1orMoreContacts=Are you sure you want to remove this contact from the mailing list '#3'?\n\n#1;Are you sure you want to remove these #2 contacts from the mailing list '#3'? Will not work. Icelandic sends you the first form also for 21,31 etc. with the #1 placeholder but you will not replace it because you do not do that for numSelectedItems > 1. @@ +111,5 @@ > +# LOCALIZATION NOTE (confirmDelete1orMoreContactsAndLists): > +# Semicolon list of singular and plural forms. > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > +# There's no singular for this string, so just keep #noSingular to fill the > +# singular position in the list. Please do no invent special keywords. And this will not work anyway. There seems to be no singular form (numSelectedItems == 1 only) for some languages so the comment can't say that. @@ +144,5 @@ > +# Example: If this address book is deleted, Thunderbird will no longer collect addresses. > +# Are you sure you want to delete this address book? > +# <blank line> > +# My Collecting Addressbook > +confirmDeleteThisCollectionAddressbook=If this address book is deleted, #2 will no longer collect addresses.\nAre you sure you want to delete this address book?\n\n#1 Why is the AB name so separated out on new line? I don't think we do that anywhere. What about "Are you sure you want to delete the address book '#1'?"

Wow. Even though I'm a linguist, turns out it's hard to grasp the grammar of languages that don't have a dedicated concept of "Singular" vs. "Plural" forms. I also suspect that the languages with such exotic concepts are probably the languages for which we don't have translations, and that there's probably dozens of other strings in our application which aren't right on this issue, including probably the current versions of the strings we are changing here. So let's try another round for perfection in other languages... So this should work, isn't it: > confirmDelete1Contact=Are you sure you want to delete this contact?\n\n#1 So the above becomes a simple string with simple substitution, not using plural forms. Any language will just translate so that it sounds right in their concept. > confirmDeleteContacts=Are you sure that you want to delete this #1 contact?;Are you sure you want to delete these #1 contacts? This is a plural form so we need to use semicolon separated list of plural forms. I maintain that this form is inherently plural-only, we'll never use any form of this for num=1. And think we should let localizers know even if their concept of singular vs. plural is different or non-existing. But you are right that I cannot assume a dedicated singular slot in other languages, so I'll rephrase my comment accordingly. And I have also decided hat for the avoidance of doubt, I'll just fill the EN singular slot even though we'll never use it. So for other languages, it will be a better example to just fill all of their slots. Like Icelandic where the first slot will be used for any num ending in 1 except 11, so they actually have to fill it for num=[1], 21, 31, 41, etc. It gets harder for this one, though: a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete this #1 contacts and mailing lists?;Are you sure you want to delete these #1 contacts? b) confirmDeleteContactsAndMailingLists=;Are you sure you want to delete these #1 contacts? c) confirmDeleteContactsAndMailingLists=xxx;Are you sure you want to delete these #1 contacts? "Contacts and Mailinglists" is inherently plural and can never be singular in EN. We need to use semicolon-separated plural forms because other languages use different plural forms depending on number. a) Filling the first slot in EN violates EN grammar rules, and if they look up our concept of singular vs. plural, they might also be irritated why we are filling it though it can never apply. b) Not filling it might irritate other language with other concepts, and perhaps seduce them to omit slots, i.e. to skip one semicolon, which then breaks the whole system in their language. c) Maybe using a placeholder to indicate our empty slot in EN is best? I'd still think that using a more descriptive placeholder in EN might assist other languages to understand what is happening here: What about "(empty num=1 singular slot in English);..." So what should I do? a), b), or c), or something else? Please advise. This is mind-warping.

(In reply to Thomas D. (needinfo?me) from comment #16) > It gets harder for this one, though: > > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > contacts? Sorry, copy and paste error, forgot the word mailing lists at the end of the strings; also added some *emphasis* but asterix are not part of the strings: > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > contacts and mailing lists? > b) confirmDeleteContactsAndMailingLists=*;*Are you sure you want to delete > these #1 contacts and mailing lists? > c) confirmDeleteContactsAndMailingLists=*xxx*;Are you sure you want to delete > these #1 contacts and mailing lists? > So what should I do? a), b), or c), or something else? Please advise. This > is mind-warping. Or maybe we can do this: a2) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete *these* #1 contacts and mailing lists?;Are you sure you want to delete these #1 contacts and mailing lists? That way, we're still filling a slot which should be empty in EN, but at least we're using correct grammar in that slot. So we're giving others an example to fill all of their slots, that's hoping that they don't see the wrongness of filling the first slot in EN. I guess there's no perfect solution, and this really needs a well-thought-out comment.

ni?aceman for comment 16 and comment 17

(In reply to Thomas D. (needinfo?me) from comment #16) > So this should work, isn't it: > > > confirmDelete1Contact=Are you sure you want to delete this contact?\n\n#1 > > So the above becomes a simple string with simple substitution, not using > plural forms. Any language will just translate so that it sounds right in > their concept. I think so. But I would make it "Are you sure you want to delete the contact "#1"?" > > confirmDeleteContacts=Are you sure that you want to delete this #1 contact?;Are you sure you want to delete these #1 contacts? > > This is a plural form so we need to use semicolon separated list of plural > forms. > I maintain that this form is inherently plural-only, we'll never use any > form of this for num=1. And think we should let localizers know even if > their concept of singular vs. plural is different or non-existing. But you > are right that I cannot assume a dedicated singular slot in other languages, > so I'll rephrase my comment accordingly. And I have also decided hat for the > avoidance of doubt, I'll just fill the EN singular slot even though we'll > never use it. So for other languages, it will be a better example to just > fill all of their slots. Like Icelandic where the first slot will be used > for any num ending in 1 except 11, so they actually have to fill it for > num=[1], 21, 31, 41, etc. I would make it "Are you sure that you want to delete the selected contact?;Are you sure that you want to delete the selected #1 contacts?" You can write the comment that this string is never called for #1 == 1, but you still fill it in even for English. Some languages will still use the first form. > It gets harder for this one, though: > > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > contacts? > b) confirmDeleteContactsAndMailingLists=;Are you sure you want to delete > these #1 contacts? > c) confirmDeleteContactsAndMailingLists=xxx;Are you sure you want to delete > these #1 contacts? > > So what should I do? a), b), or c), or something else? Please advise. This > is mind-warping. (In reply to Thomas D. (needinfo?me) from comment #17) > (In reply to Thomas D. (needinfo?me) from comment #16) > > It gets harder for this one, though: > > > > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > > contacts? > > Sorry, copy and paste error, forgot the word mailing lists at the end of the > strings; also added some *emphasis* but asterix are not part of the strings: > > > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > > contacts and mailing lists? > > b) confirmDeleteContactsAndMailingLists=*;*Are you sure you want to delete > > these #1 contacts and mailing lists? > > c) confirmDeleteContactsAndMailingLists=*xxx*;Are you sure you want to delete > > these #1 contacts and mailing lists? > > > So what should I do? a), b), or c), or something else? Please advise. This > > is mind-warping. > > a2) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > *these* #1 contacts and mailing lists?;Are you sure you want to delete these > #1 > contacts and mailing lists? > > That way, we're still filling a slot which should be empty in EN, but at > least we're using correct grammar in that slot. So we're giving others an > example to fill all of their slots, that's hoping that they don't see the > wrongness of filling the first slot in EN. I guess there's no perfect > solution, and this really needs a well-thought-out comment. I would suggest: a3) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete the selected contact *or* mailing list?;Are you sure you want to delete the selected #1 contacts and mailing lists? You can put the comment that this is never called with #1 == 1. It is not important that in EN the first for is never used. For other languages it will. I think we already had a case like this in another bug and we just filled in the singular form for English correctly. Also, we have strings that do not contain the #1 placeholder (do not show the number of items). E.g. in messenger.properties: markFolderRead=Mark Folder Read;Mark Folders Read In this case translators also need to be clever and fill in all their forms, but in a neutral way not knowing how many contacts there are. So e.g. in my language where we have 3 forms (num=1,num=2-4,num>4) the translator puts the strings identical for form 2 and 3, which wouldn't be the case if the num was shown in a placeholder. Hopefully that is possible for all languages, we didn't get a complaint for those strings AFAIK.

(In reply to :aceman from comment #19) > (In reply to Thomas D. (needinfo?me) from comment #16) > > So this should work, isn't it: > > > > > confirmDelete1Contact=Are you sure you want to delete this contact?\n\n#1 > > > > So the above becomes a simple string with simple substitution, not using > > plural forms. Any language will just translate so that it sounds right in > > their concept. > > I think so. But I would make it "Are you sure you want to delete the contact > "#1"?" No. Three reasons to keep the item name on a separate line: 1) Salience. Hiding it in the sentence makes the name stand out less, but the name is really what matters most. The sentence is actually just a more verbose repetition of the title, so it's not really important. But it's important, and should be as easy as possible to parse/check visually, which exact item name will get deleted. Windows Explorer also does it exactly like that, and they certainly have their reasons. 2) Unknown item name length. The length of the name is totally unknown to us. At the end, you can neither read the name properly (because it's somewhere in the sentence), nor the sentence (because the name is disturbing, worse when it might be in middle position in other languages, like "Are you sure deleting 'Active customers address book archive 2016-August' is what you want?). 3) Avoiding grammatical oddness in other languages. Embedding the unmodified item name into the sentence is easy for languages like English and German, but maybe not for every language. "Are you sure you want to delete 'itemName'" requires item name to be in object case or whatever way the language uses for objects, but we don't have item names in object case, we just have item names. Depending on the language, this might sound really odd if rules for objects are not correctly followed. > > > confirmDeleteContacts=Are you sure that you want to delete this #1 contact?;Are you sure you want to delete these #1 contacts? > > > > This is a plural form so we need to use semicolon separated list of plural > > forms. > > I maintain that this form is inherently plural-only, we'll never use any > > form of this for num=1. And think we should let localizers know even if > > their concept of singular vs. plural is different or non-existing. But you > > are right that I cannot assume a dedicated singular slot in other languages, > > so I'll rephrase my comment accordingly. And I have also decided hat for the > > avoidance of doubt, I'll just fill the EN singular slot even though we'll > > never use it. So for other languages, it will be a better example to just > > fill all of their slots. Like Icelandic where the first slot will be used > > for any num ending in 1 except 11, so they actually have to fill it for > > num=[1], 21, 31, 41, etc. > > I would make it "Are you sure that you want to delete the selected > contact?;Are you sure that you want to delete the selected #1 contacts?" > You can write the comment that this string is never called for #1 == 1, but > you still fill it in even for English. Some languages will still use the > first form. I don't think it's wise to skip the #1 placeholder in EN if we can easily have it. Localizers might be tempted to do the same. Icelander looks at first slot, there's no #1 placeholder, so you end up having first slot in icelandic without placeholder. Risky. > > It gets harder for this one, though: > > > > > a) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > > > this #1 contacts and mailing lists?;Are you sure you want to delete these #1 > > > contacts and mailing lists? > > > b) confirmDeleteContactsAndMailingLists=*;*Are you sure you want to delete > > > these #1 contacts and mailing lists? > > > c) confirmDeleteContactsAndMailingLists=*xxx*;Are you sure you want to delete > > > these #1 contacts and mailing lists? > > > > > So what should I do? a), b), or c), or something else? Please advise. This > > > is mind-warping. > > > > a2) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > > *these* #1 contacts and mailing lists?;Are you sure you want to delete these > > #1 > > contacts and mailing lists? > > > > That way, we're still filling a slot which should be empty in EN, but at > > least we're using correct grammar in that slot. So we're giving others an > > example to fill all of their slots, that's hoping that they don't see the > > wrongness of filling the first slot in EN. I guess there's no perfect > > solution, and this really needs a well-thought-out comment. > > I would suggest: > a3) confirmDeleteContactsAndMailingLists=Are you sure that you want to delete > the selected contact *or* mailing list?;Are you sure you want to delete the > selected > #1 contacts and mailing lists? > > You can put the comment that this is never called with #1 == 1. It is not > important that in EN the first for is never used. For other languages it > will. I think that's very risky in terms of UX error prevention. The word "or" must never appear in this string, so putting it in the template imo has a high risk of causing wrong translations which just pick up the "or" because they don't know which item combinations we really check for. Iow, they can't know that we only put contact *or* mailing list here but it can never occur in the product. So they can feel obliged to translate this for their first slots exactly as we offer them. So I think the choice is really between blank slot, random placeholder, or 2a), correct grammar but will never be used for EN, which they may not know. We want them to fill all of their slots, so I think filling ours with 2a) is wise. > I think we already had a case like this in another bug and we just filled in > the singular form for English correctly. But there is no singular form used in the product here, and it's impossible to fill singular because "contacts and mailing lists" is already plural, so the only way to make it singular is OR but I have already argued against that. > Also, we have strings that do not contain the #1 placeholder (do not show > the number of items). > E.g. in messenger.properties: markFolderRead=Mark Folder Read;Mark Folders > Read > > In this case translators also need to be clever and fill in all their forms, > but in a neutral way not knowing how many contacts there are. > So e.g. in my language where we have 3 forms (num=1,num=2-4,num>4) the > translator puts the strings identical for form 2 and 3, which wouldn't be > the case if the num was shown in a placeholder. Hopefully that is possible > for all languages, we didn't get a complaint for those strings AFAIK. Yeah, it's tricky.

Alright, reshuffled the strings again, and almost back to where I came from. I decided to follow a very simple strategy for the plural-only stringsets: In the EN singular slot, which will never be used because we only request this string for num > 1, just have the full plural form which we have for num > 1, and which is expected from other translations for this string, namely including the #1 placeholders for the number of items. For the simple reason that if they use us as a template, let's not irritate them with having anything singular here without #1 placeholder. Especially for languages lice Icelandic, who need to fill their first slot with the correct plural form including #1, which unfortunately includes EN singular for num=1, so now if they just copy our singular slot string, which we offer as a plural string, and which covers certain plural forms for them, it'll come out right. And since our first form is never used, I think otherwise it doesn't matter what we put there, as long as it's safe as a template.

Clearly missed all deadlines. Not tracking any more.

(In reply to Jorg K (GMT+1) from comment #22) > Clearly missed all deadlines. Not tracking any more. This is ready to land. Acemans review has been fully addressed, strings fixed accordingly. Can land with or without the getSelectedDirectory() bug, just find and replace 3 or 4 callers. If there are any doubts on the code, at least land the strings.

(In reply to Thomas D. (needinfo?me) from comment #23) > This is ready to land. Acemans review has been fully addressed, strings > fixed accordingly. It will land on C-C once reviewed. It missed TB 52 ESR (as has bug 1308776): https://groups.google.com/d/msg/mozilla.dev.l10n/D8tX-xjAOQU/RaJRW_DaCQAJ As I said previously, bugs with string change *must* be ready by the branch date. Only this time we have an exceptionally long Aurora period and a super important bug that held us up. Some bugs got a free ride ("Trittbrettfahrer"), but now the ship has left.

If you're wondering why I haven't reviewed this yet: I'll let Aceman go first.

Aceman, given your previous reviews which I have addressed, can this be reviewed by code inspection since your machine is down? Or should we ask Jörg to take this review?

Comment on attachment 8816881 [details] [diff] [review] Patch V.4: 101% localized, extra smart and extra context-sensitive deletion prompts! Review of attachment 8816881 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the new version. I hope we are not running in circles and you can understand what I mean here. ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties @@ +46,5 @@ > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > +# Note: The number of items is always more than 1 and will appear in the > +# confirmation prompt message below this title. > +# Example: Delete Multiple Contacts > +confirmDelete2orMoreContactsTitle=Delete Multiple Contacts;Delete Multiple Contacts If the number of items is not displayed in the string ever, I don't think we need to run this through plural forms.If the number is not shown in the string users may not know why a particular plural form was chosen. I'd hope all languages have a form for "delete (some number>1) of contacts" that we can use here. They should already have such for the mentioned markFolderRead string. @@ +53,5 @@ > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > +# #1 The number of selected contacts, always more than 1. > +# Don't localize \n\n#1 unless your local layout comes out wrong. > +# Example: Are you sure you want to delete these 3 contacts? > +confirmDelete2orMoreContacts=Are you sure you want to delete these #1 contacts?;Are you sure you want to delete these #1 contacts? So why is the first form in English plural too (contacts)? Will that not make translators think they should use the same string for all forms (as in the title)? We must make it obvious that this should be filled properly with all the various plural forms. For many languages the information that number is more than one is irrelevant, because the form for 1 (but also e.g. 11) will be used. These 2 questions repeat for most of the strings as they use the same pattern. @@ +64,5 @@ > +# Don't localize \n\n#1 unless your local layout comes out wrong. > +# Example: Are you sure you want to remove this contact from the mailing list 'Customers List'? > +# <blank line> > +# John Doe > +confirmRemoveThisContact=Are you sure you want to remove this contact from the mailing list '#3'?\n\n#1 Document #3, but it seems your added code never replaces #3 with anything so maybe you meant some other placeholder number. @@ +111,5 @@ > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > +# There's no number in this title, but the number of items is always more than 2 > +# and will appear in the confirmation prompt message. > +# Example: Delete Contacts and Mailing Lists > +confirmDelete2orMoreContactsAndListsTitle=#noSingular;Delete Contacts and Mailing Lists I don't like the special string "#noSingular". But you do not use it in the other strings above so this may just be a leftover.

(In reply to :aceman from comment #27) > Comment on attachment 8816881 [details] [diff] [review] > Patch V.4: 101% localized, extra smart and extra context-sensitive deletion > prompts! > > Review of attachment 8816881 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the new version. I hope we are not running in circles and you can > understand what I mean here. Thanks for the review, glad to see you back in reviews after your hardware issues. Let's try to run on and break out of the circles ;) > ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties > @@ +46,5 @@ > > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > > +# Note: The number of items is always more than 1 and will appear in the > > +# confirmation prompt message below this title. > > +# Example: Delete Multiple Contacts > > +confirmDelete2orMoreContactsTitle=Delete Multiple Contacts;Delete Multiple Contacts > > If the number of items is not displayed in the string ever, I don't think we > need to run this through plural forms. If the number is not shown in the > string users may not know why a particular plural form was chosen. I'd hope > all languages have a form for "delete (some number>1) of contacts" that we > can use here. Let's hope so, agreed. Fixed. > @@ +53,5 @@ > > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > > +# #1 The number of selected contacts, always more than 1. > > +# Don't localize \n\n#1 unless your local layout comes out wrong. > > +# Example: Are you sure you want to delete these 3 contacts? > > +confirmDelete2orMoreContacts=Are you sure you want to delete these #1 contacts?;Are you sure you want to delete these #1 contacts? > > So why is the first form in English plural too (contacts)? I explained my reason above, ... > Will that not > make translators think they should use the same string for all forms (as in > the title)? We must make it obvious that this should be filled properly with > all the various plural forms. ...but yours are also good and convincing. Fixed. It'll always feel wrong because now we're filling an English singular form for a string which can never be singular. So it's hard to tell which English example will confuse translators more. Let's try yours ;) > For many languages the information that number > is more than one is irrelevant, because the form for 1 (but also e.g. 11) > will be used. Yeah, but if we're misleadingly offering an English singular form which never gets used, we should at least explain ourselves. I think it helps to understand this form, even for languages which have different patterns. I don't think it's harmful. So I kept this comment. There's one form where we can't use singular in the English singular slot without breaking our grammar completely: > +confirmDelete2orMoreContactsAndLists= > Are you sure you want to delete these #1 contacts and mailing lists?; > Are you sure you want to delete these #1 contacts and mailing lists? Contacts and Mailing lists is inherently plural, no matter what. My linguistic conscience would not allow breaking all rules of grammar ;) So I hope this is least confusing; the previous strings have already shown to translators what is needed here. (I was talking about this in my comment 16 and asking for advice). > @@ +64,5 @@ > > +# Don't localize \n\n#1 unless your local layout comes out wrong. > > +# Example: Are you sure you want to remove this contact from the mailing list 'Customers List'? > > +# <blank line> > > +# John Doe > > +confirmRemoveThisContact=Are you sure you want to remove this contact from the mailing list '#3'?\n\n#1 > > Document #3, but it seems your added code never replaces #3 with anything so > maybe you meant some other placeholder number. Indeed, good catch. Fixed. > @@ +111,5 @@ > > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > > +# There's no number in this title, but the number of items is always more than 2 > > +# and will appear in the confirmation prompt message. > > +# Example: Delete Contacts and Mailing Lists > > +confirmDelete2orMoreContactsAndListsTitle=#noSingular;Delete Contacts and Mailing Lists > > I don't like the special string "#noSingular". But you do not use it in the > other strings above so this may just be a leftover. Yep. Fixed. Caveat: I didn't test the changes of this patch. Will this break out of the circle of iterations? Hope so...

Comment on attachment 8826701 [details] [diff] [review] Patch V.5: Let's try this again: Localized, smart and context-sensitive deletion prompts Review of attachment 8826701 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I think I like the plural forms now. I give r+ for the addressBook.properties. I have read the abCommon.js changes and commented on some nits. After you make the necessary changes I'd ask Jorg to run test the patch and r+ the code part. Thanks. ::: mail/components/addrbook/content/abCommon.js @@ +277,5 @@ > + let confirmDeleteTitle; > + let confirmDeleteMessageID; > + let confirmDeleteMessage; > + let brandShortName; > + let clearPrefsRequired = false; I see this was already there, but "clearCollectionPrefs" would be probably more correct. @@ +312,5 @@ > + // Replace #1 with the name of the selected address book or mailing list. > + confirmDeleteMessage = confirmDeleteMessage.replace("#1", directory.dirName); > + if (brandShortName) > + // For a collection address book, replace #2 with the brandShortName. > + confirmDeleteMessage = confirmDeleteMessage.replace("#2", brandShortName); Please add {} for this block. @@ +429,5 @@ > + // For numSelectedItems == 1, it's simple strings. > + // For messages with numSelectedItems > 1, it's multi-pluralform string sets. > + // confirmDeleteMessage has placeholders for some forms. > + confirmDeleteTitle = gAddressBookBundle.getString(confirmDeleteTitleID); > + confirmDeleteMessage = gAddressBookBundle.getString(confirmDeleteMessageID); Can this setting of confirmDeleteMessage be conditional on numSelectedItems == 1 so that we do not fetch the string needlessly for numSelectedItems > 1 (just a speed optimization). The next block would then be an "else" branch. @@ +452,5 @@ > + > + // Finally, show our smart confirmation message, and act upon it! > + if (!Services.prompt.confirm(window, > + confirmDeleteTitle, > + confirmDeleteMessage)) { Would this fit on a single line? If not, it seems off by 1 space. @@ +457,5 @@ > + // Deletion cancelled by user. > + return; > + } > + > + if (getSelectedDirectoryURI() == (kAllDirectoryRoot + "?")) { selectedDir.URI now that you already fetch it at the top of the function? ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties @@ +47,5 @@ > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > +# #1 The number of selected contacts, always more than 1. > +# Don't localize \n\n#1 unless your local layout comes out wrong. > +# Example: Are you sure you want to delete these 3 contacts? > +confirmDelete2orMoreContacts=Are you sure you want to delete this #1 contact?;Are you sure you want to delete these #1 contacts? I only have one nit in all the confirm*2orMore* strings: would "the selected" be better than "this/these" ? As the dialog does not show which ones are meant (the singular version does).

Another iteration... but at least we are no longer going in circles ;) This addresses Aceman's nits from comment #29, thanks. Jörg, Aceman suggested if you could "test the patch and r+ the code part"? (In reply to :aceman from comment #29) > Comment on attachment 8826701 [details] [diff] [review] > Patch V.5: Let's try this again: Localized, smart and context-sensitive > deletion prompts > > Review of attachment 8826701 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, I think I like the plural forms now. I give r+ for the > addressBook.properties. > I have read the abCommon.js changes and commented on some nits. > > After you make the necessary changes I'd ask Jorg to run test the patch and > r+ the code part. > Thanks. > > ::: mail/components/addrbook/content/abCommon.js > @@ +277,5 @@ > > + let confirmDeleteTitle; > > + let confirmDeleteMessageID; > > + let confirmDeleteMessage; > > + let brandShortName; > > + let clearPrefsRequired = false; > > I see this was already there, but "clearCollectionPrefs" would be probably > more correct. Done. > @@ +312,5 @@ > > + // Replace #1 with the name of the selected address book or mailing list. > > + confirmDeleteMessage = confirmDeleteMessage.replace("#1", directory.dirName); > > + if (brandShortName) > > + // For a collection address book, replace #2 with the brandShortName. > > + confirmDeleteMessage = confirmDeleteMessage.replace("#2", brandShortName); > > Please add {} for this block. Alright. > @@ +429,5 @@ > > + // For numSelectedItems == 1, it's simple strings. > > + // For messages with numSelectedItems > 1, it's multi-pluralform string sets. > > + // confirmDeleteMessage has placeholders for some forms. > > + confirmDeleteTitle = gAddressBookBundle.getString(confirmDeleteTitleID); > > + confirmDeleteMessage = gAddressBookBundle.getString(confirmDeleteMessageID); > > Can this setting of confirmDeleteMessage be conditional on numSelectedItems > == 1 so that we do not fetch the string needlessly for numSelectedItems > 1 > (just a speed optimization). The next block would then be an "else" branch. No, we're not fetching twice. That's why I have added detailed comments which describe exactly what we're doing. First round fetches the raw string from .properties file, which can be a simple singular form, or a semicolon-separated plural form: > confirmDeleteMessage = gAddressBookBundle.getString(confirmDeleteMessageID); > // confirmDeleteMessage = "Delete this contact" // for num == 1 [OR] > // confirmDeleteMessage = "Delete this #1 contact?; Delete these #1 contacts?" // for num > 1 For messages with num == 1, we're done (confirmDeleteMessage holds our simple confirmation msg string). If required (only for messages with num > 1), second round passes that semicolon-separated raw string into the Pluralform parser to extract the correct plural form: > if (numSelectedItems > 1) { > confirmDeleteMessage = PluralForm.get(numSelectedItems, confirmDeleteMessage); > // confirmDeleteMessage = "Delete these #1 contacts?" Thereafter, we replace the placeholders according to what they represent. All that using just a single variable. I think the code is pretty well organized here, condensed, straightforward and easy to read: moderate abstraction (focusing on the actual strings chosen), one step after the other (set message and title string IDs first, do centralized one-for-all string parsing later). > @@ +452,5 @@ > > + > > + // Finally, show our smart confirmation message, and act upon it! > > + if (!Services.prompt.confirm(window, > > + confirmDeleteTitle, > > + confirmDeleteMessage)) { > > Would this fit on a single line? If not, it seems off by 1 space. Doesn't fit on one line, but I condensed this into two lines, thanks. > @@ +457,5 @@ > > + // Deletion cancelled by user. > > + return; > > + } > > + > > + if (getSelectedDirectoryURI() == (kAllDirectoryRoot + "?")) { > > selectedDir.URI now that you already fetch it at the top of the function? Existing code, but yes, that's better. Done. > ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties > @@ +47,5 @@ > > +# See: http://developer.mozilla.org/docs/Localization_and_Plurals > > +# #1 The number of selected contacts, always more than 1. > > +# Don't localize \n\n#1 unless your local layout comes out wrong. > > +# Example: Are you sure you want to delete these 3 contacts? > > +confirmDelete2orMoreContacts=Are you sure you want to delete this #1 contact?;Are you sure you want to delete these #1 contacts? > > I only have one nit in all the confirm*2orMore* strings: would "the > selected" be better than "this/these" ? As the dialog does not show which > ones are meant (the singular version does). Hmmm. I followed Windows Explorer phrasing (try shift delete of multiple files), boldly assuming that their UX here is carefully-considered ;) To my sense of language (which is quite good, I studied English...), any combination of "the selected" with a number sounds odd: Delete the 2 selected contacts? (odd) Delete the selected 2 contacts? (that sounds ungrammatical) Delete these 2 selected contacts? (odd, if not ungrammatical) Delete these selected 2 contacts? (getting worse, ungrammatical) I think the deeper linguistic reason is that both of the following are clear/unique references in the given context: - these 2 contacts (those two which are currently selected and on which you just chose "delete" which triggered this message) - the selected contacts Combining two clear/unique references becomes odd because the second reference does not add any relevant information, so it obfuscates more than clarifying because it's now harder to parse two references which are really one. Like saying: "How is your wife, the mother of your children?" - which might make someone think, "So does he have another (previous?) wife which is NOT the mother of his children?". "These 2 contacts" also sounds much more natural, less clumsy, as it avoids the technical term "selected"; and given the context of the preceding workflow where user just chose delete on items he selected before, I think the reference is very clear and unambiguous (even without mentioning the item names on the dialogue).

Hmm, there were two places where the confirmation prompt call could be improved. Apart from this nit, this is same as previous patch, see comment 30. > > > + if (!Services.prompt.confirm(window, > > > + confirmDeleteTitle, > > > + confirmDeleteMessage)) { > > > > Would this fit on a single line? If not, it seems off by 1 space. > > Doesn't fit on one line, but I condensed this into two lines, thanks.

Comment on attachment 8826923 [details] [diff] [review] Patch V.5.2: (another nitfix) Localized, smart and context-sensitive deletion prompts This looks like a nice piece of work to finally get some decent confirmation messages going. I've come a little late to the party, so excuse any silly questions. If I understand it correctly, different strings will be used for deletion of a single item as opposed to deleting multiple items, for example: confirmDeleteThisContact vs. confirmDelete2orMoreContacts. That's done because the message are slightly different, for example the single delete message nominates the contact that will be deleted whereas the multiple delete message doesn't. I thought the idea of plural forms was that one string has two parts, for singular and plural, which can have different formats, and not one for singular and anther one for plural only, whose "singular part" is never used. That then leads to problems like: confirmDelete2orMoreContacts= Are you sure you want to delete this #1 contact?; Are you sure you want to delete these #1 contacts? The translator will translate both, but the first part is never used. Why not put "unused"? Glancing over the previous comment, you originally had "noSingular", which I find a whole lot better than an unused string. However ... What stops you doing: confirmDeleteContact= Are you sure you want to delete this contact?\n\n#1; Are you sure you want to delete these #1 contacts? You can use different inserts if you're worried about #1 having different meanings. Looking at the strings, you have these new ones: 1) confirmDeleteThisContact, confirmDelete2orMoreContacts 2) confirmRemoveThisContact, confirmRemove2orMoreContacts 3) confirmDeleteThisMailingList, confirmDelete2orMoreMailingLists 4) confirmDelete2orMoreContactsAndLists I'd combine the two items in 1) to 3) into one item to avoid unused singular forms. I think Aceman asked to make the unused singular part singular, but that hasn't happened here: confirmDelete2orMoreContactsAndLists= Are you sure you want to delete these #1 contacts and mailing lists?; Are you sure you want to delete these #1 contacts and mailing lists? So, combine items 1) to 3) into one string. If there are really pressing reasons why you can't do/don't want to do that, I'm sure you will point me to the comment where that's been discussed. In this case, unused strings should be marked unused. Clearing the review for now.

(In reply to Jorg K (GMT+1) from comment #32) > Comment on attachment 8826923 [details] [diff] [review] > Patch V.5.2: (another nitfix) Localized, smart and context-sensitive > deletion prompts > > This looks like a nice piece of work to finally get some decent confirmation > messages going. Thank you! :) > I've come a little late to the party, so excuse any silly questions. There are no silly questions as long as they are serious... But this might look like an interesting time-saving strategy of just cross-reading or not reading previous comments and then inquire from others what is going on... at the risk of redundancies in comments ;) > If I > understand it correctly, different strings will be used for deletion of a > single item as opposed to deleting multiple items, for example: > confirmDeleteThisContact vs. confirmDelete2orMoreContacts. > That's done because the message are slightly different, for example the > single delete message nominates the contact that will be deleted whereas the > multiple delete message doesn't. Yes, that's part of the reason, I think... > I thought the idea of plural forms was that one string has two parts, for > singular and plural, which can have different formats, and not one for > singular and anther one for plural only, whose "singular part" is never used. That's what I thought, too, until Aceman the pluralform guru and a deep look into documentation taught me otherwise... See middle of Aceman's comment 15; the fastest way to get to the exact spot here on this bug is Ctrl+F, search for "Icelandic". Pluralforms Documentation (#Icelandic): https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Plural_rule_.2315_.283_forms.29 > That then leads to problems like: > confirmDelete2orMoreContacts= > Are you sure you want to delete this #1 contact?; > Are you sure you want to delete these #1 contacts? > The translator will translate both, but the first part is never used. Why > not put "unused"? Glancing over the previous comment, you originally had > "noSingular", which I find a whole lot better than an unused string. However > ... That's what I originally thought, but Aceman thought otherwise and his reasons were reasonable. :) We've tried it all, Jörg, and the current version is what we ultimately came out with... Again, comment 15 holds all the answers, search for "#nosingular", continue pressing Enter to see all of Aceman's comments on this issue. It's hard to believe and understand, but indeed some languages don't have dedicated forms for singular only (num=1) as English does. Like Icelandic, where all numbers ending in one (except 11, mind you!) have the same form, so in a way they are using the singular form for things which are plural in the English concept...: "1, 21, 31, 41, 51, 61, 71, 81, 91, 101, ..." It seems to follow from that that even talking of a singular form will be an alien concept for such languages, because there's no special form for num=1 only. > What stops you doing: Aceman's comment 15 :p > confirmDeleteContact= > Are you sure you want to delete this contact?\n\n#1; > Are you sure you want to delete these #1 contacts? > You can use different inserts if you're worried about #1 having different > meanings. No, the worry is Icelandic: They need to fill their first slot including the number of items, because it applies to 1, 21, 31, 41, etc., most of which we consider plural. So offering them an English model where the first slot does not even have that number-of-items placeholder can certainly lead to confusion. Also, Aceman reported that the forms might actually be different again when there's no explicit number. So having the #1 placeholder for number-of-items or not does seem to make a big difference. Ultimately, we decided that the best example we can give is just to fill our slots with the correct forms which normally go into our slots (slot1: num=1/singular, slot2: num>1/plural), so that other languages will also be inspired to fill all of their own slots correctly according to their distribution patterns: Icelandic: slot1: num=1,21,31,41 etc.(singular/certain plural, anything ending in 1, except 11), slot 2: num= 0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, (anything else, nothing/certain plural) > Looking at the strings, you have these new ones: > 1) confirmDeleteThisContact, confirmDelete2orMoreContacts > 2) confirmRemoveThisContact, confirmRemove2orMoreContacts > 3) confirmDeleteThisMailingList, confirmDelete2orMoreMailingLists > 4) confirmDelete2orMoreContactsAndLists > I'd combine the two items in 1) to 3) into one item to avoid unused singular > forms. > > I think Aceman asked to make the unused singular part singular, but that > hasn't happened here: > confirmDelete2orMoreContactsAndLists= > Are you sure you want to delete these #1 contacts and mailing lists?; > Are you sure you want to delete these #1 contacts and mailing lists? I explained that in my comment 28, search for: "conscience". This is the EN singular slot for a form which is always inherently plural and can never be singular in the English concept of things (1 contact and 1 mailing list is the smallest possible combination here, that makes 2 items), so we can't fill this differently without breaking our own grammar completely. Broken grammar isn't a good role model. > So, combine items 1) to 3) into one string. If there are really pressing > reasons why you can't do/don't want to do that, I'm sure you will point me > to the comment where that's been discussed. In this case, unused strings > should be marked unused. "unused" is arguably a bad role model for other languages who need to fill all of their slots, so an empty slot can confuse them to do the same in their language, which can come out very wrong for Icelandic & Co. > Clearing the review for now. Btw, Aceman already cleared the strings part with his r+, and only asked you to review the code part ;) You are welcome to read the entire discussion and development above, but otherwise let's avoid going in circles so that we can get done with this! Yes, Pluralforms are mind-warping when it comes to radically different language concepts... My personal favorite is Plural rule #16 (Celtic/Breton) with as many as 6 different forms for very wild conglomerates of numbers with unpredictable exceptions... https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Plural_rule_.2316_.283_forms.29 It's another good example that even where we think that we know it all and our view of things must be "right" and we can even give reasons to justify our view, someone else can look at the same the thing from a totally different perspective, which actually makes the same thing look radically different to the extent of *being* radically different. Think about it!

Interesting, thanks for the summary. So let me say it in my own words: We use confirmDeleteThisContact if we know it's only one. Not even Icelandic or Celtic/Breton will change that fact. The confirmDelete2orMoreContacts is for deleting more than one contact, the singular form will not be used in English, but it might be used by other languages for certain numbers. Celtic/Breton translators will know to add four more forms to the two English supplied as a standard. Thomas' conscience tells him that the first form for confirmDelete2orMoreContactsAndLists= Are you sure you want to delete these #1 contacts and mailing lists? should remain plural. Well, One could put Are you sure you want to delete this #1 contact or mailing list? to make it a singular like the others. No idea how Icelandic translators would react to that. I feel educated now, and yes indeed, asking instead of reading has worked ;-) Now I can proceed to what I was asked to do. No need to set r? again.

Comment on attachment 8826923 [details] [diff] [review] Patch V.5.2: (another nitfix) Localized, smart and context-sensitive deletion prompts Review of attachment 8826923 [details] [diff] [review]: ----------------------------------------------------------------- The code looks OK, so I tested all the cases: 1) confirmDeleteThisContact, confirmDelete2orMoreContacts 2) confirmRemoveThisContact, confirmRemove2orMoreContacts: 3) confirmDeleteThisMailingList, confirmDelete2orMoreMailingLists All work, but are you sure you want the empty line for deleting one contact, ML? I don't like it. 4) confirmDelete2orMoreContactsAndLists Works. 5) confirmDeleteThisAddressbook Works, but I don't like the empty line. 6) confirmDeleteThisLDAPDir Works, but I don't like the empty line, oops, repeating myself. 7) confirmDeleteThisCollectionAddressbook Works, but ... won't say it again. Who said that the empty line is good? Do we have any UI that looks like this? Now, can I be really terrible? Do we need all this complication? What was wrong with: ... delete the selected contact; ... delete the selected contacts. Which one(s) the user is deleting is right there in front of them. There is no need to read the address out to them again. Sure, I'm in favour of asking "... remove from mailing list" instead of "deleting". So my tack would be: Make 1) to 3) simple, no need to read out the name on the contact/ML/AB they're deleting. 4) is fine and 5) to 7) are fine, too, but without reading out the thing that the user just selected. You really want to be like Windows and include the name in the prompt where possible? Otherwise the patch is fine with the nit below fixed, but I think you're overcomplicating the UI. ::: mail/components/addrbook/content/abCommon.js @@ +435,2 @@ > > + if (numSelectedItems > 1) { This should go into the 'else' part below.

(In reply to Jorg K (GMT+1) from comment #35) > Who said that the empty line is good? Do we have any UI that looks like this? I asked the same in end of comment 15. I think this schema is new to TB. But it got review from Paenglab in some older version of the patch. Maybe we could at least remove the empty line and leave the item name on the next line after the message (maybe in parentheses). > Now, can I be really terrible? Do we need all this complication? > What was wrong with: ... delete the selected contact; ... delete the > selected contacts. > Which one(s) the user is deleting is right there in front of them. There is > no need to read the address out to them again. Sure, I'm in favour of asking > "... remove from mailing list" instead of "deleting". In some other messages we do spell out the item being removed. This is not new and may be useful.

OK, I'll try a few variations of the layout and add some screenshots. I guess I can be convinced to spell out the item being removed ;-)

So which one do you like best? I like the second one.

1) The UI of the current patch has ui-r+ by Richard. 2) Spelling out details about item(s) to be deleted obviously helps ux-error-prevention. 3) The reasons for having the name on a separate line (vs. embedded into the question) have already been discussed in comment 20. By analogy (perhaps slightly less binding), the same reasons apply for keeping an empty line between the question and the item name. I wish we could shrink that empty line to half an empty line. From Jörgs mockup, 1) and maybe 2) are acceptable, 3) isn't, for reasons mentioned in comment 20. I must admit 2) looks quite nice and compact, but I'm not yet convinced that it's enough in terms of item name salience. I'll also play with this a little.

1) With all due respect to Richard, I think he did a quick review in comment #13. 2) OK. 3) Version 2 is my favourite and has a new line.

Accepting that our formatting options are limited due to the type of prompt we're using here, I must admit that variant 1 (with blank line) looks a bit disconnected, although I still like the added salience of the item name. Half a blank line would be perfect... So Jörg's variant 2 looks better in comparison to variant 1 and 3. However, it's now very condensed, with very low salience of item name: In spite of the new line, item name still looks like part of the question, and doesn't stand out much at all, whilst being the most important part of the information. Here's variant 2b, which tweak's Jörg's favorite just a little bit to make the item name stand out more, for added salience, by adding a bullet in front of it. - Keep the nicely condensed layout proposed by Jörg - Visually emphasize the item name, without adding clutter - This also helps to easily recognize long item names as a separate entity, as opposed to part of the question. - Bullets are a typical way of listing items - Bullet may be considered as hint of an icon I think this is a nice tweak, combining the best of both worlds (condensed format + salient item name). What do you think?

Note that the really important information of the prompt is the item name (in the context of the title): Delete Contact (?) * John Doe The question just elaborates the same information, but does not add any information, so it's irrelevant in the long run. Even the title is less important because well, you just chose Delete, hopefully intentionally. On the other hand, the item name remains important, even for experienced users, to double-check if you're deleting the right thing. So the bullet literally forces and guides your eyes to focus on the main point! :)

I like version 2 and 2b.

Yes, 2b is OK, but how do you generate the bullet? It's not a "*", is it? Some special character? (That would be the end of landing the patch with author Düllmann on Windows, since in order to do this, I need to encode the patch as ANSI, and the special character would get lost.)

Comment on attachment 8826977 [details] A few variations on the confirmation panel Best would be 1.5 ;) which would be the same space above "Thomas Düllmann" as we have below it. Because this isn't doable without hacks, I vote for 2.

Comment on attachment 8827054 [details] Screenshot 2 (variant 2b): Condensed confirmation prompt with bulleted item name [as implemented] This would be a good solution to set out the contact name together with the condensed view.

So based on broad consensus, this patch is ready to bite the bullet! Implements variant 2b as seen in attachment 8827054 [details]. (In reply to Jorg K (GMT+1) from comment #35) > Comment on attachment 8826923 [details] [diff] [review] > Patch V.5.2: (another nitfix) Localized, smart and context-sensitive > deletion prompts > [...] > ::: mail/components/addrbook/content/abCommon.js > @@ +435,2 @@ > > > > + if (numSelectedItems > 1) { > > This should go into the 'else' part below. Done, thanks.

Comment on attachment 8827277 [details] [diff] [review] Patch V.6: (bite the bullet) Localized, smart and context-sensitive deletion prompts Thanks, let's put the good news first: r+ ;-) Now let me be a terrible nit picker. Is there any reason why there are two spaces after the bullet?

I tried it now, I think one space looks better.

(In reply to Jorg K (GMT+1) from comment #44) > (That would be the end of landing the patch with author Düllmann on Windows, So surely we could find someone to push it from Linux. The UTF-8 bullet is probably needed.

I converted the patch to ANSI and pushed to try and it's certainly no good: https://hg.mozilla.org/try-comm-central/rev/f4a450d5d56f8e1814b0faa7c6e922b89bb7fd87#l2.45 So repeating the rules: To get ü in the author ... ... on Windows the patch needs to be all ANSI and the content must be pure 7bit ASCII. ... on Linux/Mac the patch needs to be all UTF-8.

Comment on attachment 8827277 [details] [diff] [review] Patch V.6: (bite the bullet) Localized, smart and context-sensitive deletion prompts Review of attachment 8827277 [details] [diff] [review]: ----------------------------------------------------------------- Seems good now, thanks. I'll let Jorg decide on the double/dingle spaces after the bullet. ::: mail/components/addrbook/content/abCommon.js @@ +292,5 @@ > + brandShortName = document.getElementById("bundle_brand").getString("brandShortName"); > + confirmDeleteMessageID = "confirmDeleteThisCollectionAddressbook"; > + confirmDeleteTitleID = "confirmDeleteThisCollectionAddressbookTitle"; > + clearCollectionPrefs = true; > + } else if (directory.URI.includes(kLdapUrlPrefix)) { Sorry, didn't notice previously, should this be directory.URI.startsWith(kLdapUrlPrefix) ? @@ +436,5 @@ > + // Get plural form where applicable; substitute placeholders as required. > + if (numSelectedItems == 1) { > + // If single selected item, substitute itemName. > + confirmDeleteMessage = confirmDeleteMessage.replace("#1", itemName); > + } else { Yeah, you were not fetching both strings needlessly, I got somehow lost in the logic :) But I see you could still fold needless condition checks here (if (numSelectedItems > 1) {} ... if (numSelectedItems == 1) {}), thanks.

(In reply to Jorg K (GMT+1) from comment #51) > I converted the patch to ANSI and pushed to try and it's certainly no good: > https://hg.mozilla.org/try-comm-central/rev/ > f4a450d5d56f8e1814b0faa7c6e922b89bb7fd87#l2.45 > > So repeating the rules: > To get ü in the author ... > ... on Windows the patch needs to be all ANSI and the content must be pure > 7bit ASCII. > ... on Linux/Mac the patch needs to be all UTF-8. Since Patch V.4, I have consistently used "Duellmann" as my author name, so I think there's no need to continue discussing this. I vaguely recall there might have been workarounds to get this right, requiring manual intervention and deliberately accepting 'wrong' spelling at certain stages, but even if it worked, it would still be complex and error-prone, so to make everyone's life easier and avoid nasty misspellings in the tree, I've decided to settle for using "Duellmann" consistently, without ü. Thanks Jörg for investigating this matter. Otherwise yes, for this patch we need UTF-8 format to get the bullet right.

(In reply to :aceman from comment #52) > Review of attachment 8827277 [details] [diff] [review]: > ----------------------------------------------------------------- > Seems good now, thanks. > I'll let Jorg decide on the double/dingle spaces after the bullet. double/dingle... I like the sense and sound of that spelling variant! ;) > Sorry, didn't notice previously, should this be > directory.URI.startsWith(kLdapUrlPrefix) ? Done. > @@ +436,5 @@ > > + // Get plural form where applicable; substitute placeholders as required. > > + if (numSelectedItems == 1) { > > + // If single selected item, substitute itemName. > > + confirmDeleteMessage = confirmDeleteMessage.replace("#1", itemName); > > + } else { > > Yeah, you were not fetching both strings needlessly, I got somehow lost in > the logic :) But I see you could still fold needless condition checks here > (if (numSelectedItems > 1) {} ... if (numSelectedItems == 1) {}), thanks. I don't see anything now which could still easily be folded... 8) I have folded, unfolded, and refolded those conditions many time, but afaics they will just look different, but not become any simpler, also wrt readability. So I understand from Aceman's and Jörg's r+ that this is now good to go. (In reply to Jorg K (GMT+1) from comment #48) > Comment on attachment 8827277 [details] [diff] [review] > Patch V.6: (bite the bullet) Localized, smart and context-sensitive deletion > prompts > > Thanks, let's put the good news first: r+ ;-) I love good news, and good news first, thanks! :) > Now let me be a terrible nit picker. Is there any reason why there are two > spaces after the bullet? You are a terrible nit picker indeed! Perambulatory perfectionist! ;p Historically, I introduced the double spacing when testing "ballot x" symbol before the item name: ✘ John Doe. I discarded "ballot x" because it was too distracting, harder to parse because easy to confuse with character "x". I then kept the double-space deliberately with the bullet because it seemed to look alright. Until Jörg discovered that it looks better with only one space! It's all good... Btw, http://htmlarrows.com has a nicely sorted reference of all those symbols, it's an amazing collection!

Jörg, if this is now ready for check-in, please go ahead! Tia.

Re. the additional "folding": numSelectedItems > 1 does not appear in code any more, so there is no more folding possible. I think we misunderstood Acemen when he said: > Yeah, you were not fetching both strings needlessly, I got somehow > lost in the logic :) But I see you could still fold needless condition > checks here (if (numSelectedItems > 1) {} ... > if (numSelectedItems == 1) {}), thanks. This refers to the version which I complained about in comment #35 and you subsequently fixed. The "thanks" refers to fixing it. I think we're well and truly done here, I'll land this personally. If I were to put "checkin-needed" onto it, the person trying to import the patch using "hg qimport bz:1319493" would get a big fright, since many errors are displayed as somewhere in the queue of superseded patches, there is an ANSI encoded one. So going to all-UTF-8 and "ue" is certainly a good decision. Thanks to all involved and thanks for the patience. It just goes to show that UI bugs are hard to get right, no one discusses when you fix a crash with a one liner ;-)

https://hg.mozilla.org/comm-central/rev/76f1a04a52f1216c39b47e963389cd5e93a6f1dd (I changed the commit message slightly since the author can't (ui)review his own work). Sadly the comments in the addressBook.properties were wrong since we removed those blank lines, so here a follow-up: https://hg.mozilla.org/comm-central/rev/ccf7f9af783387d3729dad1636b69ed7f82db729

Sadly this has causes two test failures. More in the next comment.

Trivial patch to fix test failure, ... that's what I thought, but it comes worse when running this locally: SUMMARY-PASS | test-address-book.js::setupModule SUMMARY-PASS | test-address-book.js::test_order_of_address_books SUMMARY-PASS | test-address-book.js::test_persist_collapsed_and_expanded_states SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\addrbook\test-address-book.js | test-address-book.js::test_deleting_contact_causes_confirm_prompt EXCEPTION: Error in atexit._run_exitfuncs: Traceback (most recent call last): File "c:\mozilla-build\python\Lib\atexit.py", line 24, in _run_exitfuncs func(*targs, **kargs) File "runtest.py", line 498, in prettyPrintResults prettyPrintException(failure['exception']) File "runtest.py", line 449, in prettyPrintException print ' EXCEPTION:', e.get('message', 'no message!') File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\encodings\cp850.py", line 12, in encode return codecs.charmap_encode(input,errors,encoding_map) UnicodeEncodeError: 'charmap' codec can't encode character u'\u2022' in position 55: character maps to <undefined> Error in sys.exitfunc: Traceback (most recent call last): File "c:\mozilla-build\python\Lib\atexit.py", line 24, in _run_exitfuncs func(*targs, **kargs) File "runtest.py", line 498, in prettyPrintResults prettyPrintException(failure['exception']) File "runtest.py", line 449, in prettyPrintException print ' EXCEPTION:', e.get('message', 'no message!') File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\encodings\cp850.py", line 12, in encode return codecs.charmap_encode(input,errors,encoding_map) UnicodeEncodeError: 'charmap' codec can't encode character u'\u2022' in position 55: character maps to <undefined> c:/mozilla-source/comm-central/mozilla/../mail/testsuite-targets.mk:42: recipe for target 'mozmill-one' failed mozmake: *** [mozmill-one] Error 1 So basically the test hicks-up on the bullet \u2022. I could push the patch to change the string names, but I can't easily fix the fact that the testing framework falls apart. Aceman?

This fixes the test and also removes the bullets. We had agreement that the option without the bullets was also fine.

https://hg.mozilla.org/comm-central/rev/bc37a3058e456e840f3da0b4edd847415d6d2c51 Landed this (without the trailing spaces) to fix the test.

Oh boy, I really messed this up. Turns out that to fix the test failure, the bullets could stay. So I backed out the test fix and landed it again: https://hg.mozilla.org/comm-central/rev/6802408ee443a8011e4c63c19517f7800b78fa71 (backout) https://hg.mozilla.org/comm-central/rev/83785a57c3a64710edbe57ecdd7c80b66e5f5168 (fixed tests again) So in summary, we have three changesets here: https://hg.mozilla.org/comm-central/rev/76f1a04a52f1216c39b47e963389cd5e93a6f1dd https://hg.mozilla.org/comm-central/rev/ccf7f9af783387d3729dad1636b69ed7f82db729 (fixed comments) https://hg.mozilla.org/comm-central/rev/83785a57c3a64710edbe57ecdd7c80b66e5f5168 (fixed tests again) I hope I'm done here now.

Yes, I have also noticed in other tests (I think for the identity list in composer) that the test suite has problems outputting utf-8 text. We can't output raw data strings from the running TB if they may contain such characters. I had to use some safe English strings or use a variant of assert_* that does not output the value of the compared arguments.