1310442 - Make labels of cmd_properties action-oriented and context-sensitive, e.g. "Edit Contact", "Edit List", etc. (consistent with improved dialogue titles from bug 1312262)

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[Thomas D. (:thomas8)]

STR:

1) In composition's contacts side bar, right-click on a contact.
2) On context menu, look for "Edit contact" menuitem
3) Click on "Properties" menuitem, and look at the title of the resulting dialogue

Actual result
2) Intuitive, action-oriented "Edit contact" is not seen; we only offer less intuitive, less natural, more technical "Properties" menu.
3) After clicking on "Properties...", the resulting dialogue is titled "Edit Contact for ..."

Expected result
Should have intuitive, natural, less technical, action-oriented "Edit Contact" menuitem, consistent with the dialogue title

Implementation:
Rename "Properties" to "Edit Contact" for all respective context menus of contacts (also in Contacts Side bar)

Comment 1

[Thomas D. (:thomas8)]

It has been good UX practice that command labels should preferably be action-oriented / verbal. Noun labels for contextual actions are very rare. Furthermore, context menu label and title of dialogue triggered should be consistent wherever possible.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1310442-editContactContext.patch

Comment 3

[Thomas D. (:thomas8)]

Attachment: 1310442-editContactContext.patch (fix utf-8)(WIP)

Comment 4

[Thomas D. (:thomas8)]

Comment on attachment 8801544 [details] [diff] [review]
1310442-editContactContext.patch (fix utf-8)(WIP)

still need to do the main AB context menu for contacts (any other contact context menus?), so it's WIP

Comment 5

[Thomas D. (:thomas8)]

Attachment: 1310442-editContactContext.patch

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 8801593 [details] [diff] [review]
1310442-editContactContext.patch

So this patch covers both Contacts Side Bar and Main Address Book Window.

The main motivation, as laid out above, is to reduce our usage of the somewhat old-fashioned and technical "Properties" command label and replace it with more intuitive, more natural, less technical, more action-oriented "Edit Contacts...".
To preserve consistency, that also requires changing the button label of the current Properties button on AB main toolbar to "Edit". Interestingly, the tooltip of that button already has "Edit the selected element", and the resulting dialogue title for the major/more frequent usecase is "Edit Contact for John Doe"; i.e. in the current design, the "Properties" label is really the odd one out.

