1019719 - Shorten and localize default client device names for Android Sync

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks two things: revisiting our choice of default device names, and localizing the string we generate (see [1]).  At the moment, we end up with sub-optimal device names like "Firefox Aurora on GT-I1950".  The latter part is *supposed* to be human-readable -- per Android -- but it's often not.  antlam suggested just calling these all "Firefox on ... " or even "Firefox on my ..." rather than baking in the release channel; I'm less thrilled with this idea (as a tester) but we could do it.

[1] https://github.com/mozilla-services/android-sync/commit/149eed65484dfa6584d87cd9b53472de40e1a387#src-main-java-org-mozilla-gecko-sync-sharedpreferencesclientsdatadelegate-java-P20

Comment 1

[Nick Alexander :nalexander [he/him]]

I'm happy to mentor this bug.  It's in the android-sync repo, so it's a little different than most bugs, but it's a good first-Android Sync bug.

What needs to happen is that, in SharedPreferencesClientsDataDelegate.java, rather than building the string directly from an English fragment, we use an Android resource to interpolate the product name and the current Android model identifier into a translated string.

Comment 2

[Richard Newman [:rnewman]]

See also Bug 679272, Bug 838838.

> antlam suggested just calling these all "Firefox on ... " or
> even "Firefox on my ..." rather than baking in the release channel; I'm less
> thrilled with this idea (as a tester) but we could do it.

Is "Beta on..." or "Aurora on..." an option? For users who cross between Beta and Release, "Firefox" would be actively misleading.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

via IRC.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This bug is a bit different from others in that we're working out of the Android Sync repository [1], whose patches get ported into mozilla-central. Since this process is a little complicated, for this patch, you can work out of mozilla-central, and we'll take care of backporting it into android-sync.

That being said, the resource files we use for Android sync are distinct from those used in mozilla-central. For example, we'll be working with mobile/android/base/locales/en-US/sync_strings.dtd, rather than mobile/android/base/locales/en-US/android_strings.dtd.

[1]: https://github.com/mozilla-services/android-sync

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Richard Newman [:rnewman] from comment #2)
> See also Bug 679272, Bug 838838.
> 
> > antlam suggested just calling these all "Firefox on ... " or
> > even "Firefox on my ..." rather than baking in the release channel; I'm less
> > thrilled with this idea (as a tester) but we could do it.
> 
> Is "Beta on..." or "Aurora on..." an option? For users who cross between
> Beta and Release, "Firefox" would be actively misleading.

+1

Comment 6

[Bryan Kok [:transfusion]]

Attachment: bug-1019779.patch

Here is my first approach.
Is there a neater way to get just the string of the release channel instead of checking if "Firefox" is in MOZ_APP_DISPLAYNAME and omitting that out?

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8454674 [details] [diff] [review]
bug-1019779.patch

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

Looking pretty good!

When you post a patch, you should flag a reviewer, who is typically one of your mentors. You can select a reviewer by entering their email address in the r? field. Note that the box should auto-complete so if you don't know their email address, you can use their name, or their IRC nick (which typically auto-completes better) by prepending with a colon. For example, the best way to flag me as a reviewer is probably by typing some of, ":mcomella".  

