893961 - [MP] Defect - Clean up metro sync strings

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Sync, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 (WIP)

(In reply to Francesco Lodolo [:flod] from comment #40)
> Asking here about strings before opening follow-ups (I got lost in the
> comments)
> 
> Is there a specific reason for mixing Sync (hard-coded) and
> &syncBrand.shortName.label;?

I used "Sync" (hard-coded) when describing a part of the UI on another platform (desktop and Android Firefox).  I suppose it makes more sense to be consistent, so I've changed them to "&syncBrand.shortName.label;" in the attached patch.

> <!ENTITY sync.flyout.manualsetup.description1 "Please enter your
> &syncBrand.shortName.label; account information and the Recover Key
> generated by your computer">
> Typo: it's RecoverY Key, not Recover Key.

Fixed in the attached patch.

> <!ENTITY sync.flyout.setup.manual.label "I&apos;m not near my computer...">
> Should use "…" (single unicode character) instead of "..."

Fixed in the attached patch.

> <!ENTITY sync.flyout.pairNewDevice.note3 "[Desktop] Tools -&gt; Set Up Sync
> -&gt; I have an Account">
> Why "Tools"? This is valid only for a small subset of Windows.

Also most Windows users probably don't realize how to get the toolbar to appear.  Maybe this string should indicate how to get to this menu option through the "Firefox" button in the upper-left corner.

Yuan: Any thoughts on this?

> <!ENTITY sync.flyout.setup.description5 "[Desktop] Preferences -&gt; Sync">
> Again. Preferences is good for Linux and Mac, not Windows. Maybe "Settings"
> (generic, os-agnostic)?

This string is meant to reflect the UI on other devices, so I think we should choose words that actually appear in those UIs.  Maybe we should have different instructions for "Desktop" on each operating system?  Or maybe just specify "Windows Desktop?"

Yuan: Can you coordinate with Francesco to figure out what strings make sense here?

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Yuan: Ping. I just want to make sure this didn't fall off the radar.

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #0)

> > <!ENTITY sync.flyout.pairNewDevice.note3 "[Desktop] Tools -&gt; Set Up Sync
> > -&gt; I have an Account">
> > Why "Tools"? This is valid only for a small subset of Windows.
> 
> Also most Windows users probably don't realize how to get the toolbar to
> appear.  Maybe this string should indicate how to get to this menu option
> through the "Firefox" button in the upper-left corner.
> 
> Yuan: Any thoughts on this?
I don't think we should use "Tools" here. It's mainly for Mac. 
I was trying to find a help article of opening preferences panel in different OS. But there wasn't one. Besides, Australis will change the menu panel completely. I would suggest not to specify the term here. 


How about this?

Note:

To find the code,
[Desktop] Open "Options" or "Preferences" panel on the Firefox menu bar, then follow the steps: Sync -> Set up Sync -> I have an account
[Android] More -> Settings -> Sync 


> 
> > <!ENTITY sync.flyout.setup.description5 "[Desktop] Preferences -&gt; Sync">
> > Again. Preferences is good for Linux and Mac, not Windows. Maybe "Settings"
> > (generic, os-agnostic)?
> 
> This string is meant to reflect the UI on other devices, so I think we
> should choose words that actually appear in those UIs.  Maybe we should have
> different instructions for "Desktop" on each operating system?  Or maybe
> just specify "Windows Desktop?"
> 
> Yuan: Can you coordinate with Francesco to figure out what strings make
> sense here?


How about this?

Note:
To find "Pair a Device",
[Desktop] Open "Options" or "Preferences" panel on the Firefox menu bar, then open the "Sync" panel
[Android] More -> Settings ->Sync

Comment 3

[Francesco Lodolo [:flod]]

Updates on this bug?

Since this bug is about Sync, on dev-l10n someone noticed another hard-coded Sync in browser.properties

http://hg.mozilla.org/mozilla-central/file/74fe1012de43/browser/metro/locales/en-US/chrome/browser.properties#l49
syncCharm=Sync

As a reference, similar strings in Firefox use a replaced variable for &syncBrandShortName;
http://hg.mozilla.org/mozilla-central/file/a71cedddadd1/browser/locales/en-US/chrome/browser/browser.properties#l379

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

(In reply to Francesco Lodolo [:flod] from comment #3)
> Updates on this bug?

I thought Yuan's questions in comment 2 were directed at you, flod. I've updated the patch to include the string changes in comment 2. Let us know if those look OK to you.

> Since this bug is about Sync, on dev-l10n someone noticed another hard-coded
> Sync in browser.properties
> 
> http://hg.mozilla.org/mozilla-central/file/74fe1012de43/browser/metro/
> locales/en-US/chrome/browser.properties#l49
> syncCharm=Sync

I updated the patch to make this line of code reference the syncBrandShortName available in brand.properties.

Comment 5

[Francesco Lodolo [:flod]]

Comment on attachment 792448 [details] [diff] [review]
Patch v2

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

This won't work: at this point in the process (localization enabled on l10n-central) you can't update strings without changing their names.

And you need someone from UI to check the strings, more than a localizer, that's why I didn't answer comment 2 ;-)

::: browser/metro/base/content/browser-ui.js
@@ +1308,5 @@
>          onselected: function() FlyoutPanelsUI.show('PrefsFlyoutPanel')
>      });
>      // Sync
>      this.addEntry({
> +        label: Strings.brand.GetStringFromName("syncBrandShortName"),

Will this work? Where is syncBrandShortName defined?

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ +17,3 @@
>  <!ENTITY sync.flyout.setup.description3        "Note:">
> +<!ENTITY sync.flyout.setup.description4        "To find &quot;Pair a Device&quot;,">
> +<!ENTITY sync.flyout.setup.description5        "[Desktop] Open &quot;Options&quot; or &quot;Preferences&quot; panel on the &brandShortName; menu bar, then open the &quot;&syncBrand.shortName.label;&quot; panel">

Options and Preferences are menus, not panels (same in pairNewDevice.note2).

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

(In reply to Francesco Lodolo [:flod] from comment #5)
> This won't work: at this point in the process (localization enabled on
> l10n-central) you can't update strings without changing their names.

Updated the names of all changed strings in the attached patch.

> And you need someone from UI to check the strings, more than a localizer,
> that's why I didn't answer comment 2 ;-)

Requesting feedback from :flod and :yuan

> ::: browser/metro/base/content/browser-ui.js
> @@ +1308,5 @@
> >          onselected: function() FlyoutPanelsUI.show('PrefsFlyoutPanel')
> >      });
> >      // Sync
> >      this.addEntry({
> > +        label: Strings.brand.GetStringFromName("syncBrandShortName"),
> 
> Will this work? Where is syncBrandShortName defined?

It's defined in brand.properties. This worked when I tested it locally, and I can't think of a reason for it to break elsewhere.

> ::: browser/metro/locales/en-US/chrome/sync.dtd
> @@ +17,3 @@
> >  <!ENTITY sync.flyout.setup.description3        "Note:">
> > +<!ENTITY sync.flyout.setup.description4        "To find &quot;Pair a Device&quot;,">
> > +<!ENTITY sync.flyout.setup.description5        "[Desktop] Open &quot;Options&quot; or &quot;Preferences&quot; panel on the &brandShortName; menu bar, then open the &quot;&syncBrand.shortName.label;&quot; panel">
> 
> Options and Preferences are menus, not panels (same in pairNewDevice.note2).

Fixed in the attached patch. Yuan, please verify that we're OK with these strings from a UX standpoint.

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 792644 [details] [diff] [review]
Patch v3

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

About string versioning
sync.flyout.setup.manual.label.2 -> sync.flyout.setup.manual.label2
sync.flyout.setup.description4.2 -> sync.flyout.setup.description5
etc.


sync.flyout.setup.description5.2 "[Desktop] Open "Options" or "Preferences" menu on the &brandShortName; menu bar, then open the "&syncBrand.shortName.label;" menu"

Not sure the "on the &brandShortName; menu bar" is really needed (also, Linux and Windows don't really have a menu bar anymore), Sync is a panel in settings. But again, that's something ux should check.

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Francesco Lodolo [:flod] from comment #7)
> About string versioning
> sync.flyout.setup.manual.label.2 -> sync.flyout.setup.manual.label2
> sync.flyout.setup.description4.2 -> sync.flyout.setup.description5
> etc.

That doesn't work with the naming scheme in this file: sync.flyout.setup.description1 through sync.flyout.setup.description5 are named such because they are all part of the same block of text. Changing sync.flyout.setup.description4 to sync.flyout.setup.description5 will mean that there are two strings called sync.flyout.setup.description5.

It sounds like we don't want to add a version after a '.' in these string names. Is there a preferable way to version the strings while maintaining the information that these strings are part of the same visual block of text?

One possibility would be to do this:
  sync.flyout.setup.description4 -> sync.flyout.setup.description4v2


> sync.flyout.setup.description5.2 "[Desktop] Open "Options" or
> "Preferences" menu on the &brandShortName; menu bar, then open the
> "&syncBrand.shortName.label;" menu"
> 
> Not sure the "on the &brandShortName; menu bar" is really needed (also,
> Linux and Windows don't really have a menu bar anymore), Sync is a panel in
> settings. But again, that's something ux should check.

Yuan: Can you address this?

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 792644 [details] [diff] [review]
Patch v3

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

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ +17,3 @@
>  <!ENTITY sync.flyout.setup.description3        "Note:">
> +<!ENTITY sync.flyout.setup.description4.2      "To find &quot;Pair a Device&quot;,">
> +<!ENTITY sync.flyout.setup.description5.2      "[Desktop] Open &quot;Options&quot; or &quot;Preferences&quot; menu on the &brandShortName; menu bar, then open the &quot;&syncBrand.shortName.label;&quot; menu">

I agree with flod -- "menu" is still not quite right here.  (Technically it's the "Options" item in the "Tools" or "Firefox" menu, or the "Preferences" item in the "Edit" menu...)  I'd say something like:

Open the &brandShortName; "Options" or "Preferences" window, then select the "&syncBrand.shortName.label;" panel (or "tab"?).

Comment 10

[Francesco Lodolo [:flod]]

> That doesn't work with the naming scheme in this file:
> sync.flyout.setup.description1 through sync.flyout.setup.description5 are
> named such because they are all part of the same block of text. Changing
> sync.flyout.setup.description4 to sync.flyout.setup.description5 will mean
> that there are two strings called sync.flyout.setup.description5.

Sorry, I didn't realize that from the patch, I thought the numbers were from previous iterations (and versioning).

Being that the case, I think .X is quite confusing but correct.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

Carrying forward r+

Spoke on IRC with :flod - although it's confusing, it's probably most correct to name/version the strings in this file as, e.g., sync.blah.description2_3. In this example, the string is the second part of a multi-string block of text, and this is the third revision of the string.

Also updated the 'menu' strings to remove the word 'menu' entirely

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 792986 [details] [diff] [review]
Patch v4

:yuan, :flod, and :mbrubeck -
  I think the one remaining piece of this bug is what to write in the instructions for desktop.

Right now, we have

<!ENTITY sync.flyout.setup.description5_2     "[Desktop] Open &quot;Options&quot; or &quot;Preferences&quot; and then open &quot;&syncBrand.shortName.label;&quot;">

and

<!ENTITY sync.flyout.pairNewDevice.note3_2    "[Desktop] Open &quot;Options&quot; or &quot;Preferences&quot; and then follow the steps: &syncBrand.shortName.label; -&gt; Set up &syncBrand.shortName.label; -&gt; I have an account">

Please post your thoughts/comments

Comment 13

[Francesco Lodolo [:flod]]

I'm not fond of "Open X and then open Y" in the first string, maybe the second open -> select. For the rest I think it's clear.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5

(In reply to Francesco Lodolo [:flod] from comment #13)
> I'm not fond of "Open X and then open Y" in the first string, maybe the
> second open -> select. For the rest I think it's clear.

Done.

If anyone has any further comments, please let me know.

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v6

Forgot to qref previously. This patch contains the open->select change

Comment 16

[Marco Mucci [:MarcoM]]

Hey Tim, can you provide a point value for this defect.

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

p=1

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/integration/fx-team/rev/6471b1c0aa26

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6471b1c0aa26