However, Properties still sounds ok for context menu of *ABs* in the directory tree on the left, as we are NOT editing the Address Book but just its name property (and there's bug 1308776 seeking to add "Show as initial default" pref there).

Ideally, but perhaps not required for landing this, I'd like the command entry in the main menu to change dynamically:
We currently have:
Edit > Properties (for both AB properties and contact properties aka Edit contact).
We could make it so that:
- when a single contact is selected, show Edit > Contact
- when an AB is selected, show Edit > Properties

Richard, Aceman, what do you think?

In the next iteration, I might have to reconsider ellipsis. I'm not 100% sure if the ellipses I added are needed or not. On the one hand, to actually complete the action of *editing* a card, further info is required. Otoh, you can also just display the "Edit contact for..." dialogue, which does not require any further user input.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #6)
> The main motivation, as laid out above, is to reduce our usage of the
> somewhat old-fashioned and technical "Properties" command label and replace
> it with more intuitive, more natural, less technical, more action-oriented
> "Edit Contacts...".

Typo, that must be "Edit Contact" (singular), as we only allow editing a single selected contact.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8801593 [details] [diff] [review]
1310442-editContactContext.patch

This makes sense.
But we have three kinds of dialogs in AB: adressbooks, contacts and mailing lists.

An example how it's differentiated is https://dxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCommon.js#178

Comment 9

[Thomas D. (:thomas8)]

Attachment: WIP Patch2 (incomplete): contextual labels for cmd_Properties in Edit Menu, Context Menu, and Button tooltip

So here's a proof of concept wip patch.

With some inspiration from similar code and some tweaking, I believe this is a pretty clean and smart approach to dynamic labels for the same command, using broadcaster elements as pseudo commands which observe the main cmd_properties, but with a flavor for the labels as required by each element type (Main menu and Context Menu with dynamic labels, Button with dynamic tooltips).

Note that we can't change the label on the main command itself because it would broadcast to all observing elements which is not what we want. Likewise, we don't want to change labels of specific UI elements referenced by their ID because we don't know if there'll be more consumers lateron, including addons. So changing labels centrally and making the real elements observe that looks like a cleaner and more sustainable way of doing this.

It's working pretty well in the dirPane, but not yet working for the AB results pane. I guess if I tweak the resultsPane listener in a similar way like the dirPane listener, it should work. Advice welcome.

My check in the isCommandEnabled section of the dirPane listener is not yet correct, because the 'else' branch where I was hoping to catch when focus is in results pane, that's never reached so it's not working.

(In reply to Richard Marti (:Paenglab) from comment #8)
> Comment on attachment 8801593 [details] [diff] [review]
> 1310442-editContactContext.patch
> 
> This makes sense.
> But we have three kinds of dialogs in AB: adressbooks, contacts and mailing
> lists.

Thanks for the catch.
 
> An example how it's differentiated is
> https://dxr.mozilla.org/comm-central/source/mail/components/addrbook/content/
> abCommon.js#178

Yeah, that's a good start, but not yet sufficient.
Unfortunately, we can also have "List" elements in the ab results view.
Needs some more sophisticated differentiation.

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #9)
> Created attachment 8802853 [details] [diff] [review]
> WIP Patch2 (incomplete): contextual labels for cmd_Properties in Edit Menu,
> Context Menu, and Button tooltip
>
> It's working pretty well in the dirPane, but not yet working for the AB
> results pane. I guess if I tweak the resultsPane listener in a similar way
> like the dirPane listener, it should work. Advice welcome.

Typo: Controllers, not listeners. But they are also listening...
In the dirPane (List of Address books), when you select a Mailing List or an Address book, the respective labels of cmd_properties on Menu, Context Menu, and Button tooltiptext are updated accordingly.

Comment 11

[Thomas D. (:thomas8)]

> function goSetLabelAccesskeyTooltiptext(aID, aLabelAttribute, aAccessKeyAttribute, aTooltipTextAttribute)

That helper function really belongs into toolkit's globalOverlay.js, where similar but less practical functions are hosted. For purposes and ease of patch testing, I placed it in abCommon.js.

Comment 12

[Thomas D. (:thomas8)]

Comment on attachment 8802853 [details] [diff] [review]
WIP Patch2 (incomplete): contextual labels for cmd_Properties in Edit Menu, Context Menu, and Button tooltip

I found the real ResultsPaneController in abResultsPane.js (although confusingly, there's also meager twin/referrer abResultsController in addressbook.js), so I'm in the process of adding the missing bits there.

Comment 13

[Thomas D. (:thomas8)]

Attachment: Patch V.3: Contextual labels for cmd_Properties in Contacts Sidebar and AB: Edit Menu, Context Menu, and Button tooltip

Yeah! Here's the full thing, contextual labels everywhere, and fallback to generic "Properties" for mixed selections. Now with cleaner and leaner code. Enjoy!

Richard, this is ready for ui-review and review if you're happy with it.

Comment 14

[Thomas D. (:thomas8)]

This comes with a bunch of strings and it would be nice to see this land for TB52.

Comment 15

[Thomas D. (:thomas8)]

Hmmm, Contacts Side Bar can also show Mailing Lists and mixed selections, so we also need the dynamic labels etc. there. Any starting points like pointers to the respective controllers welcome.

Comment 16

[Thomas D. (:thomas8)]

(In reply to Thomas D. (needinfo?me) from comment #15)
> Hmmm, Contacts Side Bar can also show Mailing Lists and mixed selections, so
> we also need the dynamic labels etc. there. Any starting points like
> pointers to the respective controllers welcome.

I've copied over the context menu <broadcaster> element into abContactsPanel.xul, and the respective Strings into abContactsPanel.dtd. That links up the command correctly (click triggers cmd_properties via cmd_properties-contextMenu broadcaster), but unfortunately and somewhat suprisingly (to me at least), the command updating magic is not automatically hooked up. Meaning that I'm looking to find a way of hooking into the existing controllers, or find the controller for abContactsPanel, or write up code to create or mimic the controller behaviour. Command's isEnabled was working great for the main AB, but I'm not sure how to make that work for abContactsPanel.xul. Any advice welcome.

https://dxr.mozilla.org/comm-central/search?q=cmd_properties&redirect=false
https://dxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abContactsPanel.xul

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 8803410 [details] [diff] [review]
Patch V.3: Contextual labels for cmd_Properties in Contacts Sidebar and AB: Edit Menu, Context Menu, and Button tooltip

The menu label change works great. But I think we need also to change the AB context menu to "Edit Address Book" to be consistent with the two other types. On normal ABs it's only the name to edit but on LDAP ABs where is a lot more you can change.

I hope you can figure out how to do the same in the Contact sidebar.

We are also not really consistent with the dialog titles. The AB dialog is "Address Book Properties" or "Directory Server Properties" for LDAP, the Mailing list Dialog is only "Mailing List" and the contact dialog "Edit Contact for ...". What when we use for all dialogs "Properties". But this would be a other bug.

Comment 18

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #17)
> Comment on attachment 8803410 [details] [diff] [review]
> Patch V.3: Contextual labels for cmd_Properties in Contacts Sidebar and AB:
> Edit Menu, Context Menu, and Button tooltip
> 
> The menu label change works great. But I think we need also to change the AB
> context menu to "Edit Address Book" to be consistent with the two other
> types. On normal ABs it's only the name to edit but on LDAP ABs where is a
> lot more you can change.

I explained in Bug 1312262 comment 2 why I think we should stay with "Properties" for AB: "Edit Address Book" misleadingly suggests that it would edit the *contents* of the AB.

> I hope you can figure out how to do the same in the Contact sidebar.

Grrrr. I did. Lots of hunting for what has now turned out to be a single-line tweak: Helper function was using top.document.getElementById, whereas for contacts sidebar, it must be just document.getElementById. With that, existing command UI updating which I implemented for main AB applies flawlessly to contacts side bar, easily linked up by my flavored command ID, cmd_properties-contextMenu. Will post updated patch soon.

> We are also not really consistent with the dialog titles. The AB dialog is
> "Address Book Properties" or "Directory Server Properties" for LDAP, the
> Mailing list Dialog is only "Mailing List" and the contact dialog "Edit
> Contact for ...". What when we use for all dialogs "Properties". But this
> would be a other bug.

Per Bug 1312262 comment 2, I think that horizontal consistency between dialogues is less important than vertical consistency between command label and dialogue triggered.
In the same comment, I've presented a full proposal to make dialogue captions leaner and more useful, with icons and the name of the edited element (as we already do for contacts).

Comment 19

[Thomas D. (:thomas8)]

Attachment: Patch V.4: Contextual labels for cmd_Properties in Contacts Sidebar (!) and AB: Edit Menu, Context Menu, and Button tooltip

As explained in comment 18, dynamic labels now also working for contacts sidebar.
I deliberately included a broadcaster of type cmd_properties-menu and the respective strings so that we can land them for future use: Composition lacks an Edit > Contact menu when contacts are selected in contacts side bar, same as we have for Edit > Rename Attachment.

In the next iteration, I'd like to include the "New Mailing List" and "Edit Mailing List" dialogue titles similar to Richard's patch in the other bug so that we have those in case we can't land dialogue captions with dynamic element name in time.

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V.4.1: Contextual labels for cmd_Properties in Contacts Sidebar (really!) and AB: Edit Menu, Context Menu, and Button tooltip

Sorry, forgot the main hook for dynamic labels on contacts side bar, command="cmd_properties-contextMenu"...

Comment 21

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (needinfo?me) from comment #19)
> In the next iteration, I'd like to include the "New Mailing List" and "Edit
> Mailing List" dialogue titles similar to Richard's patch in the other bug so
> that we have those in case we can't land dialogue captions with dynamic
> element name in time.

No need to implement this. This is already working in my patch and the LDAP dialog is also working locally. Should the dialogue captions with dynamic element name not be ready we can use my next iteration and move the dynamic title to a new bug.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8803786 [details] [diff] [review]
Patch V.4.1: Contextual labels for cmd_Properties in Contacts Sidebar (really!) and AB: Edit Menu, Context Menu, and Button tooltip

Aceman is the much better reviewer.

Comment 23

[:aceman]

Comment on attachment 8803786 [details] [diff] [review]
Patch V.4.1: Contextual labels for cmd_Properties in Contacts Sidebar (really!) and AB: Edit Menu, Context Menu, and Button tooltip

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

So, in the context menu in the AB pane you still have "Properties". In the cards/results pane, you have "Edit <object>". In the Edit menu there is "<object> Properties". Seems fine to me everybody can use what he likes :)

It is interesting that you can reference a broadcaster via command= (in a menuitem) where the broadcaster observes a command. And it works.

While testing this, I noticed that the first time you open the contacts sidebar in compose window and rightclick a card, the delete and edit items are incorrectly disabled and the edit item does not even have a label (only a hotkey). This bug seems to not be caused by this patch, however this patch makes it worse with the missing label. The bug causes you updating of label to not run. Can you see why it is not run? Subsequent rightclicks do work fine.

::: mail/components/addrbook/content/abCommon.js
@@ +841,5 @@
> + * command or broadcaster element. JS does not allow omitting function arguments
> + * in the middle of the arguments list, so in that case, please pass an explicit
> + * falsy argument like null or undefined instead; the respective attributes will
> + * not be touched. Examples:
> + * 

Trailing space.

@@ +864,5 @@
> +  if (node) {
> +    if (aLabelAttribute) {
> +      let value = node.getAttribute(aLabelAttribute);
> +      if (value)
> +        node.setAttribute("label", value);

It may be that the previous version of the item had a label but this one doesn't. You can't just skip setting the label here, but need to clear the previous one. The same for accesskey and tooltiptest. I see that e.g. goSetMenuValue does not have the clearing. The problem is hidden in that there is the same accesskey for all object types (e.g. for Properties or Edit), so if one object didn't have an accesskey, using the previous key would not be noticable. But e.g. for the tooltiptext using an incorrect one would be bad. I have tested this happens when I removed one of the tooltips on the edit button.
So, in case !aLabelAttribute, clear the label attribute. In case aLabelAttribute was set but it was not found, I'd dump() a warning message. Or do we expect the attribute to not be there at some cases? Similarly, can "node" be not found? Is that a fatal problem or not?

Comment 24

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #23)
> Comment on attachment 8803786 [details] [diff] [review]
> Patch V.4.1: Contextual labels for cmd_Properties in Contacts Sidebar
> (really!) and AB: Edit Menu, Context Menu, and Button tooltip
> 
> Review of attachment 8803786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, in the context menu in the AB pane you still have "Properties". In the
> cards/results pane, you have "Edit <object>". In the Edit menu there is
> "<object> Properties". Seems fine to me everybody can use what he likes :)

Different users have different needs, that's my creed ;)
Jokes aside, this is actually carefully crafted!

On context menu, it's "Properties" for any flavors of address books, because you'll really just get the AB's properties, but you can't "Edit" its content. It's "Edit" for contacts and mailing lists (also in the directory tree) where you can really edit the actual content, not just some properties. 

For the main menu, you have to read the whole menu sequence to get the correct phrase:
Inserting an image is "Insert > Image". Editing the content of an item is "Edit > Item". So that would be "Edit > Contact" in our case. For AB, it must be Edit > (Address book) Properties, see above.
I decided to add the (item type) because I thought it's helpful to clarify what's really going to happen and if your selection is right (especially for users with accessibility problems who are more likely to use the menus). The same for tooltips on "Edit" button, they let you know what item type you'll actually be editing. The reason why I decided to keep the word "Properties" in the menu is twofold:
1) At the end of the day, it's still the same command (cmd_Properties), having the same keyboard shortcut (Ctrl+I), and we should reflect that somehow in the wording. Btw Alt+Enter is missing on windows in AB, except in contacts sidebar where I implemented it.
2) Given 1), I wanted to ensure that users can always use the same access key for this command, regardless of the item type at hand. Wrt l10n, that can only be ensured if we have one identical word on each command flavor. That magic word is Properties. So that's how it becomes "Edit > Contact Properties", which I think is still pretty consistent with it's shorter version on context menu, "Edit Contact".

All in all, of course the current use of "Properties" for everything all over the place also had a certain charme of ux-minimalism. But also the charme of a technical dinosaur. So for all the reasons mentioned in the description of this bug, I think it's a worthwhile change which improves the UX with more natural, more action-oriented, more contextual labels.

> It is interesting that you can reference a broadcaster via command= (in a
> menuitem) where the broadcaster observes a command. And it works.

Like a charme! I love it. Perfectly lean and clean connections, like Lego bricks ;) I really don't understand why XUL has fallen out of favor with Mozilla :(

