321381 - Non-localized strings in the new-views

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Both the regular views and the decorated views have several arrays of strings (month and and day of week names) that are hardcoded.  We need to get out the stringBundle service and get those localized.

Comment 1

[Thorsten 'nightrat' Fritz]

I will try to fix this. calendar-decorated-week-view.xml is already done

Comment 2

[Thorsten 'nightrat' Fritz]

Attachment: First patch

This is a first version of the patch. Because this is my first patch at all, can anyone look over it and tell me if it is good or what i can make better. Thanks.

Comment 3

[Robin Edrenius]

(In reply to comment #2)
> Created an attachment (id=206760) [edit]
> First patch
> 
> This is a first version of the patch. Because this is my first patch at all,
> can anyone look over it and tell me if it is good or what i can make better.
> Thanks.
> 

Looks good to me, I applied it and tried sunbird and everything was working fine (bare in mind though, that I'm no expert either)

You should ask for review (set the first-review flag to '?') from [e-mail] or [e-mail]

Comment 4

[Simon Paquet [:sipaq]]

Comment on attachment 206760 [details] [diff] [review]
First patch

I'm not a competent reviewer for this (jminta is), but nevertheless I have some suggestions to improve this patch.

>--- ./calendar-decorated-day-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-decorated-day-view.xml	2005-12-24 13:37:29.000000000 +0100
>@@ -79,19 +79,26 @@
>[...]
>-                    //XXX l10n love needed
>+                    //XXX l10n love done

Please remove this commment.

>--- ./calendar-decorated-month-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-decorated-month-view.xml	2005-12-24 13:38:28.000000000 +0100
>@@ -110,20 +110,30 @@
>[...]
>-                    //XXX l10n Get me out of here
>+                    //XXX l10n done

And here.

>--- ./calendar-decorated-multiweek-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-decorated-multiweek-view.xml	2005-12-24 11:05:57.000000000 +0100
>@@ -150,18 +150,20 @@
>[...] 
>-                        //XXX l10n love needed
>+                        //XXX l10n love done

And here.

>--- ./calendar-decorated-week-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-decorated-week-view.xml	2005-12-24 10:45:26.000000000 +0100
>@@ -117,18 +117,20 @@
>[...]
>-                        //XXX l10n love needed
>+                        //XXX l10n love done

And here.

>--- ./calendar-month-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-month-view.xml	2005-12-24 13:35:50.000000000 +0100
>@@ -404,25 +404,22 @@
>[...]
>           // XXX aaaaaaaa

This comment should also be removed.

>--- ./calendar-multiday-view.xml	2005-12-23 22:58:41.000000000 +0100
>+++ ../../../../../nightrat-cvs/calendar-multiday-view.xml	2005-12-24 13:34:57.000000000 +0100
>@@ -2215,19 +2215,29 @@
>[...]
>Gemeinsame Unterverzeichnisse: ./CVS und ../../../../../nightrat-cvs/CVS.

This has nothing to do in this patch.

Comment 5

[Joey Minta]

Comment on attachment 206760 [details] [diff] [review]
First patch

Great work!  This looks like a very nice start.

I'll second Simon's call to remove the XXX comments.  (Any time you see an 'XXX' that's a sign that more work needs to be done in the area.  Once that work has been done, the XXX can be removed.)

The main point I want to make, though, is that the areas where this code is being used are going to get called a lot.  Furthermore, they need to be fast.  Since we're only going to use 5 of the months and 5 of the days, we're wasting time if we get all of them and put them in an array.  I'd prefer if we remove the arrays entirely and make some minor changes to the actual label setting lines.  For instance:

Since this corresponds to 'Sunday'
dayViewStringBundle.GetStringFromName( "day.1.name" )];

We can change this
                     var nameArray = new Array();
                     for (var i = -2; i < 3; i++) {
                         nameArray.push(dayNames[(aDate.weekday+i+7)%7]);
                     }

to something like
                     var nameArray = new Array();
                     for (var i = -2; i < 3; i++) {
                         // Add 1 since .weekday == 0 means Sunday, but our
                         // "day.1.name" is Sunday in our strings.
                         var stringName = "day."+(((aDate.weekday+i+7)%7)+1)+".name";
                         var dayName = dayViewStringBundle.GetStringFromName(stringName);
                         nameArray.push(dayViewStringBundle.GetStringFromName(stringName));
                     }

(Note that I have not tested this code, but I think you get the idea.)  Then there is no more array to worry about, and we only have to get 5 names instead of 7.

A similar method can be used for the months.  Looking forward to the next version of this patch. :-)

Comment 6

[gekacheka]

For week numbers: suggest a parameterized string in
calendar.properties so that each language can format it as necessary.
For example, some languages (such as Japanese) do not use spaces, and
may put the unit (week) after the number rather than before.  An
N-dash (html –, not a hyphen) is appropriate between numbers,
and looks better in most fonts.

# Single week number
WeekNumber=Week %1$S
# range of week numbers (\u2013 is same as html –)
WeekNumberRange=Week %1$S\u2013%2$S

Then use 
  var weekRangeString = dateBundle.getFormattedString("WeekNumberRange",
                                                      [fromWeek, toWeek]);

Comment 7

[Thorsten 'nightrat' Fritz]

Thanks Simon and jminta for your review and hints.

I remvoed the comments and edited the code (like the suggestion of jminta) and all works fine.

I also tried to change calendar-month-view.xml and calendar-multiday-view.xml, but it failed, An error occured in the javascript console: aDate is not defined. I have no idea how to define it.

Comment 8

[Joey Minta]

(In reply to comment #7)
> I also tried to change calendar-month-view.xml and calendar-multiday-view.xml,
> but it failed, An error occured in the javascript console: aDate is not
> defined. I have no idea how to define it.
> 
This error occurs if you try to use a variable somewhere where it doesn't exist.  The error should also include a line number, which will tell you exactly where the unknown variable is mentioned.

In this case, I'm guessing you directly copied the code I suggested into calendar-month-view.xml.  This would create an error around:
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#424

Here, instead of using (aDate.weekday+i+7)%7 you need to use this.mIndex, since that will give you the number you need.  Since aDate is never mentioned in this function, it gives the error you mentioned.

A similar problem will happen in calendar-multiday-view.xml:
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#2229

This time you'll see that d.weekday gives the number needed.

Comment 9

[Joey Minta]

What's the current status on this bug?  It might be nice to get it landed before trying to handle things like bug 322060 (though that can be worked around, so it's not an actual blocker).

Comment 10

[Joey Minta]

Attachment: patch v1

This covers all the hardcoded strings I could find.  Mostafah, since you've been doing a fair bit of the l10n work, would you mind reviewing this?

Comment 11

[Mostafa Hosseini]

Comment on attachment 208889 [details] [diff] [review]
patch v1

r=mostafah Looks good.
Tested on sunbird and seamonkey. (Does seamonkey even use these files?)

Comment 12

[Joey Minta]

patch checked in

Comment 13

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!