652483 - Sync UI: When printing or saving a sync key, the dialog prompts you to change it

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

In sync preferences, when you go to "Manage account > my sync key" the resulting dialog box says "Change your Sync Key."  It's possible that the user wanted to change it, although it is equally possibly that they simply wanted to view it to back up a copy.  Prompting them to change their key might actually cause them to change it needless.

Additionally, we no longer allow user modified sync keys, to ensure the best level of encryption.

Suggested changes:
-Change dialog title to "My Sync Key"
-Change "Generate" link to "Generate a new key"

Comment 1

[Marina Samuel [:emtwo]]

Attachment: patch for bug 652483

Comment 2

[Marina Samuel [:emtwo]]

Attachment: screenshot of new recommended string values

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 529013 [details] [diff] [review]
patch for bug 652483

This isn't a complete fix, if my understanding is correct. (It might not be -- I've had a couple of cocktails so far this evening!)

This approach requires localization; you're changing the string for "Change your Sync Key" in en-US only. We have something like 88 locales, all told.

You might be better off changing the XUL to refer to mySyncKey.label, which is the English string "My Sync Key" (granted, it's "my" not "your"), and eliminating the change.synckey entity altogether. (That might require another bug to follow up with localizers for cleanup, though. I'm not too familiar with the process there.)

That said, you need to change syncKeyGenerate.label, so you're going to hit the localization issue anyway.

My suggestion is to find out who can give you a firm answer about whether this has localization impact. If it doesn't, you get r+ from me (though I don't like "change.synckey.title" not referring to "change"!). If it does, then we need to make sure we coordinate localization work, and decide whether anything else needs to happen.

I'll leave a channel-hanger to see if someone can weigh in on this.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> This isn't a complete fix, if my understanding is correct. (It might not be --
> I've had a couple of cocktails so far this evening!)

Functionally it's complete, at least as far as I understand Alex in comment 0. However, technically the patch is not sufficient. Strings may never be changed while keeping the same string ID. If a string change is required, a new string ID has to be chosen (if no good new name suggests itself, one can simply add a "2").

This means you want to replace 'change.synckey.title' with 'mySyncKey.title' and 'syncKeyGenerate.label' with 'generateNewSyncKey.label' or something similar.

> This approach requires localization; you're changing the string for "Change
> your Sync Key" in en-US only. We have something like 88 locales, all told.

This sounds like you want to make Marina do the localization for all of our 80 something locales ;). m-c (and therefore s-c) are en-US only, nobody has to ever worry about translations when making changes there. We're not string frozen, so string changes are perfectly fine.

Comment 5

[Marina Samuel [:emtwo]]

(In reply to comment #4) 
> This means you want to replace 'change.synckey.title' with 'mySyncKey.title'
> and 'syncKeyGenerate.label' with 'generateNewSyncKey.label' or something
> similar.

Is it ok to make these replacements within the same file?  The file syncGenericChange.properties seems to contain mostly strings starting with 'change.*' the string 'mySyncKey.title' doesn't seem to fit in well with the rest then.  The 'generateNewSyncKey.label' seems ok within the same file to me though.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> Is it ok to make these replacements within the same file?

Yup.

> The file
> syncGenericChange.properties seems to contain mostly strings starting with
> 'change.*' the string 'mySyncKey.title' doesn't seem to fit in well with the
> rest then.  The 'generateNewSyncKey.label' seems ok within the same file to me
> though.

The string IDs are made up and have no special meaning. But you do have a good point about maintaining consistency regarding the change.* prefix. You could make it change.mySyncKey.title or change.synckey2.title or something else. Up to you, really :)

Comment 7

[Marina Samuel [:emtwo]]

Attachment: patch for bug 652483

- Kept original string constants & added new ones in the .properties & .dtd files
- Changed the constants being referenced in the .js & .xul files

is this better?

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 529140 [details] [diff] [review]
patch for bug 652483

Yup that's better, but you can get rid of the old strings, unless they're still being used somewhere. I doubt it, but please double check, though.

Comment 9

[Marina Samuel [:emtwo]]

Attachment: patch for bug 652483

removed the unused strings

Comment 10

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/services-central/rev/f3eba0269bb2

Comment 11

[Tracy Walker [:tracy]]

Verified with s-c builds of 20110502

Comment 12

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/f3eba0269bb2