[Gallery][Video][l10] Dynamic language support for group headers

RESOLVED FIXED

Status

Firefox OS
Gaia::Gallery
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ranjith253, Assigned: djf)

Tracking

unspecified

Firefox Tracking Flags

(b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 affected)

Details

(Whiteboard: [LibGLA,TD91781,QE1,A])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.143 Safari/537.36

Steps to reproduce:

1. Open gallery & press home button.
2. Go to setting , change language.
3. Come back to gallery app


Actual results:

Group header date is not translated to current language.


Expected results:

Group header date should be translated to current language.
(Reporter)

Updated

3 years ago
Whiteboard: [LibGLA,TD91781,QE,A]
(Reporter)

Updated

3 years ago
Whiteboard: [LibGLA,TD91781,QE,A] → [LibGLA,TD91781,QE1,A]
Hi Delphine -

Is this one part of L10N job?

Thanks

Vance
Flags: needinfo?(lebedel.delphine)
(Reporter)

Comment 2

3 years ago
Created attachment 8484902 [details] [diff] [review]
patch

Hi Punam,

Can we handle this issue by changing the date headers textContent on localized event? Share your thought on this approach.
Attachment #8484902 - Flags: feedback?(pdahiya)
I don't think this is an l10n issue. Adding Gandalf, I think he can help here
Flags: needinfo?(lebedel.delphine) → needinfo?(gandalf)
That actually is an l10n issue. And the patch would work, although I believe that there's a cleaner solution.

Basically, at the moment ThumbnailDateGroup constructor is using mozL10n.get without waiting for mozL10n resources to be loaded. That's a race condition.

It works, because it waits for DB loading, which takes at the moment longer than l10n resources, but it's still not really safe.

What I would recommend is wrapping the piece of the constructor that does date related l10n into a mozL10n wrapper:

var dummyDiv = document.createElement('div');
parent.appendChild(dummyDiv);

navigator.mozL10n.ready(function localizeHeader() {
  var _ = navigator.mozL10n.get;
  var dateFormatter = new navigator.mozL10n.DateTimeFormat();
  var htmlText = ThumbnailDateGroup.Template.interpolate({'group-header': dateFormatter.localeFormat(new Date(item.date), _('date-group-header'))});
  dummyDiv.innerHTML = htmlText;
});

=================

