Closed
Bug 1011464
Opened 10 years ago
Closed 10 years ago
[appmgr v2] get l10n right
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
So we have landed code here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webide/
It is not compiled yet, and before doing so, I'd like to make sure we're getting l10n right. Locales code is not hosted in /browser/locales/ because we want this tool to be able to live outside of Firefox at some point.
Can someone from l10n could look at this and help me make sure we're doing the right thing here?
Comment 1•10 years ago
|
||
'webide' sounds like a huge pile of strings, there are currently only a few. I'm curious what the final scope of the project is going to be.
Technically, the placing of the files should work. I'd really like to leave those file out of localization until we're switching the feature on by default, or only shortly before that. In particular if the UI you have right now is going through some churn still, we don't need to do stuff like key changes until we officially start the l10n process.
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #1)
> 'webide' sounds like a huge pile of strings, there are currently only a few.
> I'm curious what the final scope of the project is going to be.
The goal is to replace the App Manager. See some screenshots there:
https://bugzilla.mozilla.org/show_bug.cgi?id=1007371#c3
> Technically, the placing of the files should work.
Apparently, it does.
> I'd really like to leave
> those file out of localization until we're switching the feature on by
> default, or only shortly before that. In particular if the UI you have right
> now is going through some churn still, we don't need to do stuff like key
> changes until we officially start the l10n process.
How do I do that without removing the .dtd and .properties?
Assignee | ||
Comment 3•10 years ago
|
||
Oh, a test fail on Linux:
KeyError: 'webide'
make[3]: *** [repackage-zip] Error 1
make[2]: *** [repackage-zip-x-test] Error 2
make[1]: *** [l10n-check] Error 2
make: *** [l10n-check] Error 2
Any idea of what I'm doing wrong?
Comment 4•10 years ago
|
||
I think the best way to do that is to make the jar.mn and the chrome files register and reference chrome://content instead of chrome://locale, and to make sure there's no l10n.ini in which browser/devtools/webide is picked up.
I wish our packager wasn't so picky in this situation, or at least explained why it's failing.
Assignee | ||
Comment 5•10 years ago
|
||
Ok. First step, we move .dtd and .properties in content. We'll revert that once the project is stable enough.
Comment 6•10 years ago
|
||
Comment on attachment 8423942 [details] [diff] [review]
don't use chrome://locales
Review of attachment 8423942 [details] [diff] [review]:
-----------------------------------------------------------------
Yep, that looks good, thanks.
Attachment #8423942 -
Flags: review?(l10n) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 8•10 years ago
|
||
Setting leave-open, because I think that's what we want. I'm expecting follow-ups to actually switch stuff on when it's time, and that's fine to do in this bug. Paul, agree?
Keywords: leave-open
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #8)
> Setting leave-open, because I think that's what we want. I'm expecting
> follow-ups to actually switch stuff on when it's time, and that's fine to do
> in this bug. Paul, agree?
Yes.
No longer blocks: 1010387
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8423942 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Could someone help me understand/reproduce this failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=44965007&tree=Try
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
Comment 13•10 years ago
|
||
I'm far from familiar with the build system, but why are you keeping l10n files in /webide/locales?
Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?
Comment 14•10 years ago
|
||
Try again with this patch, and that will make the error message a bit more explicit about what's wrong:
diff --git a/python/mozbuild/mozpack/packager/l10n.py b/python/mozbuild/mozpack/packager/l10n.py
--- a/python/mozbuild/mozpack/packager/l10n.py
+++ b/python/mozbuild/mozpack/packager/l10n.py
@@ -73,20 +73,26 @@ def _repack(app_finder, l10n_finder, cop
for e in l10n.entries:
if isinstance(e, ManifestChrome):
base = mozpack.path.basedir(e.path, app.bases)
l10n_paths.setdefault(base, {})
l10n_paths[base][e.name] = e.path
# For chrome and non chrome files or directories, store what langpack path
# corresponds to a package path.
- paths = dict((e.path,
- l10n_paths[mozpack.path.basedir(e.path, app.bases)][e.name])
- for e in app.entries
- if isinstance(e, ManifestEntryWithRelPath))
+ paths = {}
+ for e in app.entries:
+ if instance(e, ManifestEntryWithRelPath):
+ base = mozpack.path.basedir(e.path, app.bases)
+ if base not in l10n_paths:
+ errors.fatal("Locale doesn't contain %s/" % base)
+ if e.name not in l10n_paths[base]:
+ errors.fatal("Locale doesn't have a manifest entry for '%s'" %
+ e.name)
+ path[e.path] = l10n_paths[base][e.name]
for pattern in non_chrome:
for base in app.bases:
path = mozpack.path.join(base, pattern)
left = set(p for p, f in app_finder.find(path))
right = set(p for p, f in l10n_finder.find(path))
for p in right:
paths[p] = p
Flags: needinfo?(mh+mozilla)
Updated•10 years ago
|
Flags: needinfo?(l10n)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #13)
> I'm far from familiar with the build system, but why are you keeping l10n
> files in /webide/locales?
> Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?
We're trying to keep WebIDE's code selfcontained, for maybe one day extract it as an addon or a xulrunner app.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #15)
> (In reply to Francesco Lodolo [:flod] from comment #13)
> > I'm far from familiar with the build system, but why are you keeping l10n
> > files in /webide/locales?
> > Shouldn't they be moved to /browser/locales/en-US/chrome/browser/devtools/?
>
> We're trying to keep WebIDE's code selfcontained, for maybe one day extract
> it as an addon or a xulrunner app.
Maybe it's not that simple. If not, I'll just move code into browser/locales
Assignee | ||
Comment 17•10 years ago
|
||
With Mike's patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=524256d9cb56
Comment 18•10 years ago
|
||
Note, we're missing a fragment in browser/locales/l10n.ini to pick up browser/devtools/webide.
I'm not too exited that we're having overlays of overlays of overlays in the browser file tree, tbh. It's just horrible to find the en-US files at this point. I'd really prefer to have devtools files in one spot beneath browser/devtools/locales/en-US
No idea how the packaging side works, though.
Assignee | ||
Comment 19•10 years ago
|
||
I'm not confident enough about how locales work to bypass the usual locales build system. And the value of isolating locales in /webide/ is almost null at this time. Let's go for the regular way.
Attachment #8465343 -
Attachment is obsolete: true
Attachment #8466287 -
Flags: review?(jryans)
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #18)
> Note, we're missing a fragment in browser/locales/l10n.ini to pick up
> browser/devtools/webide.
>
> I'm not too exited that we're having overlays of overlays of overlays in the
> browser file tree, tbh. It's just horrible to find the en-US files at this
> point. I'd really prefer to have devtools files in one spot beneath
> browser/devtools/locales/en-US
>
> No idea how the packaging side works, though.
So for now, I'll move webide files into /browser/locales/. But we could file a bug about moving locales in /browser/devtools/.
Assignee | ||
Comment 22•10 years ago
|
||
Pike: bug 1047499
Comment 23•10 years ago
|
||
Comment on attachment 8466287 [details] [diff] [review]
v2
Review of attachment 8466287 [details] [diff] [review]:
-----------------------------------------------------------------
I realized I haven't looked at the strings in a while. Some minor nits since you're going to expose them for localization (not sure if you want to that here on in another bug).
::: browser/devtools/webide/locales/en-US/webide.dtd
@@ -62,5 @@
> -
> -<!ENTITY projectPanel_myProjects "My Projects">
> -<!ENTITY projectPanel_runtimeApps "Runtime Apps">
> -<!ENTITY runtimePanel_USBDevices "USB Devices">
> -<!ENTITY runtimePanel_WiFiDevices "WiFi Devices">
Wi-Fi, not WiFi
::: browser/devtools/webide/locales/en-US/webide.properties
@@ -21,5 @@
> -
> -notification_showTroubleShooting_label=troubleshooting
> -notification_showTroubleShooting_accesskey=t
> -
> -error_operationTimeout=Operation timed out: %1$S
Can you add localization comments on variables (error_* strings)?
@@ -31,5 @@
> -error_cantFetchAddonsJSON=Can't fetch the add-on list: %S
> -
> -addons_stable=stable
> -addons_unstable=unstable
> -addons_simulator_label=Firefox OS %1$S Simulator (%2$S)
Same here: no clue what these variables are without a comment.
Comment on attachment 8466287 [details] [diff] [review]
v2
Review of attachment 8466287 [details] [diff] [review]:
-----------------------------------------------------------------
Seems good to me. Up to you if you'd like to address flod's comments here or in a follow-up.
Attachment #8466287 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Thank you Flod. I addressed your comments.
https://hg.mozilla.org/integration/fx-team/rev/9a3b88192df4
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
Comment 26•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•