Closed Bug 321381 Opened 14 years ago Closed 14 years ago

Non-localized strings in the new-views

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Unassigned)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
I will try to fix this. calendar-decorated-week-view.xml is already done
Attached patch First patchSplinter Review
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.
(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 jminta@gmail.com or mvl@exedo.nl
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.
Attachment #206760 - Flags: first-review?(jminta)
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. :-)
Attachment #206760 - Flags: first-review?(jminta) → first-review-
Attachment #206760 - Flags: first-review- → first-review?
Attachment #206760 - Flags: first-review? → first-review?(jminta)
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 &ndash;, 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 &ndash;)
WeekNumberRange=Week %1$S\u2013%2$S

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

Attachment #206760 - Flags: first-review?(jminta) → first-review-
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.
(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.
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).
Attached patch patch v1Splinter Review
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?
Attachment #208889 - Flags: first-review?(mostafah)
Comment on attachment 208889 [details] [diff] [review]
patch v1

r=mostafah Looks good.
Tested on sunbird and seamonkey. (Does seamonkey even use these files?)
Attachment #208889 - Flags: first-review?(mostafah) → first-review+
patch checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
You need to log in before you can comment on or make changes to this bug.