This will result in this code being executed as soon as l10n resources are loaded (in most cases instantly at the launch time), and then `localizeHeader` will be refired on each language change.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gandalf)
(In reply to ranjith253 from comment #2)
> Created attachment 8484902 [details] [diff] [review]
> patch
> 
> Hi Punam,
> 
> Can we handle this issue by changing the date headers textContent on
> localized event? Share your thought on this approach.

Sorry for the late reply, I agree with gandalf suggestion in #comment4 to check for l10n resources to be loaded, can you please try and test if it fixes the issue. Thanks!
Attachment #8484902 - Flags: feedback?(pdahiya)
(Assignee)

Comment 6

3 years ago
Ranjith,

Thanks for your work on this patch. The ready() function that gandalf suggests is better than listening for the localized event as you do in your patch.

I'm working on a simpler approach however, which is to just not use the DateTimeFormat() class at all and instead modify the template like this

  <span data-l10n-id="month-${month}-long"></span><span>${year}</span>

Then, I'll interpolate the template with $month set to date.getMonth() and $year set to date.toLocaleString("%Y").  I think that will give me equivalent output to DateTimeFormat(), but the month name will automatically change with the locale.

Gandalf: does this seem like an okay approach to localizing "%B %Y" date like "November 2014"? Are there any locales that format the year as anything other than four decimal digits? If so that wouldn't be changed when the user switches languages. But if not, then it seems cleaner to do it this way and have the month name localized with data-l10n-id.
Flags: needinfo?(gandalf)
(Assignee)

Comment 7

3 years ago
Taking this bug and editing the title to indicate that it applies to the video app as well.
Assignee: nobody → dflanagan
Summary: [Gallery][l10] Dynamic language support for group headers → [Gallery][Video][l10] Dynamic language support for group headers
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1097581
(Assignee)

Updated

3 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
(Assignee)

Comment 9

3 years ago
Created attachment 8526406 [details] [review]
proof of concept patch on github

Gandalf: is this an acceptable way to localized a "%B %Y" date ("November 2014") or are there locales where this will fail because the four digits of the year need to be formatted differently?

If I can avoid it I want to not have to use the ready() callback to change all of these when the user's language changes.

I suppose that the right way to do this might be to modify l10n.js to do localization of <time> elements so I could do something here like:

  <time datetime="2014-11-01" data-l10n-format="%B %Y">

Or maybe just add:

  <span data-l10n-datetime="2014-11-01" data-l10n-id="thumbnail-group-date">

And then have, in the locale file:

  thumbnail-group-date = %B %Y

But for now, I'm hoping that you'll let me get away with just using Date.getFullYear() in all locales.
Attachment #8526406 - Flags: feedback?(gandalf)
Unfortunately, we can't assume that "%B %Y" is the right order.

Which means that until we have L20n file format and date formatting inside l10n syntax, we need to get date format from an entity :(

See: https://mxr.mozilla.org/l10n-gaia/search?string=date-group-header
Flags: needinfo?(gandalf)
(Assignee)

Comment 11

3 years ago
Thanks, Gandalf. I'll modify the patch to use ready() to re-localize those date headers manually. 

I've also noticed that we fail to properly re-localize the "KB", "MB" and other file size indicators, so I'll try to fix that in this patch as well.
(Assignee)

Comment 12

3 years ago
And, of course, the very same date header problem exists in Gallery.
(Assignee)

Comment 13

3 years ago
Wish I could delete comments... The bug was filed for Gallery. I was looking at video, but I've already updated the bug to note that it affects both apps.  The new bit is that the video app also has "MB" and "KB" not localized.
(Assignee)

Comment 14

3 years ago
Created attachment 8529250 [details] [review]
link to patch on github

Ranjith: does this patch solve the bug for you?

Punam: I think you're the best person to review this patch since you did some of the work on the date headers for gallery and video and are also familiar with this bug and with MediaUtils size formatting code.  

For both gallery and video, this patch adds a ready() call in the ThumbnailList class to invoke a localize() method when the locale changes. The localize() method for the thumbanil list calls the localize method for each of the headers, so they can reformat the date. In the video app, the localize method of the group calls the localize method of the thumbnails in the group so they can reformat the size.
Attachment #8526406 - Attachment is obsolete: true
Attachment #8526406 - Flags: feedback?(gandalf)
Attachment #8529250 - Flags: review?(pdahiya)
Attachment #8529250 - Flags: feedback?(ranjith253)
Hi David

Thanks for fixing this issue for both gallery and video app for date and size unit. Attached patch works.

I tried using ready call inside ThumbnailDateGroup and it successfully invoke localize method for each of the headers when the locale changes and when L10N is ready. That means we can remove the ready call from ThumbnailList and consolidate localization changes inside ThumbnailDateGroup class. Please see comments in github for details.
Flags: needinfo?(dflanagan)
Comment on attachment 8529250 [details] [review]
link to patch on github

Cancelling review flag,  please set review flag with your input on suggested updates in comment#15. Thanks
Attachment #8529250 - Flags: review?(pdahiya)
(Assignee)

Comment 17

3 years ago
Comment on attachment 8529250 [details] [review]
link to patch on github

Punam,

I purposely did not follow Gandalf's suggestion of calling ready() for each date header because it seems inefficient to me to register that many callbacks. The thumbnail list class can iterate through the list of headers that need to be updated and it seems wrong to register dozens of callbacks when that is not necessary.

If we take Gandalf's approach to the extreme, then we'd have a ready() callback for each thumbnail in the video app.  And then for the related accessability patch for Gallery, we'd end up with a ready() callback for each thumbnail in that app, and now we're talking about hundreds of references to callbacks and their context instead of just dozens.

So anyway, that is why I think it is worth the inconvenience of the code that checks whether l10n loading is complete: I think we end up with something that is a lot more efficient.
Flags: needinfo?(dflanagan)
Attachment #8529250 - Flags: review?(pdahiya)
(In reply to David Flanagan [:djf] from comment #17)
> Comment on attachment 8529250 [details] [review]
> link to patch on github
> 
> Punam,
> 
> I purposely did not follow Gandalf's suggestion of calling ready() for each
> date header because it seems inefficient to me to register that many
> callbacks.

I totally agree that it is better to have one callback for the whole list, rather than a listener per group.
I did not suggest adding a ready per group, I said that your one ready per list covers all groups.

Re-reading this code, I now think that you may actually want to use the approach you chose because a group may be added after ready() has been fired. right?

If that's the case, then I believe that the patch is good from L10n API standpoint (until we will be able to move datetime formatting into l10n itself)
Comment on attachment 8529250 [details] [review]
link to patch on github

Thanks for clarifying, looks good and has r+. I noticed gaia-try hasn't run or not showing up for this PR and might need to be rebased and pushed again to make it run.
Attachment #8529250 - Flags: review?(pdahiya) → review+
(Assignee)

Comment 20

3 years ago
Landed on master: https://github.com/mozilla-b2g/gaia/commit/90c1b9c146b2a4abe545bf758f2e47d898820ad1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Attachment #8529250 - Flags: feedback?(ranjith253)
You need to log in before you can comment on or make changes to this bug.