Closed Bug 586685 Opened 10 years ago Closed 10 years ago

Pull out hard coded strings for localization

Categories

(Firefox Graveyard :: Panorama, defect, P1)

defect

Tracking

(blocking2.0 beta7+)

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: iangilman, Assigned: mitcho)

References

Details

(Whiteboard: [fixed-tabcandy][strings][qa-])

Attachments

(1 file, 3 obsolete files)

For instance, from dolske's review of bug 574217: 

> >+        // ___ make info item
> >+        var html =
> >+          "<div class='intro'>"
> >+            + "<h1>Welcome to Firefox Tab Sets</h1>" // TODO: This needs to be localized if it's kept in
> >+            + "<div>(more goes here)</div><br>"
> >+            + "<video src='http://people.mozilla.org/~araskin/movies/tabcandy_howto.webm' "
> >+            + "width='100%' preload controls>"
> >+          + "</div>";
> 
> This ought to be localized for landing, otherwise the localizers (hi Pike!)
> will freak out and yell at you. :)
> 
> Either create a DTD w/string entities to load into your .html, or open a string
> bundle from your JS code and pull strings from that.
Depends on: 574217
If Tab Candy lands on mozilla-central, we'll need this to follow it right-quick so that localizers can get on it for b4.
I can't really grok the scope of this ad-hoc.