> While testing this, I noticed that the first time you open the contacts
> sidebar in compose window and rightclick a card, the delete and edit items
> are incorrectly disabled and the edit item does not even have a label (only
> a hotkey).
> This bug seems to not be caused by this patch, however this patch
> makes it worse with the missing label. The bug causes you updating of label
> to not run. Can you see why it is not run? Subsequent rightclicks do work
> fine.

I used to see this occasionally while the patch was still WIP, but for the final version, I can't reproduce. Are you able to reproduce? Is it something about recycled windows? I'd think it's quite impossible because isEnabled must run before anything, and I think there are at least two rounds of enforced command updating, and I imagine I even checked that. Or maybe that was the main AB?
If you want, we can set generic labels on all menuitems and the tooltip on the button, in xul, so that if the updating initially fails, we don't go blank. But really that's a bug which isn't caused by my patch, and it shouldn't happen.

> ::: mail/components/addrbook/content/abCommon.js
> @@ +841,5 @@
> > + * command or broadcaster element. JS does not allow omitting function arguments
> > + * in the middle of the arguments list, so in that case, please pass an explicit
> > + * falsy argument like null or undefined instead; the respective attributes will
> > + * not be touched. Examples:
> > + * 
> 
> Trailing space.

They had to be somewhere... I just wanted to make sure you are paying attention ;)

