Closed
Bug 1019719
Opened 10 years ago
Closed 10 years ago
Shorten and localize default client device names for Android Sync
Categories
(Firefox for Android Graveyard :: Android Sync, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 34
People
(Reporter: nalexander, Assigned: transfusion, Mentored)
References
Details
(Whiteboard: [lang=java][good second bug])
Attachments
(1 file, 2 obsolete files)
11.31 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
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
Comment 5•10 years ago
|
||
(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
Assignee | ||
Comment 6•10 years ago
|
||
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+
Reporter | ||
Comment 8•10 years ago
|
||
(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)
Reporter | ||
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
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)
Reporter | ||
Comment 16•10 years ago
|
||
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
Reporter | ||
Comment 18•10 years ago
|
||
Argh, landed wrong version of patch: https://hg.mozilla.org/integration/fx-team/rev/dc98e9e61e13
https://hg.mozilla.org/mozilla-central/rev/70afb8fb9ae5 https://hg.mozilla.org/mozilla-central/rev/dc98e9e61e13
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•7 years ago
|
Product: Android Background Services → Firefox for Android
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•