46050 - Address Book name must be unique in the app (disallow duplicate names)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Suresh]

Summary says it all. We should not allow to AB's of same name.

Comment 1

[lchiang]

Suresh - what happens if the user creates a second address book of the same name
as an existing one?  Does autocomplete still work?  Do address book cards get
lost?  Thanks.

Comment 2

[Suresh]

1. If the user creates a AB of same name, it gets added to the AB pane. It is 
not validated with the names of the existing ones.
2. Yes, Autocomplete works against entries in all AB's.
3. No, AB cards are NOT lost.

Comment 3

[scottputterman]

reassigning to chuang.  I think this might be a dup.

Comment 4

[lchiang]

future per mail triage

Comment 5

[laurel]

*** Bug 61574 has been marked as a duplicate of this bug. ***

Comment 6

[Keyser Sose]

Pretty sure this is a duplicate.

*** This bug has been marked as a duplicate of 45946 ***

Comment 7

[Håkan Waara]

Is this really a duplicate?

bug 45946's name is "Do not allow duplicate entries in AB". Please verify or
reopen this bug.

Comment 8

[Håkan Waara]

Oh well, verifying.

Comment 9

[vbreddy]

I think This bug is not a duplicate for #45946.. 45946 is about the entries 
inside an address book. The current bug talks about the uniqueness of name 
of the address book itself.

Comment 10

[scottputterman]

reassigning to racham

Comment 11

[R.K.Aa.]

*** Bug 121190 has been marked as a duplicate of this bug. ***

Comment 12

[R.K.Aa.]

*** Bug 146341 has been marked as a duplicate of this bug. ***

Comment 13

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 14

[Mark Banner (:standard8)]

added "(disallow duplicate names)" to title to aid searching for duplicates.

Comment 15

[Mark Banner (:standard8)]

Note this also applies to importing address books

Comment 16

[buovjaga]

Still repro.

Thunderbird 52.1.1 (32-bit)
Windows 7 64-bit

Comment 17

[:aceman]

Attachment: patch

This should do it. Checks in local AB renaming and also in the LDAP AB dialog.

Comment 18

[Thomas D. (:thomas8)]

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

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

Thanks for this patch! Sorry for getting to this a bit late.
This works well, but I'd like to polish this a bit.
ui-r=me with these nits fixed.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +191,5 @@
>  contactsMoved=%1$S contact moved;%1$S contacts moved
>  
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +invalidNameDuplicate=Please enter an unique addressbook Name.

This would have to be "a unique" because it's the sound which counts and unique begins with the sound of a consonant (j).

I'd like to match the message text with my proposed error message for normal AB's, to be more informative and more visual:

>

Comment 19

[Thomas D. (:thomas8)]

Hm. That review got destroyed, probably because of the UTF-8 character which I used. Will do again tomorrow, too late now.

Comment 20

[Thomas D. (:thomas8)]

Attachment: Strings.txt: Dynamic error message strings with unicode symbols

Alright, BMO doesn't like advanced unicode characters; the characters themselves and any text which follows get discarded without warning or recovery options, both for reviews and comments (comments might be recoverable using browser's back button, reviews are not). Filed as bug 1387335.

So pls find attached my proposal for more specific, more informative, and more visual error message texts. Displays correctly in unicode-compatible text editors like Notepad++; note that Thunderbird will display the symbols in color.

I'll refer to this proposal in my new review.

Comment 21

[Thomas D. (:thomas8)]

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

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

Unfortunately, my last review got killed by bug 1387335...
So here we go again:

This works well, but I'd like to polish this a bit.
ui-r=me with these nits fixed.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +191,5 @@
>  contactsMoved=%1$S contact moved;%1$S contacts moved
>  
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +invalidNameDuplicate=Please enter an unique addressbook Name.

You intentionally matched the style of similar error messages of this dialogue. I'd prefer to match my proposed message text for normal AB's, to be more specific, more informative and more visual:

(*) represents unicode character. See attachment 8893692 [details] for correct display.

(*) My LDAP Directory
An LDAP directory with this name already exists. Please enter a different name.

duplicateNameText=(*) %1$S\nAn LDAP directory with this name already exists. Please enter a different name.

Pls add an extensive localization comment, what not to change, how placeholder will be replaced, no space after \n etc.

@@ +235,5 @@
>  addressBookTitleNew=New Address Book
>  ## LOCALIZATION NOTE (addressBookTitleEdit): %S will be replaced by the the Address Book's name
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +duplicateNameText=An address book with the given name already exists. Please enter a different name.

Dito, see attachment 8893692 [details]:

(*) My Custom AB
An address book with this name already exists. Please enter a different name.

duplicateNameText=(*) %1$S\nAn address book with this name already exists. Please enter a different name.

Add localization comment, see above.

