490097 - Clean up autoconfig strings and move them from content to locales

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Phil Ringnalda (:philor)]

From bug 422814 comment 176:

[[[
Not counting things where Bryan doesn't like the wording, there's certinfo.url,
depending on how it goes 10 to 16 customfields/enableURLs strings, one of the
pair of cleartext strings where in one we say "email" and the other we say
"email and password", the two where we have double-hyphens for an em dash, and
perhaps ten or fifteen removals where we're localizing what I think are
developer-targetted console error messages like "enum value not supported" that
I'm inclined to say we shouldn't be making our localizers try to translate.
]]]

certinfo.url is gone, and the conclusions in the m.d.l10n thread (subject's something like "Sorry about those disappearing autoconfig files") talking about the localized developer messages in the console were inconclusive, but otherwise, what I said (and what I missed, too, probably).

Comment 1

[Phil Ringnalda (:philor)]

Don't *think* anything else there is still the case, but from bug 422814 comment 75

[[[
>+selfsigned_incoming=This is an mis-configured incoming server.  The certificate used by the server can't be trusted, so while the connection is encrypted, we can't be sure that the server is controlled by who it claims to be.

Either "misconfigured" (go go gadget spellchecker, saying that's wrong but
misconfiguration is right), or perhaps better "incorrectly configured" or
"incorrectly set up" - all of which still fail to adequately get across "You
didn't do anything wrong, but your mail server was set up by someone who didn't
know what they were doing."
]]]

Comment 2

[Magnus Melin [:mkmelin]]

Please also make 
<!ENTITY ssltls.label                    "SSL / TLS">

be like it's elsewhere:
<!ENTITY ssltls.label                    "SSL/TLS">

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Working on this, I'm setting a max-width for the error message right now to force wrapping the long lines.  It's not ideal yet but I'm making progress.

I haven't worked on string changes yet until I've removed all the unnecessary strings first.

(In reply to comment #2)
> be like it's elsewhere:
> <!ENTITY ssltls.label                    "SSL/TLS">

Will fix this too.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Should have WIP sometime today

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

I have a strings change patch somewhere that I'll put up here soon however bwinton is taking over the rest of this bug.

Comment 6

[David :Bienvenu]

Since we're trying to push b3 sooner rather than later, and we'll be trying to do a string freeze soon (if only a slushy freeze), it would be great to see some movement on this...

Comment 7

[Stefan Plewako [:stef]]

We should improve following lines (these are only most obvious ones) otherwise they will be confusing for translators and/or users.

mail/base/content/accountCreation.dtd:
<!ENTITY go_button.label "Go"> → test configuration again?
<!ENTITY outgoing_is_checked.label "I've double-checked the outgoing settings."> → don't even how to display this
<!ENTITY incoming_is_checked.label "I've double-checked the incoming settings."> → as above
<!ENTITY understood.label "I know what I'm doing.">  → no idea what this is about, some broken implementation of OK/Accept button?
<!ENTITY getmeout.label "This is scary, let me out."> → as above

mail/base/content/accountCreation.properties:
finished_with_errors=Well, that didn't go terribly well → cute but we should mention sth about errors...

mail/base/content/accountCreationUtil.properties:
enum_value.error=enum value not supported → this is about JS?


In general the language of that window is very different from the rest of the app, it is probably intended but for users it will be strange and confusing.

The window is also chaotic and very hard to test, buttons are everywhere (not in normal places), badly labeled (like "go" button), some string ids are almost useless (e.g. getmeout.label).

"Go to advanced settings" link is misleading and should be changed too... it should be a button with "create account and inspect account configuration" like label.

Additionally, I don't see accesskeys at all...

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: A picture showing the hidden labels.

(In reply to comment #7)
> mail/base/content/accountCreation.dtd:
> <!ENTITY outgoing_is_checked.label "I've double-checked the outgoing
> settings."> → don't even how to display this
> <!ENTITY incoming_is_checked.label "I've double-checked the incoming
> settings."> → as above
> <!ENTITY understood.label "I know what I'm doing.">  → no idea what this is
> about, some broken implementation of OK/Accept button?
> <!ENTITY getmeout.label "This is scary, let me out."> → as above

To display those, Choose File >> New >> Mail Account.
Enter "[e-mail]" as the name, email, and password.
Select Next, then select "Create Account".
You should see a screen similar to the one attached to this bug.

> mail/base/content/accountCreationUtil.properties:
> enum_value.error=enum value not supported → this is about JS?

Yup.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch, mainly to show the progress made so far.

I've moved the accountCreation.dtd and accountCreation.properties files, and made the following changes:
SSL / TLS -> SSL/TLS.
This is an insecure incoming/outgoing server. -> The incoming/outgoing server was incorrectly set up.
two dashes -> em dash.

Finally, I've added a few localization notes to some of the strings Stefan mentioned.  I expect I'll be changing the strings themselves in a future patch, but hopefully this will give a little more context in the meantime.

Thanks,
Blake.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: A similar screen from Firefox.

Here's what Firefox displays for a similar security warning…

The changes that this suggests to me are:
I've double-checked the incoming/outgoing settings. -> I understand the risks.
This is scary, let me out. -> Get me out of here!
I know what I'm doing. -> Continue  (Another option could be "Create Account" again, to mirror the button you clicked on the previous screen.)

I'll throw up a picture of the warning screen with these changes made, so that you can see how it looks.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: The new warning messages.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch with more strings moved.

Okay, I think this is at the point where Bryan could offer some useful advice.

Thanks,
Blake.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm not excited about the warning.label being a single label that describes all the situations.  This might need to be spun out as a separate bug but I think it would be good to describe the situation a little more clearly

e.g.

This Connection is Untrusted
This Connection is Insecure
There are Problems with these Connections

I can't remember if we use two different dialogs for the incoming and outgoing servers or if we pile them into the same warning dialog.  I've assumed that they get lumped together and made strings for that case.

For the text I've changed it from "The $X was incorrectly set up" to line up more with the Firefox wording.  I'm afraid people will think that _they_ incorrectly set up the server; especially since they just went through our settings dialog.  Most times the incorrect server settings are the fault of one person and the burden placed on everyone else who can't change it.


Also I'd like to create some separation of the 'lecture/details' and the initial message.  I've put the initial message up at the top of the dialog with the lecture/details below them.

-

cleartext_incoming=You've setup &brandShortName; to connect to the incoming mail server <strong>$server</strong>, which is an insecure server.

selfsigned_incoming=You've setup &brandShortName; to connect securely to the incoming mail server <strong>$server</strong> but we can't confirm that your connection is secure.

cleartext_outgoing=You've setup &brandShortName; to connect to the outgoing mail server <strong>$server</strong>, which is an insecure server.

selfsigned_outgoing=You've setup &brandShortName; to connect securely to the incoming mail server <strong>$server</strong> but we can't confirm that your connection is secure.

-

These sentences don't flow well one directly after the other.  I might try to reword these in order to solve that problem or just create combination sentences.

These are the lecture/details sentences that tell people more about what is wrong with either of their servers.  Each of these lectures are the same so I didn't want to have them repeat themselves at the top.
-

selfsigned_details=Normally secure mail servers will present a trusted certificate to prove you are connecting to the server it claims to be.  The connection to the mail server will be encrypted but cannot be validated as being the correct server.

cleartext_details=Insecure mail servers do not use encrypted connections to protect your passwords and private information. <strong>By connecting to this server you could expose your password and private information.</strong>

-

I think we should use the FF "Technical" details collapse system.  By providing the host name in the original text then we don't need to bring up the jargon people are less likely to understand.  The users are in this dialog because they don't want these details so we should continue trying to hide them.

-

\/ Technical Details

     Incoming settings:  imap.harborside.com:143 (SSL/TLS)
     Outgoing settings:  smtp.harborside.com:465 (SSL/TLS)

-

I don't think this link is going to be the best link for this problem, hopefully rebron will have something better we could work with eventually.  For now it could be a placeholder.

-

lectureYourProvider.description "&brandShortName; can allow you get to your mail using the provided configurations.  However you should contact your administrator or email provider regarding these improper connections. See the <a href='http://www.mozilla.org/support/thunderbird/faq'>Thunderbird FAQ</a> for more information.">

-

We've gone back and forth over this one a number of times.  I know this button actually has to create an account but it doesn't connect to that account so I don't think there is any real damage to worry about.  Anyone who clicks on the 'Advanced Config' button has to be willing to take some risks and deal with an account they later might need to remove.

How does this sound?

-

<!ENTITY gotoadvanced.label              "Create in Advanced Editor">

-

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with some of clarkbw's suggestions.

(In reply to comment #13)
> I'm not excited about the warning.label being a single label that describes
> all the situations.  This might need to be spun out as a separate bug but I
> think it would be good to describe the situation a little more clearly
> e.g.
> This Connection is Untrusted
> This Connection is Insecure
> There are Problems with these Connections

Since it's just one label for both the incoming and outgoing connection, I
think that it would be better to spin it out as a separate bug.

> I can't remember if we use two different dialogs for the incoming and outgoing
> servers or if we pile them into the same warning dialog.  I've assumed that
> they get lumped together and made strings for that case.

Yup, just one dialog for both.

> For the text I've changed it from "The $X was incorrectly set up" to line up
> more with the Firefox wording.

Cool.  Changed.

> Also I'd like to create some separation of the 'lecture/details' and the
> initial message.  I've put the initial message up at the top of the dialog
> with the lecture/details below them.

Okay, this caused the content to overflow the bounds, so I added scrollbars for this iteration.  We can re-word/polish it for beta 4.

> cleartext_incoming=You've setup &brandShortName; to connect to the incoming
> mail server <strong>$server</strong>, which is an insecure server.

Because this is in a properties file, I've removed the formatting.

> I think we should use the FF "Technical" details collapse system.  By
> providing the host name in the original text then we don't need to bring up
> the jargon people are less likely to understand.  The users are in this
> dialog because they don't want these details so we should continue trying to
> hide them.
> \/ Technical Details
>      Incoming settings:  imap.harborside.com:143 (SSL/TLS)
>      Outgoing settings:  smtp.harborside.com:465 (SSL/TLS)

I agree.  Can this also be a new bug/part of the polish bug for beta 4?

> I don't think this link is going to be the best link for this problem,
> hopefully rebron will have something better we could work with eventually.
> For now it could be a placeholder.

This link isn't rendering, perhaps because it's in an entity?
For now I've removed the link, but left the text.

> lectureYourProvider.description "&brandShortName; can allow you get to your
> mail using the provided configurations.  However you should contact your
> administrator or email provider regarding these improper connections. See the
> <a href='http://www.mozilla.org/support/thunderbird/faq'>Thunderbird FAQ</a>
> for more information.">

If you really want the link there, and I agree that it's a good idea, let's
log it as a new bug for beta 4.

> How does this sound?
> <!ENTITY gotoadvanced.label              "Create in Advanced Editor">

I love it!

Thanks,
Blake.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 387368 [details] [diff] [review]
The previous patch, with some of clarkbw's suggestions.

Looks good; r=dmose with just a few nits.  I'll tweak them and upload a new patch momentarily.

>--- a/mail/base/content/accountCreation.dtd
>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
>
> [...]
>
>+<!ENTITY lectureYourProvider.description "&brandShortName; can allow you get to your mail using the provided configurations.  However you should contact your administrator or email provider regarding these improper connections. See the Thunderbird FAQ for more information.">

Looks like there wants to be another space before "See", though it's conceivable that XUL will collapse those.

>--- a/mail/base/content/accountCreation.properties
>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties
>@@ -1,9 +1,13 @@
> 
> [...]
>right
>+# LOCALIZATION NOTE: %1$S will be the name of the app (i.e. ThunderBird), %2$S will be the hostname of the server the user was trying to connect to.
>+cleartext_incoming=You've setup %1$S to connect to the incoming mail server %2$S, which is an insecure server.
>+selfsigned_incoming=You've setup %1$S to connect securely to the incoming mail server %2$S but we can't confirm that your connection is secure.

%2$S should be followed by a comma here.

>+cleartext_outgoing=You've setup %1$S to connect to the outgoing mail server %2$S, which is an insecure server.
>+selfsigned_outgoing=You've setup %1$S to connect securely to the incoming mail server %2$S but we can't confirm that your connection is secure.

Same thing here.

>+selfsigned_details=Normally secure mail servers will present a trusted certificate to prove you are connecting to the server it claims to be.  The connection to the mail server will be encrypted but cannot be validated as being the correct server.

The first sentence here doesn't sound grammatically correct; I'll try to figure out something that sounds smoother.

>+cleartext_details=Insecure mail servers do not use encrypted connections to protect your passwords and private information. By connecting to this server you could expose your password and private information.

An extra space before "By" would be good.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

"Normally, a secure mail server will present a trusted certificate to prove that the it is really the server it claims to be.  The connection to the mail server will be encrypted but cannot be validated as being the correct server." is what I ended up with.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

let's try that again!

"Normally, a secure mail server will present a trusted certificate to prove that it is really the server it claims to be.  The connection to the mail server will be encrypted but cannot be validated as being the correct server."

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: revised to include review comments

philor pointed out that two spaces is actually a typography hack for monospace fonts, and since we're not using one of those here, the right thing to do was actually to make the sentences all be separated by a single space, so I did that too.  Carrying forward ui-r? request.  Note that while I've reviewed these all visually and made sure everything builds, I haven't yet had a chance to run through the various screens by hand, and I need to sleep now.  If someone else feels like doing that, feel free!

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 387376 [details] [diff] [review]
revised to include review comments

nice, thanks for the revisions!

Comment 20

[Vlado Valastiak [:wladow]]

Comment on attachment 387376 [details] [diff] [review]
revised to include review comments

>+selfsigned_outgoing=You've setup %1$S to connect securely to the incoming mail server %2$S, but we can't confirm that your connection is secure.
s/incoming/outgoing

I just wanted to note, that all these files went live accidentally 2(?) months ago and localizers were actively working on them. From what I remember, more than 10 teams have already pushed their files to the repos before these were backed out. So, in the light of this fact I suggest to take classic L10n way here and when changing the string pls change its identifier as well. Moreover identifiers should be as much descriptive as possible and I tell you
>+<!ENTITY go_button.label                 "Re-test Configuration">
doesn't meet this recommendation at all ;)

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: dmose's patch, with the changed entities/strings renamed.

(In reply to comment #20)
> (From update of attachment 387376 [details] [diff] [review])
> >+selfsigned_outgoing=You've setup %1$S to connect securely to the incoming mail server %2$S, but we can't confirm that your connection is secure.
> s/incoming/outgoing

Fixed.

> I suggest to take classic L10n way here and when changing the string pls
> change its identifier as well.

Done.

> Moreover identifiers should be as much descriptive as possible and I tell you
> >+<!ENTITY go_button.label                 "Re-test Configuration">
> doesn't meet this recommendation at all ;)

I've changed it to retest.label, but in all fairness, there was a localization note above the entity which said:
<!-- LOCALIZATION NOTE (go_button.label): This is the text that is
     displayed on the button which will re-guess the account configuration,
     taking into account the settings the user has changed.
     -->
I agree it's not ideal, but I hope it helps a little.  ;)

The other thing I changed was the wording of lectureYourProvider.description,
from "…However you, should contact…" to "…However, you should contact…", based on the example from Apple's dictionary app:
  1. used to introduce a statement that contrasts with or seems to contradict
     something that has been said previously : People tend to put on weight in
     middle age. However, gaining weight is not inevitable.

(I'm not obsoleting the previous patch in case you didn't want to change the entity/string names for some reason.)

Thanks,
Blake.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 387467 [details] [diff] [review]
dmose's patch, with the changed entities/strings renamed.

r=dmose, carrying forward ui-review.

I've filed bug 503413 so that we can actually get automated tests for the autoconfig code in the future; adding in-testsuite? to this bug as well.

The overflow: auto changes seems reasonable.  Please file a spinoff polish bug (if there isn't one already).  Candidate changes include:
 
* make the window resizable too
* it feels like the warning dialog has a very large amount of text on it, to the point where users are likely to ignore it.  The indentation seems arbitrary, which adds to the confusion.
* "Get me out of here" seems like misleading text for the warning page button, because it implies aborting the config process entirely, but actually just backs up one screen.

I'll push a version with a single indentation fix after I deal with the late-l10n stuff.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #22)
> The overflow: auto changes seems reasonable.  Please file a spinoff polish bug
> (if there isn't one already).  Candidate changes include:

Created, as Bug 503436.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

I spent some time sorting out details while drafting up the late-l10n message, and I realized that I had miscounted the number of new strings that we'd be (effectively) dumping on the localizers a week after slushy string freeze and a week before firm string freeze.  Given that we've (well, mostly I've) already caused the localizers a bunch of pain here when autoconfig first landed, and given that we have another beta, this seems like it would be exceptionally inconsiderate on our end and frustrating to them to do now.  So after some discussion with bwinton, we've decided to split this patch apart.  The new strings will be updated in place (ie in content/) for b3, but we won't move them to locales/ until the tree re-opens for b4.  The upshot of this is that late-l10n will be necessary here.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Attachment: The patch, without moving the files. [checked in]

As per IRC.  And with some more indentation fixes.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

In reply to comment #24, where I wrote:

> The upshot of this is that late-l10n will be necessary here.

by which I _actually_ meant "will _not_ be necessary here."

Comment 27

[Stefan Plewako [:stef]]

(In reply to comment #24)

All possibilities are bad but my filling is that the chosen one is worst - I don't see much sens in shipping l10n betas with greatest/most important/one that needs great testing&qa love feature hardcoded. This simply breaks/is changing far too many things in favor of not violating slushy string freeze.

Please reconsider.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #27)
> 
> All possibilities are bad 

Agreed; there's no great outcome to be had here.

> but my filling is that the chosen one is worst - I
> don't see much sens in shipping l10n betas with greatest/most important/one
> that needs great testing&qa love feature hardcoded.
> This simply breaks/is changing far too many things in favor of not violating
> slushy string freeze.
 
I understand that it hurts to have one fewer beta's worth of feedback on an autoconfig UI that's fully usable to non-English speaking audiences.

That said, I can only see two ways we could achieve that.

a) slip the beta.  This beta's been held far too long already; we badly need to get back into a more agile development mode.  Slipping feels like a non-starter to me.

b) land >100 strings late, which is likely to make a non-trivial number of the people volunteering to do the actual work (some of whom already feel mistreated by an earlier iteration of this feature) fairly upset.

Now would be a great time for either sipaq or Pike (both now CCed on the bug) to jump in and tell me whether my feel for the l10n community's likely reaction is a accurate...

It's worth keeping in mind that this is not the last Tb3 beta that we'll be shipping.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 387803 [details] [diff] [review]
The patch, without moving the files. [checked in]

r=dmose

Comment 30

[Simon Paquet [:sipaq]]

The situation sucks in so many ways, that it is hard to fathom.

The thing is that both Stefan and Dan are right here.

- Stefan is right in saying that such an important feature should get adequate 
  exposure and QA and with such a diverse community as TB has, this obviously 
  includes a fully localized feature
- Dan is right, that dumping 100+ strings on localizers now, is a sure way to 
  royally piss them off. You simply can't do that 6 days and 8 hours before the 
  l10n-mozilla-1.9.1 freeze date. It would probably have been possible early 
  in the slushy string freeze cycle, e.g. a week ago.

So without a slipping by at least a week, I see no elegant way out of this
mess. Either way, you will piss people off. 

All things considered, if there aren't any significant issues on the code side,
which would warrant a slip as well, I would probably advise to release this
thing and let the strings be hardecoded for this release.

A possible strategy to at least partly mitigate this could be to split up the
string files and the land the more user-visible strings in mail/locales now and
land all the technical strings after beta3 is out. Really not an optimal
solution, but the *only* thing that came to my mind to at least partly
alleviate this.

Does that make sense?

Comment 31

[Axel Hecht [:Pike]]

TBH, I glanced at the patch, and I'd give it an r-.

<!ENTITY understood2.label               "Create Account">

is right at the top of the patch ;-)

I seriously recommend reviewing all foo2.bar entity names. This is a hack when
all other chances fail, but not recommended coding style. Not that the
recommended coding style would be written up somewhere. It's Friday night and I'm on my way to bed, so I'll not get into a thorough review.

I didn't see dmose' string count from looking at attachment 387467 [details] [diff] [review], but those
are the strings inside content still. If it's really > 100 strings, I'd not
take them, in particular if this is not the latest beta.

I don't see what a patch including file moves would cause for l10n, i.e., what
strings get really added in the end, and how that compares to what localizers
might have in their repos already. I don't feel like figuring this out on a
Friday night either :-(

Round up, this is a big patch, apparently. It'd be great to see that getting
some l10n exposure. But not as the last thing in the beta.

I suggest to get some more l10n eyes on it, and land it early in the cycle for
the next Beta to get some nightly coverage.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Non-moving patch pushed:

<http://build.mozillamessaging.com/mercurial/comm-central/rev/291cbe3374b9>

Unfortunately, I didn't think to revise the commit message to get rid of "and
move them from content to locales" before pushing.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #30)
> So without a slipping by at least a week, I see no elegant way out of this
> mess. Either way, you will piss people off. 

So, in the case of going forward leaving the strings in content/, is it the end users that you feel will be pissed off?

The reason the strings are in content/ at all is because we deliberately cribbed the Firefox 3 dev cycle strategy of landing them in content/ first and only moving them to locales/ after we felt they were fairly stable.  This was done specifically because it was thought to be more important for the localizers who keep up with the trunk not to have to deal with string churn than for the nightly testers to be able to test localized features early in the cycle.

Do you think disagree with that strategy?

I'd also be interested to hear from Pike if that's still the strategy that Firefox is using, and if it's still considered to be the right tradeoff.

> All things considered, if there aren't any significant issues on the code side,
> which would warrant a slip as well, I would probably advise to release this
> thing and let the strings be hardecoded for this release.

That's pretty much my thinking.

> A possible strategy to at least partly mitigate this could be to split up the
> string files and the land the more user-visible strings in mail/locales now and
> land all the technical strings after beta3 is out. Really not an optimal
> solution, but the *only* thing that came to my mind to at least partly
> alleviate this.

My instinct is that this sounds like a bunch of work that doesn't really buy us a whole lot.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #31)
> TBH, I glanced at the patch, and I'd give it an r-.
> 
> <!ENTITY understood2.label               "Create Account">
> 
> is right at the top of the patch ;-)
>
> I seriously recommend reviewing all foo2.bar entity names. This is a hack when
> all other chances fail, but not recommended coding style. Not that the
> recommended coding style would be written up somewhere. It's Friday night and
> I'm on my way to bed, so I'll not get into a thorough review.

On the one hand, I should have caught that, since there's an obvious semantic mismatch.  On the other, I was doing a relatively quick review in the interest of not letting the perfect be the enemy of the good.  And I did actually go so far as to look at <https://developer.mozilla.org/en/Writing_localizable_code> while doing the review to act as a backstop for doing a quick review.

If you could just add a paragraph to that page with the appropriate best practice sometime soon (eg next month or so), I think Blake and I should be able to use that to go through and make appropriate adjustments, and we could avoid this problem in the future also.
 
> I don't see what a patch including file moves would cause for l10n, i.e., what
> strings get really added in the end, and how that compares to what localizers
> might have in their repos already. I don't feel like figuring this out on a
> Friday night either :-(

Executive summary: it's messy, since it would be a bunch of work (if possible) to figure out what they might have in their repos already from the first go-round.

> Round up, this is a big patch, apparently. It'd be great to see that getting
> some l10n exposure. But not as the last thing in the beta.
> 
> I suggest to get some more l10n eyes on it, and land it early in the cycle for
> the next Beta to get some nightly coverage.

Yep, sounds like we're all more or less on the same page about this.  Thanks for the quick feedback, guys!

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Pushing out to b4 for the remaining changes; thanks for the patch iterations, Blake!

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to move the files, and clean up the entity and property names.

A first cut at the patch to move the files, and clean up some of the entity/property names.

This shouldn't land until beta 4, which is why I haven't marked it r?, but any feedback on it before then would be greatly appreciated.

Thanks,
Blake.

Comment 37

[Simon Paquet [:sipaq]]

(In reply to comment #33)
> The reason the strings are in content/ at all is because we deliberately
> cribbed the Firefox 3 dev cycle strategy of landing them in content/ first 
> and only moving them to locales/ after we felt they were fairly stable. 
> This was done specifically because it was thought to be more important for 
> the localizers who keep up with the trunk not to have to deal with string 
> churn than for the nightly testers to be able to test localized features 
> early in the cycle.
> 
> Do you think disagree with that strategy?

I don't know what Firefox does nowadays, but I disagree with that strategy.

L10n teams who keep up with our checkins on a daily basis know what they are 
getting into and what they're up against. They have accepted this burden, so 
I see no need to continue with the current strategy, because we all have seen 
the downside of it here.

Comment 38

[Alex Ihrig [:Thunderbird_Mail_DE]]

(In reply to comment #37)
From a localizers point of view I'm confirming Simons statement and IMHO landing locale strings for this bug shouldn't be done as late-l10n-checkin (because of the large amount of strings).

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #37)
> 
> I don't know what Firefox does nowadays, but I disagree with that strategy.
> 
> L10n teams who keep up with our checkins on a daily basis know what they are 
> getting into and what they're up against. They have accepted this burden, 

There's definitely something to be said for that perspective.  My understanding was that this wasn't necessarily generally the case, but we could certainly take that stance.  I'll be interested to hear the history behind the Firefox decision here from Axel next week.

> so I see no need to continue with the current strategy, because we all have 
> seen the downside of it here.

What downside of content/ versus locales/ strategy have we seen here?  The downsides that I'm aware of are:

1) beta-testers get a partially, rather than completely, localized app.  In this bug, it's mostly been alluded to indirectly, so I'm not convinced that we've "seen that downside" here.  Furthermore, one of the most important reasons to use the word "beta" in describing a release is to communicate the idea of "not yet completed" very clearly to the users of that release.  Because of that, I'm not yet convinced that this is necessarily a problem at all.  If you or others think it is, it's not yet clear to me why.

2) fully localized code doesn't get tested until later in the cycle, meaning that there are fewer iterations over it.  In my mind, this is more likely to be a real problem, but as far as I can tell, the angst in this bug comes mostly or entirely from execution problems, not from the chosen strategy.  Do you disagree?

Is there some other downside that I've missed?

Comment 40

[Simon Paquet [:sipaq]]

(In reply to comment #39)

> 1) beta-testers get a partially, rather than completely, localized app. 
> In this bug, it's mostly been alluded to indirectly, so I'm not convinced 
> that we've "seen that downside" here.  Furthermore, one of the most 
> important reasons to use the word "beta" in describing a release is to 
> communicate the idea of "not yet completed" very clearly to the users of 
> that release.

That's correct. But historically Mozilla has put a lot of effort into ensuring 
that beta and even alpha releases are fully localized. So while this may be 
technically true, historically it hasn't been.

That we are even having that decision is a downside for me as well. Had we 
landed the strings in locales/ right away and potentially refined them later 
on, this discussion would never have been necessary.

> 2) fully localized code doesn't get tested until later in the cycle, 
> meaning that there are fewer iterations over it.  In my mind, this is more 
> likely to be a real problem, but as far as I can tell, the angst in this 
> bug comes mostly or entirely from execution problems, not from the chosen 
> strategy.  Do you disagree?
> 
> Is there some other downside that I've missed?

The most obvious downside that I see is that landing stuff in content/ and 
filing a followup bug to move it to locales/ later on has historically always 
lead to those followup bugs being overlooked or forgotten and this bug is a 
perfect example for this. It took us approx. 2.5 months to get to this bug 
after it was filed and over two months for the first unreviewed patch, which 
appeared in bugzilla only 4.5 hours before the slushy string freeze came into 
effect.

Let me make it clear that I'm not blaming anyone here. I guess it's just human 
nature to forget/postpone/overlook this stuff once it's been moved from one 
bug to another.

Comment 41

[Stefan Plewako [:stef]]

(In reply to comment #39)
> Is there some other downside that I've missed?

Wasting peoples time, many many hours.

Comment 42

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #37)
> I don't know what Firefox does nowadays, but I disagree with that strategy.

I am a Firefox localizer since 0.8 and, while indeed there were situations when some strings where in content for some period of time during development, there never was (or at least: I cannot recall) any beta released with non-localizable strings (apart from some pretty low-level layout error messages virtually nobody will ever see). 

This is different: we have a pretty big feature which cannot be localized in a beta release. 

There are two problems with stuff like this:
- if strings wait too long to get into l10n from content, there's less time to find *real* localizability issues (like formatting of various strings)
- local testers cannot test a feature in a language they don't understand (though, this is less of a problem, since most real testers do speak at least some English)

It is also unfortunate to all the localization teams that will be blamed for this by the users. It will be seen as localization team screw-up, which it is not.

Comment 43

[Axel Hecht [:Pike]]

I have just opened a thread on .l10n to discuss what happened in this bug.

Please copy your comment over there, and let's not sink that discussion in this bug, nor restrict the people involved just to those on this bug. Bugzilla isn't really the best way to have discussions either.

Comment 44

[Simon Paquet [:sipaq]]

As Pike suggested in the thread in mozilla.dev.l10n this should be added to the release notes.

Comment 45

[Vlado Valastiak [:wladow]]

Comment on attachment 388025 [details] [diff] [review]
A patch to move the files, and clean up the entity and property names.

OK, let's get back to this and land it the sooner the better in the cycle.

Two nits:
- accesskeys are completely missing from autoconfig, we should add them where appropriate
- in your last patch, the new ids when you're not altering a string, like:

>--- a/mail/base/content/accountCreation.dtd
>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
>@@ -1,5 +1,5 @@
>-<!ENTITY autoconfigWizard.style          "width: 640px; height: 480px;">
>+<!ENTITY autoconfig_wizard.style         "width: 640px; height: 480px;">

>-<!ENTITY accountInformation.label        "Account Information">
>+<!ENTITY account_information.label        "Account Information">
etc.

Is this really needed? Or what's the reason for a such change. I mean, we need to change an id when there's a semantic change to the string. And that's not this case. Moreover, previously used entity naming style (like accountInformation.label) is fairly OK and it is widely used in Mozilla trees, there's no need to add underscores as word delimiters.

Anyway, I can help here with the patch if needed.

Comment 46

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #45)
> - accesskeys are completely missing from autoconfig, we should add them where
> appropriate

Sounds like a good idea.  :)

> - in your last patch, the new ids when you're not altering a string, like:
> >--- a/mail/base/content/accountCreation.dtd
> >+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
> >@@ -1,5 +1,5 @@
> >-<!ENTITY autoconfigWizard.style          "width: 640px; height: 480px;">
> >+<!ENTITY autoconfig_wizard.style         "width: 640px; height: 480px;">
> 
> >-<!ENTITY accountInformation.label        "Account Information">
> >+<!ENTITY account_information.label        "Account Information">
> etc.
> 
> Is this really needed? Or what's the reason for a such change.

Is it really needed?  No, not really.

The reason I changed them is because the rest of the entity names in the file didn't have capital letters, and having a bunch of different conventions in the same file seemed more confusing than necessary, particularly for what I believe is fairly new code.

(If it was old code with a mixture of different styles that had evolved over years, then I wouldn't try to change them all to be the same, but for something that doesn't have that legacy, I would like to try to get it right, or at least get it consistent.  ;)

I _would_ be happy to change the identifiers to "abunchofwordsruntogether" to match the examples of receivingserver, sendingserver, noencryption, createinadvanced, rememberpassword, technicaldetails, etc…  But only if you think that would be a better convention to follow.

> Moreover, previously used entity naming style (like
> accountInformation.label) is fairly OK and it is widely used in Mozilla trees,
> there's no need to add underscores as word delimiters.

Hmm...  Perhaps it would be best to make all the entity names camelCase, instead?

> Anyway, I can help here with the patch if needed.

That would be awesome!  I'm in #maildev on irc between 9:00 and 17:00 EST (13:00 and 21:00 GMT), if you wanted to chat about the patch in realtime.

Thanks,
Blake.

Comment 47

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to move the files, and clean up the entity and property names, with wladow's fixes.

The other fix I made was to display the same message for cleartext outgoing connections as we did for cleartext incoming connections.  The icon was orange, but we didn't hook up the message for some reason.

Thanks,
Blake.

Comment 48

[Vlado Valastiak [:wladow]]

Comment on attachment 390920 [details] [diff] [review]
A patch to move the files, and clean up the entity and property names, with wladow's fixes.

>+<!ENTITY rememberPassword.label          "Remember password">
>+<!ENTITY rememberPassword.accesskey      "p">

Ah, I missed this. Lowercase "p" isn't that good choise, because it's less readable when underlined. I suggest to use "m" instead.

Comment 49

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #48)
> >+<!ENTITY rememberPassword.accesskey      "p">
> Ah, I missed this. Lowercase "p" isn't that good choise, because it's less
> readable when underlined. I suggest to use "m" instead.

I've fixed this in my local copy, so the next diff I post should have the change.

Thanks,
Blake.

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next rev of the patch.

While working on Bug 503436, I ran into the missing accesskeys for the warning page, so I thought I would add them and upload a new patch with Vlado's change.

Comment 51

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next rev of the patch, with better accesskeys.

I'm also switching the r? to philor, because dmose is on holiday.

Thanks,
Blake.

Comment 52

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 391114 [details] [diff] [review]
The next rev of the patch, with better accesskeys.

Philor's gone until the 3rd, so it's over to bienvenu.

Comment 53

[David :Bienvenu]

Comment on attachment 391114 [details] [diff] [review]
The next rev of the patch, with better accesskeys.

personally, I think "Create in Advanced Editor" could be more accurate - something like "Create and Edit Account Settings" would give the user a clue that they'll get the same screen that they would in tools | account settings. What do you think, Bryan?

The customFields stuff isn't used yet, right? If so, they can just go:

+  <vbox id="contactYcustomfields-box" hidden="true" flex="1">
+    <description class="header">&customFields-header.label;</description>
+    <description class="intro">&customFields-intro1.descr;</description>
+    <description class="intro">&customFields-intro2.descr;</description>

and a couple others...

It would be nice if <ret> worked to get from the first screen to the second, once "Next" is enabled.

Otherwise, this looks pretty good, but I'll let Bryan do his ui review first...

Comment 54

[Simon Paquet [:sipaq]]

(In reply to comment #53)
> The customFields stuff isn't used yet, right? If so, they can just go:
> 
> +  <vbox id="contactYcustomfields-box" hidden="true" flex="1">
> +    <description class="header">&customFields-header.label;</description>
> +    <description class="intro">&customFields-intro1.descr;</description>
> +    <description class="intro">&customFields-intro2.descr;</description>
> 
> and a couple others...

Blake, just as a heads-up for you:
You can find unused entities in your patches with the script I used in bug 461112 comment 1. See also bug 461112 comment 4 for more information on the script syntax.

Comment 55

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, updated with Bienvenu's suggestions.

(In reply to comment #53)
> (From update of attachment 391114 [details] [diff] [review])
> I think "Create in Advanced Editor" could be "Create and Edit Account
> Settings"  What do you think, Bryan?

I don't know about Bryan, but I like it.  Changed.
(If Bryan disagrees, I'll change it back.)

> The customFields stuff isn't used yet, right? If so, they can just go:

Removed.

> It would be nice if <ret> worked to get from the first screen to the second,
> once "Next" is enabled.

Fixed.  It now also works on the second screen to create the account.
The code was there, but wasn't quite hooked up.

> Otherwise, this looks pretty good, but I'll let Bryan do his ui review
> first...

Since Bryan hasn't gotten to the ui-review yet, here's a new patch with the changes you requested.

Thanks,
Blake.

Comment 56

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 391779 [details] [diff] [review]
The previous patch, updated with Bienvenu's suggestions.

That sounds fine to me, I am a little worried about the translations bring really long. Perhaps we should provide our alternatives in a comment; I'll let Simon weigh in on that

Comment 57

[Simon Paquet [:sipaq]]

Localizers are already free to not literally translate everything. So I don't think we should add additional comments here.

Comment 58

[David :Bienvenu]

Comment on attachment 391779 [details] [diff] [review]
The previous patch, updated with Bienvenu's suggestions.

cool, thx, Blake.

Comment 59

[David :Bienvenu]

Comment on attachment 391779 [details] [diff] [review]
The previous patch, updated with Bienvenu's suggestions.

one nit - period at end of this string:

+please_enter_missing_hostnames=Could not guess settings — please enter missing hostnames

Comment 60

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked in] The previous patch, with a fixed nit.

(In reply to comment #59)
> one nit - period at end of this string:
> +please_enter_missing_hostnames=Could not guess settings — please enter missing
> hostnames

Fixed.

Thanks,
Blake.

Comment 61

[Mark Banner (:standard8)]

Comment on attachment 392255 [details] [diff] [review]
[checked in] The previous patch, with a fixed nit.

Checked in: http://hg.mozilla.org/comm-central/rev/cdba7c9bbb72

Comment 62

[Mark Banner (:standard8)]

This patch has now landed.

There may be some further changes as we re-layout the autoconfig dialog, though we expect those to be minimal (Simon can you communicate that on the lists?).

Please file any further issues in follow-up bugs.

Comment 64

[Simon Paquet [:sipaq]]

(In reply to comment #62)
> There may be some further changes as we re-layout the autoconfig dialog, though
> we expect those to be minimal (Simon can you communicate that on the lists?).

Done. http://thunderbird-l10n.blogspot.com/2009/08/autoconfiguration-strings-have-landed.html