> @@ +864,5 @@
> > +  if (node) {
> > +    if (aLabelAttribute) {
> > +      let value = node.getAttribute(aLabelAttribute);
> > +      if (value)
> > +        node.setAttribute("label", value);
> 
> It may be that the previous version of the item had a label but this one
> doesn't. You can't just skip setting the label here, but need to clear the
> previous one. The same for accesskey and tooltiptest. I see that e.g.
> goSetMenuValue does not have the clearing. The problem is hidden in that
> there is the same accesskey for all object types (e.g. for Properties or
> Edit), so if one object didn't have an accesskey, using the previous key
> would not be noticable. But e.g. for the tooltiptext using an incorrect one
> would be bad. I have tested this happens when I removed one of the tooltips
> on the edit button.

Yeah, I had seen that and I thought I can get away with it, but alas, reviewer with eagle's eyes!!! ... I improved the original goSetMenuValue() function by combining label, accesskey, and tooltip into one function. But I didn't want to improve it too much...

I think it's quite unlikely that an existing label, accesskey, or tooltip gets completely removed and replaced by an empty string. But you are right, we never know and there's a certain risk of unintended errors. But I think I would only clear if explicit "" is passed, not for undefined/null. Programmers should know what they are doing, and there might be cases where you want to change just the label and keep accesskey and tooltiptext... Btw label losing its accesskey should never happen... I think as long as the function description is clear, anything goes...

> So, in case !aLabelAttribute, clear the label attribute. In case
> aLabelAttribute was set but it was not found, I'd dump() a warning message.
> Or do we expect the attribute to not be there at some cases? Similarly, can
> "node" be not found? Is that a fatal problem or not?

I just kept the silent error handling which was in the original function. But you are right, "node not found" is fatal, and needs a dump().

Comment 25

[Thomas D. (:thomas8)]

Attachment: New code for function goSetLabelAccesskeyTooltiptext()

Please find attached the new code for goSetLabelAccesskeyTooltiptext() function, now applying empty strings correctly and emitting error messages for failures as requested by aceman.

Aceman, can you please update the patch, I'm failing to make TortoiseHg combine the incremental changes into one consolidated patch, short of clipping my hg history and injecting all the code into all files again, which can't be the right way. Tia.

There are two other minimal changes which also need to go into the patch:

1) An added label in abContactsPanel.xul to mitigate the bug mentioned by aceman, where command updating (isEnabled) allegedly fails only for first time context menu in contacts side bar (I still can't reproduce that):

>              command="cmd_delete"/>
>    <menuseparator/>
    <menuitem label="&propertiesContext.label;"
>              key="key_properties"
>              command="cmd_properties-contextMenu"/>
>  </menupopup>

2) A new comment in abResultsPane.js:

>          default:
>            //use generic set of attributes declared above
>            break;
>        }
        // This code is shared between main AB and composition's contacts sidebar.
        // Note that in composition, there's no cmd_properties-button (yet);
        // the resulting dump() should be ignored.
