586685 - Pull out hard coded strings for localization

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Ian Gilman [:iangilman]]

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.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 2

[Axel Hecht [:Pike]]

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?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

(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.

Comment 4

[Axel Hecht [:Pike]]

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.

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 7

[Aza Raskin [:aza]]

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.

Comment 8

[Ian Gilman [:iangilman]]

Attachment: Patch v1

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!

Comment 9

[Ian Gilman [:iangilman]]

Looks like the close box bug I'm referring to (and which this patch fixes) has been reported as bug 586814.

Comment 10

[Justin Dolske [:Dolske]]

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.

Comment 11

[Ian Gilman [:iangilman]]

(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?

Comment 12

[Axel Hecht [:Pike]]

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.

Comment 13

[Seth Bindernagel]

(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.

Comment 14

[Ian Gilman [:iangilman]]

(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.

Comment 15

[Aza Raskin [:aza]]

@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.

Comment 16

[Seth Bindernagel]

(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.

Comment 17

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 18

[Axel Hecht [:Pike]]

Guys, please do not land this. There's no point in causing dozens of l10n folks to waste their time on broken l10n attempts.

Comment 19

[Ed Lee :Mardak]

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

Comment 20

[Ed Lee :Mardak]

backed out http://hg.mozilla.org/mozilla-central/rev/c14d91f2d939

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 22

[Axel Hecht [:Pike]]

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.

Comment 23

[Axel Hecht [:Pike]]

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.

Comment 24

[Ed Lee :Mardak]

What was wrong with the StringBundle .properties approach for adding text to the div?

Comment 25

[Axel Hecht [:Pike]]

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.

Comment 26

[Axel Hecht [:Pike]]

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?

Comment 27

[Antoine Turmel [:GeekShadow]]

it's possible to includes subtitles in the video of Panorama inside the browser ?

Comment 28

[Axel Hecht [:Pike]]

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.

Comment 29

[Antoine Turmel [:GeekShadow]]

problem is if the user is not connected to the net, he can't play video...

another solution : localized the video, with voice...

Comment 30

[Axel Hecht [:Pike]]

Again, the video is coming from the web anyway. No net, no video.

Comment 31

[Ian Gilman [:iangilman]]

(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.

Comment 32

[Axel Hecht [:Pike]]

Could we rip it out then, and just make something that works for the video content explicitly?

Comment 33

[Aza Raskin [:aza]]

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?

Comment 34

[Axel Hecht [:Pike]]

Sounds cool.

Comment 35

[Ian Gilman [:iangilman]]

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?

Comment 36

[Aza Raskin [:aza]]

Ignoring the video, we need to pull those out.

Comment 37

[Axel Hecht [:Pike]]

I'm happy to review patches, now that the direction of matters are clear.

Comment 38

[Ian Gilman [:iangilman]]

mitcho, can you pull out the strings as appropriate (but not the video)?

Comment 39

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2

Comment 40

[Ian Gilman [:iangilman]]

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

Comment 41

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3

Comment 42

[Dietrich Ayala (:dietrich)]

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.

Comment 43

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 44

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0d41af6784f7