(In reply to Bryan Kok [:transfusion] from comment #6)
> Is there a neater way to get just the string of the release channel instead
> of checking if "Firefox" is in MOZ_APP_DISPLAYNAME and omitting that out?

I'm actually not sure what you mean here - where do you do this?

::: mobile/android/base/locales/en-US/sync_strings.dtd
@@ +73,5 @@
>  
>  <!-- Account strings -->
>  <!ENTITY sync.account.label '&syncBrand.fullName.label; (deprecated)'>
>  
> +<!-- Default client name; &formatS1; is GlobalConstants.MOZ_APP_DISPLAYNAME, whereas &formatS2; is android.os.Build.MODEL -->

Our localizers likely won't be familiar with Android constants, nor technical terms - you should describe what these constants do. For an example, see [1].

You should also example what a "default client" is.

Localization notes should also be prefaced with "Localization note:" - see [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd?rev=087013249cb7#469

::: mobile/android/base/sync/SharedPreferencesClientsDataDelegate.java
@@ +48,5 @@
>    }
>  
>    @Override
>    public String getDefaultClientName() {
>      // Bug 1019719: localize this string!

Remove this comment.

::: mobile/android/services/strings.xml.in
@@ +98,5 @@
>    <string name="sync_text_tab_sent">&sync.text.tab.sent.label;</string>
>    <string name="sync_text_tab_not_sent">&sync.text.tab.not.sent.label;</string>
>  
> +  <!-- Default Client name. -->
> +  <string name="sync_defaultclientname">&sync.defaultclientname;</string>

The prefix `sync_*` is used for our old syncing system, which we only maintain now. This change affects Firefox Accounts, our new system, and so should have the `fxaccount_*` prefix. Don't forget to update sync_strings.dtd, and move the values into the "Firefox Account strings" section.

The string name should probably have underscores between words: i.e. "sync_default_client_name".

Don't forget to update sync_strings.dtd!

I don't think that the default client name comment is necessary as it doesn't add much over the string name.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #7)
> Comment on attachment 8454674 [details] [diff] [review]
> bug-1019779.patch
> 
> Review of attachment 8454674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking pretty good!
> 
> When you post a patch, you should flag a reviewer, who is typically one of
> your mentors. You can select a reviewer by entering their email address in
> the r? field. Note that the box should auto-complete so if you don't know
> their email address, you can use their name, or their IRC nick (which
> typically auto-completes better) by prepending with a colon. For example,
> the best way to flag me as a reviewer is probably by typing some of,
> ":mcomella".  
> 
> (In reply to Bryan Kok [:transfusion] from comment #6)
> > Is there a neater way to get just the string of the release channel instead
> > of checking if "Firefox" is in MOZ_APP_DISPLAYNAME and omitting that out?
> 
> I'm actually not sure what you mean here - where do you do this?

Bryan is referring to https://bugzilla.mozilla.org/show_bug.cgi?id=1019719#c2, I think.  Bryan:

I'd just do a few targeted searches:

String name = MOZ_APP_DISPLAY_NAME;
if (name.contains("Firefox Aurora")) {
  name = "Aurora";
} ...

and so on.

> ::: mobile/android/base/locales/en-US/sync_strings.dtd
> @@ +73,5 @@
> >  
> >  <!-- Account strings -->
> >  <!ENTITY sync.account.label '&syncBrand.fullName.label; (deprecated)'>
> >  
> > +<!-- Default client name; &formatS1; is GlobalConstants.MOZ_APP_DISPLAYNAME, whereas &formatS2; is android.os.Build.MODEL -->
> 
> Our localizers likely won't be familiar with Android constants, nor
> technical terms - you should describe what these constants do. For an
> example, see [1].
> 
> You should also example what a "default client" is.
> 
> Localization notes should also be prefaced with "Localization note:" - see
> [1].
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/
> en-US/android_strings.dtd?rev=087013249cb7#469
> 
> ::: mobile/android/base/sync/SharedPreferencesClientsDataDelegate.java
> @@ +48,5 @@
> >    }
> >  
> >    @Override
> >    public String getDefaultClientName() {
> >      // Bug 1019719: localize this string!
> 
> Remove this comment.
> 
> ::: mobile/android/services/strings.xml.in
> @@ +98,5 @@
> >    <string name="sync_text_tab_sent">&sync.text.tab.sent.label;</string>
> >    <string name="sync_text_tab_not_sent">&sync.text.tab.not.sent.label;</string>
> >  
> > +  <!-- Default Client name. -->
> > +  <string name="sync_defaultclientname">&sync.defaultclientname;</string>
> 
> The prefix `sync_*` is used for our old syncing system, which we only
> maintain now. This change affects Firefox Accounts, our new system, and so
> should have the `fxaccount_*` prefix. Don't forget to update
> sync_strings.dtd, and move the values into the "Firefox Account strings"
> section.

Actually, it will impact both, because the SPClientsDataDelegate is used by both systems.  So "sync_default_client_name" is good.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #8)
> Bryan is referring to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1019719#c2, I think.  Bryan:
> 
> I'd just do a few targeted searches:
> 
> String name = MOZ_APP_DISPLAY_NAME;
> if (name.contains("Firefox Aurora")) {
>   name = "Aurora";
> } ...
> 
> and so on.

Are the app display names ever localized? If so, this likely would not work.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #9)
> (In reply to Nick Alexander :nalexander from comment #8)
> > Bryan is referring to
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1019719#c2, I think.  Bryan:
> > 
> > I'd just do a few targeted searches:
> > 
> > String name = MOZ_APP_DISPLAY_NAME;
> > if (name.contains("Firefox Aurora")) {
> >   name = "Aurora";
> > } ...
> > 
> > and so on.
> 
> Are the app display names ever localized? If so, this likely would not work.

This is a good point, and I should have mentioned it.  They are not, and are unlikely to be localized in future; for example, see:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/aurora/configure.sh

Comment 11

[Bryan Kok [:transfusion]]

Attachment: bug-1019779.patch

Fixed string names, added a localization note, and added the targeted search for the release channel in MOZ_APP_DISPLAY_NAME.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8456943 [details] [diff] [review]
bug-1019779.patch

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

Looking pretty good, mostly nits.  Next patch, update the commit message to say what your patch does:

"Bug 1019719 - Localize the Sync client name. r=nalexander"

::: mobile/android/base/locales/en-US/sync_strings.dtd
@@ +74,5 @@
>  <!-- Account strings -->
>  <!ENTITY sync.account.label '&syncBrand.fullName.label; (deprecated)'>
>  
> +<!-- Localization note (sync.default.client.name): Default string of the "Device name" menu item upon setting up Firefox Sync.
> +     The placeholder &formatS1 will be replaced by the name of the Firefox release channel and &formatS2 by the model name of the 

nit: trailing whitespace.

@@ +75,5 @@
>  <!ENTITY sync.account.label '&syncBrand.fullName.label; (deprecated)'>
>  
> +<!-- Localization note (sync.default.client.name): Default string of the "Device name" menu item upon setting up Firefox Sync.
> +     The placeholder &formatS1 will be replaced by the name of the Firefox release channel and &formatS2 by the model name of the 
> +     Android device. Example:

nit: examples.

::: mobile/android/base/sync/SharedPreferencesClientsDataDelegate.java
@@ +5,5 @@
>  package org.mozilla.gecko.sync;
>  
>  import org.mozilla.gecko.background.common.GlobalConstants;
>  import org.mozilla.gecko.sync.delegates.ClientsDataDelegate;
> +import android.content.Context;

nit: move this above content.SharedPreferences (alphabetical order, keeping the grouping).

@@ +16,5 @@
>   * <code>SharedPreferences</code> instance.
>   */
>  public class SharedPreferencesClientsDataDelegate implements ClientsDataDelegate {
>    protected final SharedPreferences sharedPreferences;
> +  protected final Context context;

nit: blank line before constructor.

@@ +51,5 @@
>    public String getDefaultClientName() {
> +    String name = GlobalConstants.MOZ_APP_DISPLAYNAME;
> +    if (name.contains("Aurora")) {
> +        name = "Aurora";
> +    }

nit: we brace the else on the same line, so:

if (name.contains("Aurora")) {
    name = "Aurora";
} else if (name.contains("Beta")) {
    name = "Beta";
} ...

@@ +58,5 @@
> +    }
> +    else if (name.contains("Nightly")){
> +        name = "Nightly";
> +    }
> +    else if (name.contains("Fennec")) {

This is complicated: there's "Fennec nalexander" in development and "Fennec" for Nightly.  Drop this clause entirely; with the default of MOZ_APP_DISPLAYNAME, you should get the right thing.

@@ +62,5 @@
> +    else if (name.contains("Fennec")) {
> +        name = "Fennec";
> +    }
> +    else {
> +        name = "Firefox";

You already have a good default above, so drop this else.

Comment 13

[Bryan Kok [:transfusion]]

Attachment: bug-1019779.patch

Touched things up.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8457478 [details] [diff] [review]
bug-1019779.patch

Don't forget to add review flags, Bryan!

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8457478 [details] [diff] [review]
bug-1019779.patch

Bryan, this looks great.  It needs to be back-ported to android-sync; I should get to that today or tomorrow.  Leaving the NI flag so I don't forget.

Thanks for your work on this!

Comment 16

[Nick Alexander :nalexander [he/him]]

Landed with some trivial changes:

https://hg.mozilla.org/integration/fx-team/rev/70afb8fb9ae5

and back-ported to android-sync:

https://github.com/mozilla-services/android-sync/commit/cdafc277cea4228f8a494498b984f979ec68be5c

Comment 17

[Nick Alexander :nalexander [he/him]]

Thanks, Bryan!

Comment 18

[Nick Alexander :nalexander [he/him]]

Argh, landed wrong version of patch:

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70afb8fb9ae5
https://hg.mozilla.org/mozilla-central/rev/dc98e9e61e13

Comment 20

[Catalin Suciu [:csuciu]]

Verified fixed on Nightly 36.0a1 (2014-10-19)