>        goSetLabelAccesskeyTooltiptext("cmd_properties-button", null, null,
>          tooltipTextAttr);

Comment 26

[Thomas D. (:thomas8)]

Note that for customAttr="", hasAttribute("customAttr") will return true, so we'll correctly apply the empty string. Edge cases: customAttr= or customAttr (both without value) are invalid syntax which is rejected at xul level.

Comment 27

[Thomas D. (:thomas8)]

Can someone bisect the string changes into a separate patch and land that so that we don't get stuck on the string freeze?

Comment 28

[:aceman]

Attachment: Patch V.4.2: Contextual labels for cmd_Properties in Contacts Sidebar (really!) and AB: Edit Menu, Context Menu, and Button tooltip

Thanks, updated the patch. I think this is finished now.

Comment 29

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/71581aa2080102ab14918586874cdf0285ac1ba0

Comment 30

[Jorg K (CEST = GMT+2)]

And one day we'll all acknowledge that on US-ASCII is allowed in commit comments. So the author goes down into eternity as Thomas Düllmann.

Comment 31

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #30)
> And one day we'll all acknowledge that on US-ASCII is allowed in commit
> comments. So the author goes down into eternity as Thomas Düllmann.

The typo in your comment makes it a bit hard to understand, are you saying *only* or *no* US-ASCII is allowed in comment comments?
Hmmm. :/ I was actively looking into that problem and both on Notepad++ and here on bmo my patch shows "ü" correctly, and the character set is shown as UTF-8.
There's a known bug in TortoiseHg that it can't handle UTF-8 commit comments, so I understood it's only in TortoiseHg that it displays wrongly. Because when it showed correctly in TortoiseHg, it came out wrongly in the patch. So now I'm surprised that it's now displaying wrongly in the web representation of HG, looking at the link of comment 29.
Are you saying that the commit comment must be US-ASCII, but the rest of the file must be UTF-8 especially when we need special characters like the single character for ...? That looks like a buggy setup then because the commit comment comes as part of the same file like the rest of the patch, so how can we have different formats in one file?
Finally, can you pls advise how to get this right, and who between patch author and check-in person must see to that?

