Closed Bug 944749 Opened 10 years ago Closed 10 years ago

[Settings] Use different labels for menu items and headers

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: flod, Assigned: flod)

References

Details

Attachments

(2 files)

Settings is probably the area where most of the truncation problems happen. Maybe having different entities for headers and common labels could be a start.

Need to understand if this approach is too much.
Attached file Link to Github PR
About 50 more strings.
Attachment #8340461 - Flags: review?(kaze)
Attachment #8340461 - Flags: feedback?(l10n)
Assignee: nobody → francesco.lodolo
Comment on attachment 8340461 [details] [review]
Link to Github PR

I wonder if we should do the 

foo-header = {{ foo }}

trick instead of adding the strings verbatim? That way, the existing localizations would continue to work as is until folks catch up with it, and they could just keep the en-US entry if no shortening was needed.
Attachment #8340461 - Flags: feedback?(l10n)
(In reply to Axel Hecht [:Pike] from comment #4)
> I wonder if we should do the 
> foo-header = {{ foo }}

Updated the PR to use fallbacks and reduce impact on existing localizations.
Comment on attachment 8340461 [details] [review]
Link to Github PR

r=me, please rebase and ping me if you need me to merge it to master.

I feel a bit sad about the settings.en-US.properties file, though. Just to make it clear — I assume your team has considered the three following questions and answered “yes” every time:
 • is this for the master branch only?
 • if yes, do we really have to use the {{defaultString}} trick for every *-header?
 • if yes, do we really want to keep all these localization notes?

I thought we could have copied all strings and let localizers change the *-headers entities when needed.
Attachment #8340461 - Flags: review?(kaze) → review+
Ooops, I missed Pike’s comment.

(In reply to Axel Hecht [:Pike] from comment #4)
> I wonder if we should do the 
> 
> foo-header = {{ foo }}
> 
> trick instead of adding the strings verbatim?

I’d say we don’t need that trick unless we intend to uplift this patch to the 1.3 branch. So I’d suggest to see what triage says about the 1.3? flag I’ve just set, and if it’s not a 1.3 blocker I’d prefer to keep verbatim strings in the settings.en-US.properties file, which is long enough already.

Just a suggestion — Flod & Pike, I’ll trust your judgment on this.
blocking-b2g: --- → 1.3?
Considering the amount of truncation issues this could help fixing, and the fact that we failed to fix a couple of these in 1.2 already, I really think we need this in 1.3

I though about this (the first patch didn't have fallbacks or comments, but I didn't realize we were so close to the end of the cycle), but this approach has a lot of pros: if a locale was in good shape in 1.2 and is late on 1.3, all these strings will have a localization and QA won't complain about it.

I also thought about creating a "file general localization comment" for all -header strings. This could make the file more readable, but also fail with tools that display l10n comments with the associated string.
Keywords: checkin-needed
Keywords: checkin-needed
Triage: We would like Fabrice's opinion on whether or not to 1.3+ this.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(fabrice)
master: https://github.com/mozilla-b2g/gaia/commit/588a3e02c4ace3b3341ba1f6bb7274120b53b2b3

:flod, I am not sure if you removed "checkin-needed" accidentally, but your commit looks fine so I presume you want people to land this. So I did. If this is a mistake please ask for a backout.
Flags: needinfo?(francesco.lodolo)
I removed the "checkin-needed" because Kaze warned me it needed rebasing (which I did), and he was also testing it locally since Travis is unreliable. 

Since I haven't heard anything from him in the last hours, I think it's fine to have it merged.
Flags: needinfo?(francesco.lodolo)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> Triage: We would like Fabrice's opinion on whether or not to 1.3+ this.

Yes, that's fine - we are not yet string frozen for 1.3
Flags: needinfo?(fabrice)
I'm not sure, but do you think it is related to this PR? (I'm talking about the {{brandShortName}} in the header of course)

I have not yet updated settings.properties for FR, so we are using the fallback "hack" in en-US file.
This is the latest GP 1.4 build
It could definitely be.

improveBrowserOS=Improve {{brandShortName}}
improveBrowserOS-header={{improveBrowserOS}}

@kaze
Is the system able to manage the double substitution? Should we remove the fallback for this one?
Flags: needinfo?(kaze)
Blocks: 950463
(In reply to Francesco Lodolo [:flod] from comment #15)
> Is the system able to manage the double substitution?

Ugh, good question. Looking.
(In reply to Francesco Lodolo [:flod] from comment #15)
> It could definitely be.
> 
> improveBrowserOS=Improve {{brandShortName}}
> improveBrowserOS-header={{improveBrowserOS}}
> 
> @kaze
> Is the system able to manage the double substitution? Should we remove the
> fallback for this one?

Yeah, Francesco, 

after tracking, I think bug 950171 happened because of double substitution. 

Is there any way to handle this ? 

Thanks.
We can just remove the substitution in the header, but I'd like Kaze's opinion before doing that.

improveBrowserOS=Improve {{brandShortName}}
improveBrowserOS-header=Improve {{brandShortName}}
Yeah, it's easy to fix but i think this situation will happen again.

Thanks for your quick response and let's wait for Kaze's response to make sure is there any plan / mechanism for this case.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> 588a3e02c4ace3b3341ba1f6bb7274120b53b2b3
> 

I just realized I didn't close this bug when I merge the patch. Sorry O_o.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Long story short, l10n.js does not support double substitution yet. See bug 950171 comment 11.
Flags: needinfo?(kaze)
Thank Kaze, I'm going to open a follow-up bug tomorrow and open a PR to change the string.
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 588a3e02c4ace3b3341ba1f6bb7274120b53b2b3
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(francesco.lodolo)
Imported patch in my fork on 1.3, hope this is enough
https://github.com/flodolo/gaia/commit/d256a30ef3fbdf8d70cfff2c12926df2ab9bf903

One note: not sure if that's the right way to proceed, but to avoid another conflict with bug 950171 (it was filed to fix a regression created by this same PR), I dropped the change to apps/settings/elements/improve_browser_os.html
Flags: needinfo?(francesco.lodolo) → needinfo?(jhford)
[v1.3 660d52b] Merge pull request #14224 from flodolo/separate_header
Flags: needinfo?(jhford)
You need to log in before you can comment on or make changes to this bug.