Closed Bug 1141479 Opened 7 years ago Closed 7 years ago

Fix new strings for default activities (followup bug 1073495)

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

defect
Not set
normal

Tracking

(b2g-master fixed)

RESOLVED FIXED
2.2 S8 (20mar)
Tracking Status
b2g-master --- fixed

People

(Reporter: fcampo, Assigned: fcampo)

References

Details

Attachments

(3 files)

+++ This bug was initially created as a clone of Bug #1073495 +++

Per bug 1073495 comment 26 and comment 27
We have unused , duplicated and wrong string IDs

Original list on  bug 1039386 attachment 8522777 [details]
Attachment #8575228 - Flags: review?(francesco.lodolo)
I commented in the PR, but I'm missing a lot of context to understand these strings

> default-activity-newcontact = Open contact
> default-activity-updatecontact = Open contact

"Create/update contact" based on key name?

> default-activity-viewurl-email = Send email
If the activity is "view", should it be "Open link" or similar?
(In reply to Francesco Lodolo [:flod] from comment #3)
> I commented in the PR, but I'm missing a lot of context to understand these
> strings
> 
> > default-activity-newcontact = Open contact
> > default-activity-updatecontact = Open contact
> 
> "Create/update contact" based on key name?
> 
> > default-activity-viewurl-email = Send email
> If the activity is "view", should it be "Open link" or similar?
Sorry I didn't explain this properly before asking for a review, I'll try now:

As you can see in the specs [https://bug1039386.bugzilla.mozilla.org/attachment.cgi?id=8522777 page 10], we have a list of Actions with different filters. Even if those actions are different for the OS (new contact, open contact, update contact), for this feature they are grouped together, as they will be opened with the same the moment any of them are associated with a default app.

So one proposal would be the one on the patch: using different L10nIds for each of the possible actions, but sharing the string. Not sure if that is correct for the translation team
A tweak for this would be having a master key for the ones shared, maybe it's better understandable like this:
open-contact = Open contact
default-activity-newcontact     = {{open-contact}}
default-activity-opencontact    = {{open-contact}}
default-activity-updatecontact  = {{open-contact}}


The second proposal would be to have less L10nIds than total actions, and re-use the IDs directly from the JS files
{
  name: 'update',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
},
{
  name: 'open',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
},
{
  name: 'new',
  type: ['webcontacts/contact'],
  l10nId: 'default-activity-opencontact',
  settingsId: 'activity.default.opencontact'
}

Hope I made it more clear now
Flags: needinfo?(francesco.lodolo)
Thanks, a lot clearer with the specs.

I think the second approach is better: have one key for "Open contact", and use it everywhere is necessary.

I would also a localization note to the file, e.g.

# These strings are used to describe actions available to users, and which
# applications are associated as default handlers to these actions.
# For example: "Open link" -> Firefox.
Flags: needinfo?(francesco.lodolo)
A,B & C comments from last page of https://bug1039386.bugzilla.mozilla.org/attachment.cgi?id=8522777 should really be included as comment in activities.properties
Strings updated for the second proposal and comments added, can you take a new look Francesco?
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8575228 [details] [review]
PR - https://github.com/mozilla-b2g/gaia/pull/28748

I'm still not sure I should review patches on Gaia. A

nyhow, the strings look good, I have some doubts on the localization comment, especially the second part: it's a bit confusing as it is, and I don't think it's really relevant in the localization file.
Flags: needinfo?(francesco.lodolo)
Attachment #8575228 - Flags: review?(francesco.lodolo) → review+
(In reply to Francesco Lodolo [:flod] from comment #8)
> Comment on attachment 8575228 [details] [review]
> PR - https://github.com/mozilla-b2g/gaia/pull/28748
> 
> I'm still not sure I should review patches on Gaia. A
This patch has no changes in code, just strings, so I'd say we're safe with a review from l10n (tests passing).

> nyhow, the strings look good, I have some doubts on the localization
> comment, especially the second part: it's a bit confusing as it is, and I
> don't think it's really relevant in the localization file.
everything updated, code comments moved to the code and language nit added.
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
(In reply to Autolander from comment #10)
> Autolander could not locate a review from a user within the suggested
> reviewer list. Either the patch author or the reviewer should be in the
> suggested reviewer list.
well, this answers our concerns :D
Comment on attachment 8575227 [details] [review]
[gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

Autolander made me do it :p
Attachment #8575227 - Flags: review?(etienne)
Comment on attachment 8575227 [details] [review]
[gaia] fcampo:fix-strings-1141479 > mozilla-b2g:master

:)
Attachment #8575227 - Flags: review?(etienne) → review+
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
I had to revert this in https://github.com/mozilla-b2g/gaia/commit/a6b2d61cfda66faf4d00af194da2a05961370988 for Gu test failures on b2g-inbound:

https://treeherder.mozilla.org/logviewer.html#?job_id=1521972&repo=b2g-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(fernando.campo)
Resolution: FIXED → ---
(In reply to Wes Kocher (:KWierso) from comment #16)
> I had to revert this in
> https://github.com/mozilla-b2g/gaia/commit/
> a6b2d61cfda66faf4d00af194da2a05961370988 for Gu test failures on b2g-inbound:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=1521972&repo=b2g-
> inbound
Thanks for reverting it, I had the impression that autolander only merged PRs when tests were green, but I'll be more careful in the future.

Tests are passing now [https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=9c9b386cc2f6], so I'll just carry the r+ from Francesco and Etienne and merge this manually.

master: https://github.com/mozilla-b2g/gaia/commit/491314497f2d220a78aa2818d0455fb395049d81
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(fernando.campo)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.