Closed
Bug 944749
Opened 11 years ago
Closed 11 years ago
[Settings] Use different labels for menu items and headers
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Firefox OS Graveyard
Gaia::Settings
Tracking
(blocking-b2g:1.3+, 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.
Assignee | ||
Comment 1•11 years ago
|
||
About 50 more strings.
Attachment #8340461 -
Flags: review?(kaze)
Attachment #8340461 -
Flags: feedback?(l10n)
Comment 3•11 years ago
|
||
(Thanks, Flod!)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → francesco.lodolo
Comment 4•11 years ago
|
||
Comment on attachment 8340461 [details]
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.
Updated•11 years ago
|
Attachment #8340461 -
Flags: feedback?(l10n)
Assignee | ||
Comment 5•11 years ago
|
||
(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 6•11 years ago
|
||
Comment on attachment 8340461 [details]
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+
Comment 7•11 years ago
|
||
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?
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
Triage: We would like Fabrice's opinion on whether or not to 1.3+ this.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(fabrice)
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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)
Comment 12•11 years ago
|
||
(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)
Comment 13•11 years ago
|
||
Quoting preeti,
Soft string freeze 12/9/2013
https://groups.google.com/forum/#!topic/mozilla.dev.gaia/UFJjr_r-X4k
Comment 14•11 years ago
|
||
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
Assignee | ||
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
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.
Comment 20•11 years ago
|
||
(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: 11 years ago
Resolution: --- → FIXED
Comment 21•11 years ago
|
||
Long story short, l10n.js does not support double substitution yet. See bug 950171 comment 11.
Flags: needinfo?(kaze)
Assignee | ||
Comment 22•11 years ago
|
||
Thank Kaze, I'm going to open a follow-up bug tomorrow and open a PR to change the string.
Comment 23•11 years ago
|
||
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)
Assignee | ||
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
[v1.3 660d52b] Merge pull request #14224 from flodolo/separate_header
status-b2g-v1.3:
--- → fixed
Flags: needinfo?(jhford)
You need to log in
before you can comment on or make changes to this bug.
Description
•