Closed Bug 893961 Opened 6 years ago Closed 6 years ago

[MP] Defect - Clean up metro sync strings

Categories

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

All
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

References

Details

(Whiteboard: [l10n] [preview] feature=defect c=tbd u=tbd p=1)

Attachments

(1 file, 5 obsolete files)

Attached patch Patch v1 (WIP) (obsolete) — Splinter Review
(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?
Flags: needinfo?(ywang)
Yuan: Ping. I just want to make sure this didn't fall off the radar.
(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
Flags: needinfo?(ywang)
Whiteboard: [preview-triage]
Summary: Clean up metro sync strings → Defect - Clean up metro sync strings
Whiteboard: [preview-triage] → [preview-triage] feature=defect c=tbd u=tbd p=0
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
Flags: needinfo?(tabraldes)
Attached patch Patch v2 (obsolete) — Splinter Review
(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.
Attachment #775835 - Attachment is obsolete: true
Flags: needinfo?(tabraldes)
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).
Attachment #792448 - Flags: feedback-
Attached patch Patch v3 (obsolete) — Splinter Review
(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.
Attachment #792448 - Attachment is obsolete: true
Attachment #792644 - Flags: review?(mbrubeck)
Attachment #792644 - Flags: feedback?(ywang)
Attachment #792644 - Flags: feedback?(francesco.lodolo)
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 &quot;Options&quot; or &quot;Preferences&quot; menu on the &brandShortName; menu bar, then open the &quot;&syncBrand.shortName.label;&quot; 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.
Attachment #792644 - Flags: feedback?(francesco.lodolo) → feedback-
(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 &quot;Options&quot; or
> &quot;Preferences&quot; menu on the &brandShortName; menu bar, then open the
> &quot;&syncBrand.shortName.label;&quot; 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 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"?).
Attachment #792644 - Flags: review?(mbrubeck) → review+
> 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.
Attached patch Patch v4 (obsolete) — Splinter Review
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
Attachment #792644 - Attachment is obsolete: true
Attachment #792644 - Flags: feedback?(ywang)
Attachment #792986 - Flags: review+
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
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.
Attached patch Patch v5 (obsolete) — Splinter Review
(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.
Attachment #792986 - Attachment is obsolete: true
Attachment #793841 - Flags: review+
Attached patch Patch v6Splinter Review
Forgot to qref previously. This patch contains the open->select change
Attachment #793841 - Attachment is obsolete: true
Attachment #793842 - Flags: review+
Hey Tim, can you provide a point value for this defect.
Flags: needinfo?(tabraldes)
Priority: -- → P2
QA Contact: jbecerra
Summary: Defect - Clean up metro sync strings → [MP] Defect - Clean up metro sync strings
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=0
p=1
Flags: needinfo?(tabraldes)
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=1
Blocks: metrov1it14
No longer blocks: metrov1it13
Whiteboard: [preview] feature=defect c=tbd u=tbd p=1 → [l10n] [preview] feature=defect c=tbd u=tbd p=1
https://hg.mozilla.org/mozilla-central/rev/6471b1c0aa26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.