Shorten and localize default client device names for Android Sync

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: nalexander, Assigned: transfusion, Mentored)

Tracking

unspecified
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java][good second bug])

Attachments

(1 attachment, 2 obsolete attachments)

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
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.
Whiteboard: [mentor=nalexander][lang=java][good second bug]
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.
Mentor: nalexander
Whiteboard: [mentor=nalexander][lang=java][good second bug] → [lang=java][good second bug]
Mentor: michael.l.comella
via IRC.
Assignee: nobody → bryan.wyern1
Status: NEW → ASSIGNED
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
(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
Posted patch bug-1019779.patch (obsolete) — Splinter Review
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 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.
Attachment #8454674 - Flags: feedback+
(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.
(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.
Flags: needinfo?(nalexander)
(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
Flags: needinfo?(nalexander)
Posted patch bug-1019779.patch (obsolete) — Splinter Review
Fixed string names, added a localization note, and added the targeted search for the release channel in MOZ_APP_DISPLAY_NAME.
Attachment #8454674 - Attachment is obsolete: true
Attachment #8456943 - Flags: review?(michael.l.comella)
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.
Attachment #8456943 - Flags: review?(michael.l.comella) → feedback+
Touched things up.
Attachment #8456943 - Attachment is obsolete: true
Comment on attachment 8457478 [details] [diff] [review]
bug-1019779.patch

Don't forget to add review flags, Bryan!
Attachment #8457478 - Flags: review?(nalexander)
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!
Attachment #8457478 - Flags: review?(nalexander) → review+
Flags: needinfo?(nalexander)
Thanks, Bryan!
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/70afb8fb9ae5
https://hg.mozilla.org/mozilla-central/rev/dc98e9e61e13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Verified fixed on Nightly 36.0a1 (2014-10-19)
Status: RESOLVED → VERIFIED
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.