::: mailnews/addrbook/content/abAddressBookNameDialog.js
@@ +60,5 @@
>  
> +  // Do not allow an already existing name.
> +  for (let ab of fixIterator(MailServices.ab.directories,
> +                             Components.interfaces.nsIAbDirectory)) {
> +    if ((ab.dirName == newName) && (!gDirectory || (ab.URI != gDirectory.URI))) {

Pls make this case-insensitive.
It's confusing and error-prone to have different ABs whose name only differs in capitalization.
Ensure that renaming an AB to its own current name, but with different capitalization still works.

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +288,5 @@
>    try {
>      var pref_string_content = "";
>      var pref_string_title = "";
>  
> +    var description = document.getElementById("description").value.trim();

Would this still work if you use let? (Maybe not...)
If yes, pls change this whole set of variables to let.

@@ +300,5 @@
> +    let isDupeName = function(newName) {
> +      // Do not allow an already existing name.
> +      for (let ab of fixIterator(MailServices.ab.directories,
> +                                 Components.interfaces.nsIAbDirectory)) {
> +        if ((ab.dirName == newName) &&

case-insensitive matching pls, see above

@@ +383,5 @@
>    return true;
>  }
>  
>  function onCancel()
>  {  

While we are here, pls remove this existing trailing whitespace

::: suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties
@@ +120,5 @@
>  contactsMoved=%1$S contact moved;%1$S contacts moved
>  
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +invalidNameDuplicate=Please enter an unique addressbook Name.

See above / see attachment 8893692 [details] for new strings.

@@ +163,5 @@
>  addressBookTitleNew=New Address Book
>  ## LOCALIZATION NOTE (addressBookTitleEdit): %S will be replaced by the the Address Book's name
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +duplicateNameText=An address book with the given name already exists. Please enter a different name.

See above / see attachment 8893692 [details] for new strings.

Comment 22

[:aceman]

I don't think we should add unicode chars for random icons into the error messages. We never did that. And the characters may not even exist in most fonts.

I can work on the rest of the comment, thanks.

Comment 23

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #22)
> I don't think we should add unicode chars for random icons into the error
> messages. We never did that. And the characters may not even exist in most
> fonts.

They are not "random" icons, they are carefully selected to match the icons for normal ABs (book) and LDAP ABs (earth). On my system, they match very well. Slight variations may occur with other fonts.
"We never did that", if taken as an absolute, is a bad reason because it will kill any innovation.
If there was a way we could have the original icons in the error message, imho that's an excellent way of visualizing what the problem is, so it helps to understand the error message. Pls look at Windows explorer confirmation messages for deleting files (Shift+Del, otherwise they just go to dustbin silently): They come with *two* icons, including a very big version of the file's own icon, to visualize which object you're dealing with. So that's a modern approach to error/confirmation dialogues, plaintext error messages are no longer the only game in town.
But of course we must be sure that those icons display correctly on every system which we support.
Are we not shipping with our own fonts?

> I can work on the rest of the comment, thanks.

If we decide against the unicode icons, just replace the icons with bullets (•), as we did in the delete confirmation messages. I think showing the name of the offending object in the error message helps to understand what this is about, and I think the text and structure of the error messages which I suggested in attachment 8893692 [details] is quite clear and consistent. Note that the titles of the error messages are missing in the attachment, I'm assuming Aceman's message titles (Duplicate Address Book Name, and in LDAP it's "[LDAP name] Properties" by current design).

Comment 24

[Richard Marti (:Paenglab)]

I'm against the icons. Let's do it with the bullets only. When I looked at attachment 8893692 [details] first I saw a cell phone and not a book. Only when the font was bigger I saw the rounding of the spine of the book. The unicode icons depend very from the implementation. Maybe you know also about some problems of the differences of the different smileys over the platforms where the appearance is not always the same and can upset the receiver?

Comment 25

[:aceman]

Attachment: patch v2

Updated with review comments.

Comment 26

[Richard Marti (:Paenglab)]

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

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

Looks good with the comments addressed also in suite part.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +193,5 @@
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +## LOCALIZATION NOTE (invalidNameDuplicate):
> +## %S is the name of the new LDAP directory
> +invalidNameDuplicate=•%S\nAn LDAP directory with this name already exists. Please enter a different name.

Please add a space between the bullet and the name.

@@ +239,5 @@
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +## LOCALIZATION NOTE (duplicateNameText):
> +## %S is the name of the new LDAP directory
> +duplicateNameText=•%S\nAn address book with the given name already exists. Please enter a different name.

Comment refers to a LDAP directory. Please change to "address book".
Again space after bullet. And instead of "the given" use "this" like it's for the LDAP message.

Comment 27

[Thomas D. (:thomas8)]

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

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

Lgtm with some nits fixed, thanks.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +193,5 @@
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +## LOCALIZATION NOTE (invalidNameDuplicate):
> +## %S is the name of the new LDAP directory
> +invalidNameDuplicate=•%S\nAn LDAP directory with this name already exists. Please enter a different name.

We should use the placeholder %1$S as seen in my template, to avoid false positives when replacing the placeholder. For some reason known only to the user, %S might be part of an AB name like "%SysAdmins AB", but the probability of having %1$S in the filename is certainly near zero.

@@ +239,5 @@
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +## LOCALIZATION NOTE (duplicateNameText):
> +## %S is the name of the new LDAP directory
> +duplicateNameText=•%S\nAn address book with the given name already exists. Please enter a different name.

dito wrt placeholder

::: mailnews/addrbook/content/abAddressBookNameDialog.js
@@ +61,5 @@
> +  // Do not allow an already existing name.
> +  for (let ab of fixIterator(MailServices.ab.directories,
> +                             Components.interfaces.nsIAbDirectory)) {
> +    if ((ab.dirName.toLowerCase() == newName.toLowerCase()) &&
> +        (!gDirectory || (ab.URI != gDirectory.URI))) {

Any chance that those URIs might have different cases even though they are the same?

@@ +65,5 @@
> +        (!gDirectory || (ab.URI != gDirectory.URI))) {
> +      const kAlertTitle = document.getElementById("bundle_addressBook")
> +                                  .getString("duplicateNameTitle");
> +      const kAlertText = document.getElementById("bundle_addressBook")
> +                                 .getFormattedString("duplicateNameText", [newName]);

Using the correct placeholder as recommended above might mean that you have to just get the plain string (with unresolved placeholder) here and do the replacement of the placeholder yourself using string.replace(). But we've done this elsewhere, so there's nothing special or new.

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +377,4 @@
>  
> +      let errorText;
> +      if (errorArg)
> +        errorText = addressBookBundle.getFormattedString(errorValue, [errorArg])

dito, might need manual string.replace() here for the better placeholder.

::: mailnews/addrbook/public/nsIAbManager.idl
@@ +147,5 @@
>     */
>    readonly attribute nsIFile userProfileDirectory;
>  
>    /**
> +   * Finds out if the mailing list name exists in any address book.

So this function no longer only applies to *mork/MDB* ABs?

::: suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties
@@ +114,5 @@
>  # LDAP directory stuff
>  invalidName=Please enter a valid Name.
> +## LOCALIZATION NOTE (invalidNameDuplicate):
> +## %S is the name of the new LDAP directory
> +invalidNameDuplicate=•%S\nAn LDAP directory with this name already exists. Please enter a different name.

Space after bullet, and better placeholder (see above).

@@ +158,5 @@
>  ## LOCALIZATION NOTE (addressBookTitleEdit): %S will be replaced by the the Address Book's name
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +## LOCALIZATION NOTE (duplicateNameText):
> +## %S is the name of the new LDAP directory

Address Book, not LDAP dir

@@ +159,5 @@
>  addressBookTitleEdit=%S Properties
> +duplicateNameTitle=Duplicate Address Book Name
> +## LOCALIZATION NOTE (duplicateNameText):
> +## %S is the name of the new LDAP directory
> +duplicateNameText=•%S\nAn address book with the given name already exists. Please enter a different name.

Space after bullet, "the given" -> this, placeholder as above.

Comment 28

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #27)
> Comment on attachment 8898058 [details] [diff] [review]
> ::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
> @@ +193,5 @@
> >  # LDAP directory stuff
> >  invalidName=Please enter a valid Name.
> > +## LOCALIZATION NOTE (invalidNameDuplicate):
> > +## %S is the name of the new LDAP directory
> > +invalidNameDuplicate=•%S\nAn LDAP directory with this name already exists. Please enter a different name.
> 
> We should use the placeholder %1$S as seen in my template, to avoid false
> positives when replacing the placeholder. For some reason known only to the
> user, %S might be part of an AB name like "%SysAdmins AB", but the
> probability of having %1$S in the filename is certainly near zero.

Please scrap my comments about the placeholder, %S works just fine.
We're replacing the placeholder only within the error message, and we let it be replaced *with* AB name, so there's no risk of false positives because the translation of the error message text can't contain %S except as a placeholder.
I guess it helps to avoid false positives when there are *multiple* placeholders which can only be done sequentially, and any freeform text (object name) which has already been substituted for #1 might contain "#2", then in the next round where we replace #2 we can get it wrong. Does that sound right? Always learning...

One more nit:

> +## LOCALIZATION NOTE (duplicateNameText):
> +## %S is the name of the new LDAP directory
> +duplicateNameText=•%S\nAn address book with the given name already exists. Please enter a different name.

We can't automatically expect localizers to know \n, and it looks a bit confusing, so we should tell them to keep it, and show them how it looks by offering an example (which is good practice for any strings with substitution, I think), just like we did in bug 1319493:

> # LOCALIZATION NOTE (confirmDeleteThisAddressbookTitle):
> # #1 The name of the selected address book
> # Don't localize "\n• #1" unless your local layout comes out wrong.
> # Example: Are you sure you want to delete this address book and all of its contacts?
> #          • Friends and Family Address Book
> confirmDeleteThisAddressbook=Are you sure you want to delete this address book and all of its contacts?\n• #1

Comment 29

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #28)
> > We should use the placeholder %1$S as seen in my template, to avoid false
> > positives when replacing the placeholder. For some reason known only to the
> > user, %S might be part of an AB name like "%SysAdmins AB", but the
> > probability of having %1$S in the filename is certainly near zero.
> 
> Please scrap my comments about the placeholder, %S works just fine.
> We're replacing the placeholder only within the error message, and we let it
> be replaced *with* AB name, so there's no risk of false positives because
> the translation of the error message text can't contain %S except as a
> placeholder.
> I guess it helps to avoid false positives when there are *multiple*
> placeholders which can only be done sequentially, and any freeform text
> (object name) which has already been substituted for #1 might contain "#2",
> then in the next round where we replace #2 we can get it wrong. Does that
> sound right? Always learning...

Yes, good that I do not need to explain this:) That is why we need to use the special .getFormattedString("string", [array of arguments]) functions that should work atomically and not string.replace("#1", "x").replace("#2", "y") which would have the problem you describe.

Comment 30

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #27)
> ::: mailnews/addrbook/content/abAddressBookNameDialog.js
> @@ +61,5 @@
> > +  // Do not allow an already existing name.
> > +  for (let ab of fixIterator(MailServices.ab.directories,
> > +                             Components.interfaces.nsIAbDirectory)) {
> > +    if ((ab.dirName.toLowerCase() == newName.toLowerCase()) &&
> > +        (!gDirectory || (ab.URI != gDirectory.URI))) {
> 
> Any chance that those URIs might have different cases even though they are
> the same?

I think we rely on URIs to be "normalized" everywhere so hopefully they can be compared without lowercasing as is done everywhere.
 
> ::: mailnews/addrbook/public/nsIAbManager.idl
> @@ +147,5 @@
> >     */
> >    readonly attribute nsIFile userProfileDirectory;
> >  
> >    /**
> > +   * Finds out if the mailing list name exists in any address book.
> 
> So this function no longer only applies to *mork/MDB* ABs?

Yes, it is universal now, but the change in the code was done in bug 1356881 :)

Comment 31

[:aceman]

Attachment: patch v2.1

Fixed the strings.

Comment 32

[Jorg K (CEST = GMT+2)]

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

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

Very nice for an ancient bug (however, I still hold the record with bug 26734).

A few questions below. I don't like the UI. Those bullets were introduced for deletion from the address book and they signify a list that follows, not precedes, a title. And surely the consequence of "you already got this" is to enter a different one. I think we can lose the second part. Or do:

LDAP directory with this name already exists:
• Secret list
Please enter a different name.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +194,5 @@
>  invalidName=Please enter a valid Name.
> +## LOCALIZATION NOTE (invalidNameDuplicate):
> +## Don't localize "• %S\n" unless your local layout comes out wrong
> +## %S is the name of the new LDAP directory
> +invalidNameDuplicate=• %S\nAn LDAP directory with this name already exists. Please enter a different name.

Well, that pretty inconsistent with the delete prompt:
Are you sure you want to delete this contact?
• Doe, John

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +307,5 @@
> +          return true;
> +        }
> +      }
> +      return false;
> +    }

Semicolon?

@@ +309,5 @@
> +      }
> +      return false;
> +    }
> +
> +    if (!description)

What happened to || (description.trim() == "") ?

::: suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties
@@ +115,5 @@
>  invalidName=Please enter a valid Name.
> +## LOCALIZATION NOTE (invalidNameDuplicate):
> +## Don't localize "• %S\n" unless your local layout comes out wrong
> +## %S is the name of the new LDAP directory
> +invalidNameDuplicate=• %S\nAn LDAP directory with this name already exists. Please enter a different name.

Same comment as above.

Comment 33

[Jorg K (CEST = GMT+2)]

Thomas, can we do:

LDAP directory with this name already exists:
• Secret list

Like we already have:

Are you sure you want to delete this contact?
• Doe, John

Comment 34

[:aceman]

I would like that.
Also, should it display the new name or the name of the existing object (they can differ in case)?

Comment 35

[Richard Marti (:Paenglab)]

I'm for it too. And the new name should be shown.

Comment 36

[Jorg K (CEST = GMT+2)]

Well, the existing one, since that's what exists, no? And therefore matches the message.

Comment 37

[Richard Marti (:Paenglab)]

Ah yes, not well enough read the new message.

Comment 38

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #33)
> Thomas, can we do:
> LDAP directory with this name already exists:
> • Secret list

Yes, that looks better.

Pls keep the article:
/An/ LDAP directory with this name already exists.

After our error msg, could we helpfully select the name field for LDAP dirs?
For normal AB's, I guess just the cursor is OK.

Comment 39

[Thomas D. (:thomas8)]

(In reply to Mark Banner (:standard8) (afk 11 - 20 Aug) from comment #15)
> Note this also applies to importing address books

Are we going to address this part, too? (Maybe in a new bug)

Comment 40

[Jorg K (CEST = GMT+2)]

Aceman, now that we all agree about the message, can you please answer the questions from comment #32
  Semicolon?
  What happened to || (description.trim() == "") ?
so we can get this landed.

Comment 41

[Thomas D. (:thomas8)]

As for showing the existing vs. new name, both would work semantically, but I'd agree that showing the existing name which you overlooked makes more sense. Unfortunately, that's also more work for Aceman ;) Probably store the old name in a variable during the iteration when we catch the duplicate.

(In reply to Jorg K (GMT+2) from comment #40)
>   What happened to || (description.trim() == "") ?

He added trim here:
> let description = document.getElementById("description").value.trim();
So (!description) will catch that case, too.

> so we can get this landed.
Well, still needs a bit of tweaking to show the existing name, and possibly improve focus after the error msg for LDAP.

Comment 42

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #39)
> (In reply to Mark Banner (:standard8) (afk 11 - 20 Aug) from comment #15)
> > Note this also applies to importing address books
> 
> Are we going to address this part, too?

Please file a new bug as that is less interactive, imports a batch of ABs and needs more thought on what to do if one is a duplicate name.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #38)
> After our error msg, could we helpfully select the name field for LDAP dirs?

This can be done but make it a new bug where we can do this for any field that has an error.

Comment 43

[:aceman]

Attachment: patch v2.2

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8899580 [details] [diff] [review]
patch v2.2

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

I lightly tested that. When renaming an address book to the name of an existing LDAP directory, we get "An address book with this name already exists:". That's not quite accurate. When trying to create and LDAP directory with the same name as an AB, you get, in short, LDAP already exists. Do we care? It's a little confusing. Can you pass back the type of the directory to give a better message?

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +307,5 @@
> +          return ab.dirName;
> +        }
> +      }
> +      return null;
> +    }

No semicolon here? let xxx = thing; - no?

@@ +314,2 @@
>        errorValue = "invalidName";
> +    else if ((errorArg = findDupeName(description))) {

Could that be
if (errorArg = findDupeName(description))
?

Comment 45

[Richard Marti (:Paenglab)]

Comment on attachment 8899580 [details] [diff] [review]
patch v2.2

This looks good.

If it's possible to show a dialog depending of the type existing of the existing AB/LDAP, then this should be done. Good catch, Jörg.

Comment 46

[:aceman]

(In reply to Jorg K (GMT+2) from comment #44)
> I lightly tested that. When renaming an address book to the name of an
> existing LDAP directory, we get "An address book with this name already
> exists:". That's not quite accurate. When trying to create and LDAP
> directory with the same name as an AB, you get, in short, LDAP already
> exists. Do we care? It's a little confusing. Can you pass back the type of
> the directory to give a better message?

We probably can get the type but I wouldn't complicate this. Is LDAP not an addressbook? Next time it is some other object (Outlook AB, addon provided addressbook) and we won't have a string for that. Or do you want to have a special string for LDAP and another one for everything else?
 
> ::: mailnews/addrbook/prefs/content/pref-directory-add.js
> @@ +307,5 @@
> > +          return ab.dirName;
> > +        }
> > +      }
> > +      return null;
> > +    }
> 
> No semicolon here? let xxx = thing; - no?

I know you asked this in the previous version, but what do you mean? A semicolor after the closing } of the function? I never write that, but if you like it I can add it.

> > +    else if ((errorArg = findDupeName(description))) {
> 
> Could that be
> if (errorArg = findDupeName(description))
> ?

The (()) is a special hack to suppress the JS warning "SyntaxError: test for equality (==) mistyped as assignment (=)?" which could be produced here.

Comment 47

[Jorg K (CEST = GMT+2)]

I don't want to be a pain, but we have two messages. When renaming and AB/LDAP to one that exists already, you get two different messages, but they are based on the type of the thing you're renaming, not the type of the thing that already exists.

If there were only one message, I'd agree, but what you've implemented is confusing, well, I'd say the message is wrong.

As for the let xxx = function () { ... }; I don't appear the only one who wants that:
https://dxr.mozilla.org/comm-central/rev/7b75c2e0b9deb590c18aaddb2a6168146cde8ca4/calendar/base/backend/icaljs/calICSService.js#482

I know that you can write JS leaving off semicolons left, right and centre, but let's not start this.

Comment 48

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #47)
> If there were only one message, I'd agree, but what you've implemented is
> confusing, well, I'd say the message is wrong.

Jörg is right; I'm sorry for introducing this confusion. Good catch.
In that case, I'd say that one message may be enough. It doesn't really matter which type the existing thing is, it's about the duplicate name only. So I think plain and simple is best:

> An address book with this name already exists.
> o Existing AB

Even though we're also using address book in a specific sense, I think it's generic enough to cover both cases, and possibly future cases. Another generic term might be "directory", but it doesn't add much, in fact it appears to obfuscate, and we're using it for "startup directory" which might include mailing lists, which are not included here.

Comment 49

[Thomas D. (:thomas8)]

Attachment: (Empty Patch)

Let's get this out of the way...

Per my comment 48, and aceman's comment 46, I've unified the error messages.
By analogy, for file manager, Error message "Text file/Word document/Image something.xyz already exists" would be possible, but useless information; per ux-minimalism, the expected msg is "A file with this name already exists".

(In reply to :aceman from comment #46)
> We probably can get the type but I wouldn't complicate this. Is LDAP not an
> addressbook? Next time it is some other object (Outlook AB, addon provided
> addressbook) and we won't have a string for that. 

+1

Nevertheless, I deliberately kept two identical strings because I think it's wise to keep the LDAP strings complete and together, because who knows, someone might want to change that for some reason. In the same way, we also keep context menu strings separate even when they are identical with main menu strings.

I also nitfixed the comments to match the new logic of showing the *existing* AB name, plus adding examples, plus replacing ## with # because ## is really odd and the larger part of the file just uses single # for regular comments. As in function comments, it makes more sense to emphasize first/block comment headlines with ##, not the entire comment.

Comment 50

[Thomas D. (:thomas8)]

Attachment: Patch V.3: (uniform error message, nitfix string comments) Prevent duplicate AB names

Sorry, something went wrong with the last patch.

Let's get this out of the way...

Per my comment 48, and aceman's comment 46, I've unified the error messages.
By analogy, for file manager, Error message "Text file/Word document/Image something.xyz already exists" would be possible, but useless information; per ux-minimalism, the expected msg is "A file with this name already exists".

(In reply to :aceman from comment #46)
> We probably can get the type but I wouldn't complicate this. Is LDAP not an
> addressbook? Next time it is some other object (Outlook AB, addon provided
> addressbook) and we won't have a string for that. 

+1

Nevertheless, I deliberately kept two identical strings because I think it's wise to keep the LDAP strings complete and together, because who knows, someone might want to change that for some reason. In the same way, we also keep context menu strings separate even when they are identical with main menu strings.

I also nitfixed the comments to match the new logic of showing the *existing* AB name, plus adding examples, plus replacing ## with # because ## is really odd and the larger part of the file just uses single # for regular comments. As in function comments, it makes more sense to emphasize first/block comment headlines with ##, not the entire comment.

Comment 51

[Thomas D. (:thomas8)]

Attachment: Patch V.3.1: (nitfix string comment) Prevent duplicate AB names

> # LOCALIZATION NOTE (addressBookTitleEdit): %S will be replaced by the the Address Book's name

Existing nit in string comment ("the the..."); so while we're here, I rewrote that comment:

# LOCALIZATION NOTE (addressBookTitleEdit):
# %S is the current name of the address book.

I suggest that for reasons of uniformity/overview/ease of reading, we should always have
> # LOCALIZATION NOTE (someString):
on its own line, and start the actual comment on a new line.

Comment 52

[:aceman]

Comment on attachment 8900580 [details] [diff] [review]
Patch V.3.1: (nitfix string comment) Prevent duplicate AB names

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

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +287,5 @@
>  {
>    try {
> +    let pref_string_content = "";
> +    let pref_string_title = "";
> + 

You are adding trailing spaces in these empty lines.

@@ +378,5 @@
> +      if (errorArg)
> +        errorText = addressBookBundle.getFormattedString(errorValue, [errorArg])
> +      else
> +        errorText = addressBookBundle.getString(errorValue);
> + 

trailing space

Comment 53

[Thomas D. (:thomas8)]

Attachment: Patch V.3.2: (add string comment example) Prevent duplicate AB names

Sorry for patch burn (was working at night...)

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #51)
> # LOCALIZATION NOTE (addressBookTitleEdit):
> # %S is the current name of the address book.

We should also always add an example where place holders are involved.
E.g. because a string which sounds acceptable in English...
> My Custom AB Properties
...may sound less good for another language like German:
> Mein persönliches Addressbuch Eigenschaften (??)
So German may or may not prefer:
> Eigenschaften von Mein persönliches Addressbuch (Still slightly odd, but anyway...)
Having the example should help to understand the outcome and adjust the localized string accordingly.

Comment 54

[Thomas D. (:thomas8)]

Attachment: Patch V.3.3: (remove whitespace) Prevent duplicate AB names

(In reply to :aceman from comment #52)
> > + 
> 
> You are adding trailing spaces in these empty lines.

Sorry. Removing 4x trailing whitespace per Aceman's comment 52.

Comment 55

[Jorg K (CEST = GMT+2)]

I don't understand what's going on here. This bug is assigned to Aceman and I can't see any signs of having abandoned it, so why hijack it?

I'm comparing Aceman's last version v2.2 to Thomas v3.3
https://bugzilla.mozilla.org/attachment.cgi?oldid=8899580&action=interdiff&newid=8900586&headers=1
and see that only strings have changed.

Looks like there are two strings: duplicateNameText used in abNameOKButton() and invalidNameDuplicate used in onAccept() which appears to be the LDAP code, right?

Can't you just switch the error string based on some property of the duplicate directory? Would that be more than 5 additional lines?

Why do duplicates happen? Well, if they happen at all, users might have a few ABs, so it would be helpful to give the type of the AB where the duplicate exists. Otherwise, just say: "Duplicate name, you lose" :-( - I don't even see the point of repeating the name in the message since the user just typed it.

Comment 56

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #55)
> I don't understand what's going on here. This bug is assigned to Aceman and
> I can't see any signs of having abandoned it, so why hijack it?

And I thought we're working as a team, no? I don't like my friendly assistance being misconstrued as a hostile takeover...
No harm done, as you said, basically I just changed two strings according to Aceman's own proposal ("not to complicate things"), and he's still assigned, and his name is still on the patch.

> I'm comparing Aceman's last version v2.2 to Thomas v3.3
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8899580&action=interdiff&newid=8900586&headers=1
> and see that only strings have changed.
> 
> Looks like there are two strings: duplicateNameText used in abNameOKButton()
> and invalidNameDuplicate used in onAccept() which appears to be the LDAP
> code, right?

Right.

> Can't you just switch the error string based on some property of the
> duplicate directory? Would that be more than 5 additional lines?

It might be easy to do, but imho it's not useful.

> Why do duplicates happen? Well, if they happen at all, users might have a
> few ABs

That's entirely speculative. I'd argue that with few AB's, duplicates are unlikely, because you'll know and see all of them. With 100 ABs, probably in some sort of systematic structure, it might be much easier to end up with a duplicate.

> so it would be helpful to give the type of the AB where the
> duplicate exists.

Please read my comment 48 and comment 50 where I tried to explain why knowing the type of the existing AB imo contributes nothing useful / no extra value to the fact that it's a duplicate AB name; one might even say that it distracts, going against ux-minimalism.

> Otherwise, just say: "Duplicate name, you lose" :-( 

Yes, that's exactly what I'm trying to say in my patch, per Aceman's suggestion. Nothing wrong with that. Same as "This file already exists. Overwrite?" - so here we're saying: "An address book with this name already exists", then we bail out, and according to your own theory, user can figure out from that that he must enter another name or cancel.

> I don't even see the point of repeating the name in the message since the user
> just typed it.

Well, for normal ABs in the current layout, that might apply atm. But there's nothing stopping us from adding more information to the AB properties panel in the future.

Pls see my comment 20: "more specific, more informative, and more visual".
I do believe it helps to understand the problem if the offending name is included in the error message. Also looks ux-consistent with similar messages from OS. Plus, the error message actually covers the new name which you've typed, so again, it looks helpful to include the exact string in the error message so that user can understand immediately what's wrong.

More so for LDAP: There's almost a dozen properties which you can fill on two tabs, and you'll only get the error message when closing the properties dialogue. Chances are you haven't paid much attention to the name because why would anyone want to create a duplicate? In this scenario, it's definitely helpful, even required imo to include the name in the error message, so as to make it easy for the user to fix the right property.

Comment 57

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #56)
> > Can't you just switch the error string based on some property of the
> > duplicate directory? Would that be more than 5 additional lines?
> It might be easy to do, but imho it's not useful.
I know that Aceman had compilation problems last night (due to bustage), so I assume that's why he didn't implement what we discussed. I don't see why a more precise message isn't helpful, especially given that LDAP is not really considered an address book. I think Richard agrees with me, see comment #45. So I'd really like to hear from Aceman before signing off on a patch which uses the same strings with two different names.

Comment 58

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #57)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #56)
> > > Can't you just switch the error string based on some property of the
> > > duplicate directory? Would that be more than 5 additional lines?
> > It might be easy to do, but imho it's not useful.
> I know that Aceman had compilation problems last night (due to bustage), so
> I assume that's why he didn't implement what we discussed. I don't see why a
> more precise message isn't helpful

How would it be helpful? It doesn't change the fact that you have to choose another name because it's a duplicate. So you'd think it's helpful if Windows said "There's another text file/word document/image file with the same name. Overwrite?"?

> especially given that LDAP is not really
> considered an address book. I think Richard agrees with me, see comment #45.

It's always good to agree with good arguments, pro and con...
"I like this" imho isn't enough.

> So I'd really like to hear from Aceman before signing off on a patch which
> uses the same strings with two different names.

That's a technicality which is independent of the string debate, and I've explained why I did this, but I'm not sure if you've seen my explanation.

Comment 59

[Thomas D. (:thomas8)]

Btw, Aceman has provided explicit arguments *against* having type information, see his comment 46.

Comment 60

[:aceman]

I'm a bit lost in what is missing here. I could implement the string being based on the type of AB (as Jorg and Paenglab asked for). I offered the solution in comment 46 (LDAP, or (any) AB) but I think nobody confirmed that would fit the request.

Comment 61

[:aceman]

From nsDirPrefs.h:

/* DIR_Server.dirType */
typedef enum
{
  LDAPDirectory,
  HTMLDirectory,
  PABDirectory,
  MAPIDirectory,
  FixedQueryLDAPDirectory = 777
} DirectoryType;

So we already recognize these types of ABs internally (the HTML and FixedQueryLDAP seem unused).
I think addons can add their own types which we also have to cover in the messages.

Comment 62

[Richard Marti (:Paenglab)]

I thought a bit about it. I think it's not needed to show the type of the AB. It doesn't matter what type the duplicate AB is, it's enough that it is a dupe. Only one dialog saying there is a duplicate AB is enough.

Comment 63

[Jorg K (CEST = GMT+2)]

Comment on attachment 8900586 [details] [diff] [review]
Patch V.3.3: (remove whitespace) Prevent duplicate AB names

OK, creating an address book or LDAP directory:
New > Address Book
New > LDAP Directory

Deleting such a thing:
Are you sure you want to delete this address book and all of its contacts?
Are you sure you want to delete the local copy of this LDAP directory and all of its offline contacts?

Given that we differentiate these things, it would be highly confusing to have the "duplicate name" refer to both as "address book".

So let's see whether giving a detailed message is too hard.

(In reply to :aceman from comment #60)
> I could implement the string being based on the type of AB (as Jorg and Paenglab asked for).
Yes, please.

> I offered the solution in comment 46 (LDAP, or (any) AB) but I think nobody confirmed that
> would fit the request.
Yes, sorry. I'm not sure how the exact solution would look like. Outlook address books are configured via some strange LDAP settings, no? http://kb.mozillazine.org/Using_Outlook_and_OE_contacts_with_Thunderbird_or_Mozilla_Mail.

Comment 64

[Thomas D. (:thomas8)]

Richard, any problems with mentioning the name of the existing duplicate AB in the error dialogue?
- The error message actually covers the new name entered by user (esp. for normal ABs), so it's good to show it on the error message
- I think it's good practice for error dialogues to mention the trouble-causer
- As Aceman pointed out, the capitalization pattern might differ so maybe it's not immediately clear where the duplicate is (TOM's ADDRESSBOOK vs.
 Tom's addressbook)
- ux-consistent with similar OS dialogues, e.g. overwrite file dialogues.
- As Jörg pointed out, somewhat in analogy to our delete AB messages, where we also point out the affected item (and we could equally argue that the user has just selected it, but we're here to be helpful and make things as much non-brainer as possible!)

If you're OK with that, I guess that amounts to ui-r+ for my patch?

Comment 65

[Richard Marti (:Paenglab)]

Yes, show the error message with the existing AB name to show the possible different casing.
And yes that would be a ui-r+

Comment 66

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #63)
> Comment on attachment 8900586 [details] [diff] [review]
> Patch V.3.3: (remove whitespace) Prevent duplicate AB names
> 
> OK, creating an address book or LDAP directory:
> New > Address Book
> New > LDAP Directory
> 
> Deleting such a thing:
> Are you sure you want to delete this address book and all of its contacts?
> Are you sure you want to delete the local copy of this LDAP directory and
> all of its offline contacts?
> 
> Given that we differentiate these things,

For a start, we only differentiate what we know, or rather, what's most exposed in our UI.
For any other type, we'll use generic term "address book".

> it would be highly confusing to
> have the "duplicate name" refer to both as "address book".

I'm still totally failing to see how that would be confusing?
"There's another address book with the same name" - so the problem is about the name, not the type, right?
Let's say there's an existing LDAP directory "foo", even if user wrongly believes that it's a "normal" existing addressbook "foo", how's that confusing for the fact that it's a duplicate and most likely he needs to change the name of the new thing?

We're already using "Address book" as a generic term, too:
- for the main AB window which includes LDAP dirs and all other types
- for less exposed or addon types, as mentioned by Aceman (see comment 61)

> So let's see whether giving a detailed message is too hard.

Needless detail which doesn't contribute to the key message is distraction, against ux-minimalism.

> (In reply to :aceman from comment #60)
> > I could implement the string being based on the type of AB (as Jorg and Paenglab asked for).
> Yes, please.

Richard has since changed his mind (see comment 62), and Aceman seems to be neutral (after initial scepticism against complicating things).

> > I offered the solution in comment 46 (LDAP, or (any) AB) but I think nobody confirmed that
> > would fit the request.
> Yes, sorry. I'm not sure how the exact solution would look like. Outlook
> address books are configured via some strange LDAP settings, no?
> http://kb.mozillazine.org/
> Using_Outlook_and_OE_contacts_with_Thunderbird_or_Mozilla_Mail.

Now you're complicating things, instead of just having a generic term.
If we differentiate only for two types, but not for all other types, I think *that* might cause confusion.
Which is different from the delete case because after all you've really just selected a specific item and triggered "Delete" on that, so the generic term will still the correct in reference.

I'd also appreciate if you could answer my analogy of Windows OS, and if you'd really like windows to tell you the exact type of a duplicate file or is it not enough to know that there's a duplicate file with the same name somewhere?

"An Outlook Express file with the same name already exists" vs.
"A file with the same name already exists"
Which of these two is more efficient to transport the key message of "duplicate file name" problem?
Why would you want users to think about the type of the address book when all they must think about is the name?

Comment 67

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #65)
> Yes, show the error message with the existing AB name to show the possible
> different casing.
> And yes that would be a ui-r+

That's what Aceman's patch implements, and my version unifies the strings for both types, per your comment 62. So maybe you can just indicate ui-r+ on my patch so that we get a clearer picture of who agreed to what?

(If we want one or two variables for the same error message is a secondary question of customizability which is peripheral for UX).

Comment 68

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #66)
> We're already using "Address book" as a generic term, too:
> - for the main AB window which includes LDAP dirs and all other types
> - for less exposed or addon types, as mentioned by Aceman (see comment 61)

I forgot to mention the most obvious generic use of "address book" in our UI:
"All Address Books"
 | 
 +- Personal AB
 +- Custom AB
 +- LDAP Dir
 +- Outlook AB type
 +- Other internal AB types
 +- Addon AB types

Clearly, from the presentation of our UI (and logically, too), anything in the "Directory Pane" is an "Address Book" of some sort. I've already commented why I think using "directory" as a generic term might not be ideal, given that "address book" is natural language which everyone can understand.

Comment 69

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #67)
> So maybe you can just indicate ui-r+ on
> my patch so that we get a clearer picture of who agreed to what?
I see a few patches which had *different* messages also with ui-r+, so there seems to be a little inflation here and the individual approval has lost some value :-(
As you point out, LDAP is a little different.

Comment 70

[Richard Marti (:Paenglab)]

This bug is only for duplicate names. We shouldn't make it too complicated. So we should only say to the user "Hey, this name exists, choose an other one". It doesn't depend on what type it is. I think a user sees a LDAP directory also as an AB. Only people implementing software (TB developers? ;) ) knows the difference.

So keep it simple. :)

Comment 71

[:aceman]

Attachment: patch v3.4

OK, what about this? Merged the strings into one, that contains "address book or directory". If that doesn't pass, I'll change it do "address book" only.

Comment 72

[Thomas D. (:thomas8)]

Comment on attachment 8900938 [details] [diff] [review]
patch v3.4

Thank you Aceman for trying to find a compromise.
However, I think this doesn't actually make it better, hence ui-r-

- This is worse than actually distinguishing the two random types which we're aware of, because now we're making the useless overhead of partial type distinction permanent even for every trivial case where it's just a "normal" AB which already exists. I imagine a user who only has traditional AB's starting to think about the meaning of "directory" and the difference between "address book" and "directory"...
- looking at my English-German dictionary, the semantic difference between "address book" and "directory" is near-zero in our context here. The first synonym of "directory" is "address book". So even if we wanted to make type distinctions (which we don't, as Richard also explained), this wouldn't cut it. The real distinction is in the "LDAP" part of "LDAP directory". That's why the entire left pane which holds all types is historically called "directory pane", i.e. directory is just another generic term meaning "address book". So you're actually saying "An address book or address book with this name already exists". Not helpful imo.
- Pls look at the following equivalent in your file manager to see why this doesn't help:
"A text file or Word document or image file with this name already exists. Overwrite?"
I think it's obvious that type distinction contributes nothing to the fact that it's just "a (read *any*) file with this name" which already exists, and the problem is really in the name, not in the type. Same here for our case.
- I've already explained in detail why "address book" is a suitable generic term which covers all types, both in common user perception and also for TB in particular (e.g. "All address books" directory node), and generic "directory" is somewhat outdated, formal, and less suitable.
- The term "LDAP directory" in our UI is probably unfortunate labeling anyway because it's trying to create a type distinction which for most practical purposes is irrelevant. I think "LDAP address book" or even "remote address book" would probably be just fine.

Comment 73

[Thomas D. (:thomas8)]

N.B. I sincerely regret having introduced this type distinction confusion with my original proposal, because if I hadn't, we'd probably not even talk about this. I'm a bit suprised that in spite of the fact that I've withdrawn my own wrong idea with good reasons, someone is insisting to keep it...

Comment 74

[Thomas D. (:thomas8)]

N.B. Changes of the string would also have to reflect in the string comment example.

Comment 75

[Thomas D. (:thomas8)]

N.B. 3: My mistake of trying to introduce the type distinction was a classical case of "ux-implementation-level", i.e. it was trying to design the UI in ways which are based on the technical implementation (different ways of creating the error message between LDAP and other types), but don't make sense to the user.

Comment 76

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #72)
> Comment on attachment 8900938 [details] [diff] [review]
> patch v3.4
> 
> Thank you Aceman for trying to find a compromise.
> However, I think this doesn't actually make it better, hence ui-r-
> ...
> - The term "LDAP directory" in our UI is probably unfortunate labeling
> anyway because it's trying to create a type distinction which for most
> practical purposes is irrelevant. I think "LDAP address book" or even
> "remote address book" would probably be just fine.

For the avoidance of doubt, I think we should go back to plain generic vanilla:
"An address book with this name already exists:\n• %S"

Comment 77

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #42)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #39)
> > (In reply to Mark Banner (:standard8) (afk 11 - 20 Aug) from comment #15)
> > > Note this also applies to importing address books
> > 
> > Are we going to address this part, too?
> 
> Please file a new bug as that is less interactive, imports a batch of ABs
> and needs more thought on what to do if one is a duplicate name.

There's a number of respective bugs already, because this also includes the question of overwriting/merging/... of existing ABs with the imported AB, so I'm not sure if we need another explicit bug to avoid duplicate AB names?

Comment 78

[Richard Marti (:Paenglab)]

Comment on attachment 8900938 [details] [diff] [review]
patch v3.4

ui-r- for the same reason to not use directory in the string.

Comment 79

[Jorg K (CEST = GMT+2)]

Attachment: 46050.patch - patch v3.5

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aaf42ebb0c89
Prevent duplicate address book names. r=jorgk, ui-r=thomasd,Paenglab

Comment 81

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #73)
> N.B. I sincerely regret having introduced this type distinction confusion
> with my original proposal, ...
;-)

Comment 82

[:aceman]

Comment on attachment 8901159 [details] [diff] [review]
46050.patch - patch v3.5

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

Thanks.

Comment 83

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #55)
> I don't understand what's going on here. This bug is assigned to Aceman and
> I can't see any signs of [him] having abandoned it, so why hijack it?

dito ;)

Thanks. Solid net result, thanks to everybody's attention to detail.