Comment 32

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #30)
> And one day we'll all acknowledge that on US-ASCII is allowed in commit
> comments. So the author goes down into eternity as Thomas Düllmann.

If there's any way to fix this (even backout and re-push), I'd like to see this fixed please. After many hours of unpaid work, I think getting patch author's name acknowledged correctly in German spelling is not too much to ask. Iirc we've succeeded on this before.

Comment 33

[Jorg K (CEST = GMT+2)]

Only US-ASCII for the user. The patch can be UTF-8 or even ANSI (ISO-8859-1) in some cases. I guess HG puts the commit message into a different place than the file content.

It would be good if the patch author whose name is not pure US-ASCII knew. We have a contributor Grießmayr and I checked him in with "ss: https://hg.mozilla.org/comm-central/rev/b34a52928c12
We also have a  Terje Bråten and I fixed him up, too: https://hg.mozilla.org/comm-central/rev/ab68d375e7e4

You have run into this problem every single time, so it's time to learn it ;-)

Comment 34

[Richard Marti (:Paenglab)]

I'm sorry, I don't know how this could be solved. Maybe you need to use Duellmann to look more correct.

Comment 35

[Jorg K (CEST = GMT+2)]

No way to fix this other then backing it out and relanding it.

Actually, I stand corrected, I checked a few of your patches and they are mostly right, for example:
https://hg.mozilla.org/comm-central/rev/1ec5958134965cf1e346e0f405c76015a0631699

So no idea what happened here. I still suggest to use "ue" instead of "ü".

Comment 36

[Jorg K (CEST = GMT+2)]

Maybe a Windows problem. All the T/D pushes done be Aleth on Mac and Magnus on Linux were right. I haven't found a wrong one, but I remember seeing one somewhere.

Comment 37

[rsx11m]

Guys, AB is shared mailnews/ code, thus if you change entities, please don't just quietly break the suite in the future? Thanks.

Comment 38

[:aceman]

Sorry, my bad. Unfortunately only half of AB is shared (not sure why) so seeing the dependencies is not obvious. But calling the new function from /mailnews/ code should have been a clue :(

Comment 39

[rsx11m]

Hmm, this landed on 11/12 whereas bug 1315924 was filed 11/08 already, makes me thinking that it's not quite the source for that specific regression. But anyway, we'll have to fix the suite/ strings.

Comment 40

[rsx11m]

(In reply to :aceman from comment #38)
> Sorry, my bad. Unfortunately only half of AB is shared (not sure why) so
> seeing the dependencies is not obvious. But calling the new function from
> /mailnews/ code should have been a clue :(

No problem (almost), but the mail/ portions may have been forked at some point from the mailnews/ code and thus require an application-specific solution. We'll have to check if the JS/XUL parts can be made a straight port to suite/ or not.