Closed
Bug 321381
Opened 17 years ago
Closed 17 years ago
Non-localized strings in the new-views
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jminta, Unassigned)
References
Details
(Keywords: regression)
Attachments
(2 files)
11.19 KB,
patch
|
sipaq
:
first-review-
|
Details | Diff | Splinter Review |
13.41 KB,
patch
|
mostafah
:
first-review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
I will try to fix this. calendar-decorated-week-view.xml is already done
Comment 2•17 years ago
|
||
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•17 years ago
|
||
(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 4•17 years ago
|
||
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)
Reporter | ||
Comment 5•17 years ago
|
||
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-
Updated•17 years ago
|
Attachment #206760 -
Flags: first-review- → first-review?
Updated•17 years ago
|
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 –, 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]);
Updated•17 years ago
|
Attachment #206760 -
Flags: first-review?(jminta) → first-review-
Comment 7•17 years ago
|
||
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.
Reporter | ||
Comment 8•17 years ago
|
||
(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.
Reporter | ||
Comment 9•17 years ago
|
||
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).
Reporter | ||
Comment 10•17 years ago
|
||
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 11•17 years ago
|
||
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+
Reporter | ||
Comment 12•17 years ago
|
||
patch checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 13•17 years ago
|
||
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.
Description
•