There is an alternative path, which is, extract the strings post-B4 with good thinking of "does this stay in", and then, "how do we localize this properly?". That may take more than a day, thus maybe post-B4?
(In reply to comment #2)
> There is an alternative path, which is, extract the strings post-B4 with good
> thinking of "does this stay in", and then, "how do we localize this properly?".
> That may take more than a day, thus maybe post-B4?

If you're OK with hardcoded strings in b4, and can carry that water with the l10n community, I'm sure it'll work for the tab-candy people and we can make this an early b5 blocker assuming tab-candy sticks.
Yeah. I'm smelling ajax and html (not xhtml) so our localization story is weak. There may be things like moving stuff to xhtml and around that work better for l10n, but it's really easier to figure those out with a few more folks and round trips on patches.

Reminds me about bug 584444, I'd love to get a relnote for partially English strings.
Mitcho's gonna take this on. I don't think we have a lot of strings, really. First step is to do an audit and get the full list.
Assignee: nobody → mitcho
Priority: -- → P2
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/b10240adf7d6

Now all user-facing strings in JS should be localizable via stringbundle. All XUL strings were already DTD'd.

Will mark fixed when this gets into m-c.
Status: NEW → ASSIGNED
Priority: P2 → --
Whiteboard: b4
We have very few strings (like on the order of 3). The bulk of the work (which is unrelated to this) will probably be in localizing the video introduction that appears in the first-run experience of Tab Candy. Will get a bug for that later.
Blocks: 586788
Attached patch Patch v1 (obsolete) — Splinter Review
This patch also includes a fix to the group close box (it wasn't working), and so we'd really like to get it in tonight's nightly!
Attachment #465468 - Flags: review?(dolske)
Looks like the close box bug I'm referring to (and which this patch fixes) has been reported as bug 586814.
Priority: -- → P1
Comment on attachment 465468 [details] [diff] [review]
Patch v1

>-    var w = Math.min(this.bounds.width - closeButton.width() - closeButton.css('right'),
>+    var w = Math.min(this.bounds.width - parseInt(closeButton.width()) - parseInt(closeButton.css('right')),

Pull this out and land it as a patch for bug 586814? r+=me

>   // Removes the New Tab Group, which is now defunct. See bug 575851 and comments therein.
>   killNewTabGroup: function() {
>+    let newTabGroupTitle = tabviewBundle
>+              .GetStringFromName('tabview.groupItems.defunctNewTabGroupTitle');
>     this.groupItems.forEach(function(groupItem) {
>-      if (groupItem.getTitle() == 'New Tabs' && groupItem.locked.title) {
>+      if (groupItem.getTitle() == newTabGroupTitle && groupItem.locked.title) {

Does this being "defunct" mean that groupItem.locked is also going away?

Wondering if users can trigger unexpected behavior should they happen to name a group "New Tabs". Ideally the text wouldn't be keyed upon here, but maybe that just doesn't matter if this is all going away.


>+++ b/browser/base/content/tabview/ui.js	Thu Aug 12 16:52:13 2010 -0700
...
>+        let welcome = tabviewBundle.GetStringFromName('tabview.welcomeHeader');
>+        let more = tabviewBundle.GetStringFromName('tabview.welcomeText');
>+        let video = tabviewBundle.GetStringFromName('tabview.welcomeVideoSrc');

Probably don't need / want to expose the video URL to localizers yet, since there's nothing they can do with it. Remove or at 

If the intention is to actually produce this/a video in multiple locales for Fx4 final, then we should keep it.

>+++ b/browser/locales/en-US/chrome/browser/tabview.properties	Thu Aug 12 16:52:13 2010 -0700
...
>+tabview.welcomeVideoSrc=http://people.mozilla.org/~araskin/movies/tabcandy_howto.webm

For now, could add # LOCALIZATION NOTE (tabview.welcomeVideoSrc): do not localize this URL.
Attachment #465468 - Flags: review?(dolske) → review+
Attachment #465468 - Flags: approval2.0+
(In reply to comment #10)
> Comment on attachment 465468 [details] [diff] [review]
> Does this being "defunct" mean that groupItem.locked is also going away?
> 
> Wondering if users can trigger unexpected behavior should they happen to name a
> group "New Tabs". Ideally the text wouldn't be keyed upon here, but maybe that
> just doesn't matter if this is all going away.

Yes, it's possible .locked will go away as well. At any rate, people cannot create locked groups, so if there's a group with that title and it's locked, then it's definitely the thing we're trying to clean out. 

> Probably don't need / want to expose the video URL to localizers yet, since
> there's nothing they can do with it. Remove or at 
> 
> If the intention is to actually produce this/a video in multiple locales for
> Fx4 final, then we should keep it.
> 
> >+++ b/browser/locales/en-US/chrome/browser/tabview.properties	Thu Aug 12 16:52:13 2010 -0700
> ...
> >+tabview.welcomeVideoSrc=http://people.mozilla.org/~araskin/movies/tabcandy_howto.webm
> 
> For now, could add # LOCALIZATION NOTE (tabview.welcomeVideoSrc): do not
> localize this URL.

I believe the idea is to provide different videos for different locales. The comment sounds good. I've added it in tabcandy-central:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/e49b735ae874

... but I don't know how to generate a fresh patch. Mardak, please help?
See, this is why I'm not sure this should land for B4. I don't know if the best way to localize this is to keep the current code path with innerHTML and creating the content via js. There are quite a few alternatives to that, which I'd like to check with a bit more thought.

PS: Do you have an r+ to have a video url on people going to a beta audience? Not sure if that server is set up to serve that kinda load. I'm pretty sure that I don't want that URL in the localized strings until we have feedback from webdev and the folks that do the videos for whatsnew on how to localize it.
(In reply to comment #12)
> PS: Do you have an r+ to have a video url on people going to a beta audience?
> Not sure if that server is set up to serve that kinda load. I'm pretty sure
> that I don't want that URL in the localized strings until we have feedback from
> webdev and the folks that do the videos for whatsnew on how to localize it.

For what it's worth, we presently are localizing videos during this beta process.  Pascalc has been working the video on the First Run page.  You can glance at that work in bug 578988.  I suspect we would follow the same process, where we would place translations of the subtitles for the video inside each locale's SVN repo in a directory like /includes/l10n/sub-fx4-whatsnew-beta.html.  The subtitles would be a simple html file with each enclosed in a <div>.

Although the Universal Subtitles tool is available for use, their server infrastructure is not ready to handle our load.
Blocks: 587068
Whiteboard: b4 → b4 [fixed-tabcandy]
(In reply to comment #12)
> See, this is why I'm not sure this should land for B4. I don't know if the best
> way to localize this is to keep the current code path with innerHTML and
> creating the content via js. There are quite a few alternatives to that, which
> I'd like to check with a bit more thought.

This patch is on its way to landing, so at least we have the strings isolated, but we're certainly open to changing course afterward.
@Axel: The video is now on the Mozilla CDN (thanks to the quick thinking of Betlzner and quick acting of Zandr) so we should be fine on that front.

@Seth: Thanks for calling out bug 578988, really good to know. Question: Have you guys played with using DotSub to make the transcription/translation process easier?

Finally, that is a draft of the video. My hope is to make another one which is a little tighter and more scripted.
(In reply to comment #15)
> @Seth: Thanks for calling out bug 578988, really good to know. Question: Have
> you guys played with using DotSub to make the transcription/translation process
> easier?

We haven't used DotSub, only tested it.  I know some other localization projects who are using it, and remarks have been negative from a project management perspective.  Therefore, we elected not to move forward with their solution, waiting for some love from the Universal Subtitle project.
Blocks: 586814
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
No longer blocks: 586814
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Blocks: 586814
Guys, please do not land this. There's no point in causing dozens of l10n folks to waste their time on broken l10n attempts.
http://hg.mozilla.org/mozilla-central/rev/62e087ddf724
NOT.
Everything but user facing strings being localized.
Random whitespace fixes.
Random styling changes.
Also has Bug 587158 - Update intro-video URL to CDN'ed version
And string changes.
And Bug 586814 - Unable to close Tab groups
Blocks: 587158
Whiteboard: b4 [fixed-tabcandy] → [fixed-tabcandy]
Target Milestone: --- → Firefox 4.0b4
Soooo, is this WONTFIX pending a comeback from l10n on how the bloody blue hell they would like us to localize things? Not sure what the next action is, or who owns it.
I'm not sure what got backed out, my "this" in comment 18 was referring to the html div piece, which this bug is about.

The back-out looks much more impact-full, not sure what the complete story on that is.
So, I've been digging a bit.

The InfoItem class is completely based on non-localizable innerHTML code. Sad, but true, there's no story line for localizing html on the client.

I'd rather not start to create one now unless there's a really compelling reason to do so, and given that it's only used once so far, I'd be quite happy not to. Couldn't we add the div to tabview.html (moved to tabview.xhtml) and wrap it with js logic instead of creating it by hand?

Then we could use standard DTD files to localize this.

The existing code probably fails for sizing of the widget, as well as positioning in an RTL context.

Longer context: Inventing a new localization setup makes us loose all mindshare on how to find callsites for localized content, and takes away quite some community that's able to help and debug. Doesn't sound like a great path forward, in particular if it's not really used widely, with the potential to actually build mindshare.
What was wrong with the StringBundle .properties approach for adding text to the div?
It didn't fix sizing and RTL, for one.

The other piece is, if I have a hard time figuring out what the changes in the string do to the resulting display, it's a good idea to look for a simpler solution.

Also, localizing the video is really something we should do with more thought. See http://www.mozilla.com/de/firefox/4.0b3/whatsnew/ for a page with video and subtitles, that sounds like a much better solution. Also, maybe we should just put the inner pieces of that div on the web right away? That way, we can be more flexible on how to do subtitles etc, and possibly fix links etc, too. Plus richer fallback if a localizer didn't do the video subtitles based on accept-lang, too.
QA Contact: tabcandy → tabcandy
Aza, can you get us an update here? We're using subtitles on the web now for the video, right? Should we just put parts of that into an iframe?

Also, do you have a better overview if the InfoItem code is used anywhere else?
it's possible to includes subtitles in the video of Panorama inside the browser ?
I don't think it's a good idea to replicate the subtitles and the code to display them twice, if the video is coming from the web anyway.
problem is if the user is not connected to the net, he can't play video...

another solution : localized the video, with voice...
Again, the video is coming from the web anyway. No net, no video.
No longer blocks: 586788
Target Milestone: Firefox 4.0b4 → Firefox 4.0
blocking2.0: --- → ?
Whiteboard: [fixed-tabcandy] → [fixed-tabcandy][strings]
(In reply to comment #26)
> Also, do you have a better overview if the InfoItem code is used anywhere else?

No, InfoItem is used only in this instance.
Could we rip it out then, and just make something that works for the video content explicitly?
As of now, I've removed any strings around the video. Upon further thought, my guess for the best way to do the video is instead of directly embedding the video to have a single tab automatically open that points to a page that hosts the video. (This way you can save that tab for later, and group it like the rest of Panorama). Also, then everything—l10n—included would be a web page.

Thoughts?
Sounds cool.
blocking2.0: ? → beta6+
There are still hard-coded strings elsewhere in the Tab Candy code (such as the default title promt "Name this tab group"). We pulled them out once (see the attached patch, now out of date), but were told to back out the change. What should we done about them?
Ignoring the video, we need to pull those out.
I'm happy to review patches, now that the direction of matters are clear.
mitcho, can you pull out the strings as appropriate (but not the video)?
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #465468 - Attachment is obsolete: true
Attachment #473270 - Flags: feedback?(ian)
Comment on attachment 473270 [details] [diff] [review]
Patch v2

+ tabview.welcomeHeader=How to organize your tabs

You don't need this, right? Kill it. 

f+ with that. Please fix and r? dietrich
Attachment #473270 - Flags: feedback?(ian) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #473270 - Attachment is obsolete: true
Attachment #473347 - Flags: review?(dietrich)
Blocks: 594094
Comment on attachment 473347 [details] [diff] [review]
Patch v3


>     .attr('title',
>-          "New tab")
>+          tabviewBundle.GetStringFromName('tabview.groupItem.newTabButton'))

let's simplify, see below.

> XPCOMUtils.defineLazyGetter(this, "gTabViewFrame", function() {
>   return gWindow.document.getElementById("tab-view");
> });
> 
>+let tabviewBundle = Services.strings.
>+                    createBundle("chrome://browser/locale/tabview.properties");
>+

let's simplify and optimize by:

1) make a lazy getter for the bundle (see gTabViewFrame above it for example)

2) make a utility function to make it easier to access the strings in a more compact way, eg:

function tabviewString(name) tabviewBundle.GetStringFromName('tabview.' + name)

r=me with these changes.
Attachment #473347 - Flags: review?(dietrich) → review+
Attachment #473347 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0d41af6784f7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b6
Whiteboard: [fixed-tabcandy][strings] → [fixed-tabcandy][strings][qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.