Closed Bug 298348 Opened 15 years ago Closed 14 years ago

Lightning needs to be converted to DTDs for localization.

Categories

(Calendar :: Lightning Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Lightning 0.1

People

(Reporter: shaver, Assigned: sipaq)

References

Details

Attachments

(1 file, 11 obsolete files)

64.80 KB, patch
Details | Diff | Splinter Review
We currently have no l10n story; we don't even really pretend.
This doesn't block 0.8.
No longer blocks: lightning-0.1
Depends on: 298500
I'm willing to do this.
Assignee: shaver → bugzilla
QA Contact: shaver → nobody
Status: NEW → ASSIGNED
Attachment #198490 - Flags: first-review?(dmose)
Target Milestone: --- → Lightning 0.1
Attachment #198490 - Attachment is obsolete: true
Attachment #198491 - Flags: first-review?(dmose)
Attachment #198490 - Flags: first-review?(dmose)
Just for the record:
I don't really know how to localize the stuff in

http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-month-view.xml#382
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-multiday-view.xml#2132

so the patches, I will attach here, will not cover this stuff.
Ok, here's the patch for the calendar/base part. 

Three additional notes:

1. The patch contains all the changes to calendar.dtd that are also in the
first 
   patch. 
2. It also contains a change in resources/content/eventDialog.xul. If that 
   change is okay, it will probably eliminate some logic in eventDialog.js. But

   since I'm not a qualified JS coder, I didn't mess with that file.
3. This patch is without the whitespace changes I applied to calendar.dtd for 
   better readability. I will attach a patch for checkin purposes, which has
all 
   the whitespace changes.
Attachment #198914 - Flags: second-review?(mvl)
Attachment #198914 - Flags: first-review?(shaver)
Don't do whitespace changes in .dtd files, at least not without talking to
mostafah. iirc, he uses some scripts, and they don't like random whitespace changes.
And if you want such major changes, do it in a different bug, to keep cvs blame
a bit sane.
Comment on attachment 198914 [details] [diff] [review]
calendar/base part - patch v1 (without whitespace changes)

>Index: base/content/calendar-event-dialog.xul

>-  title="Edit Item"
>+  title="&event.title.new;"
You are switching from 'Edit Item' to 'New Event'. Why?

>-            <label value="Attendees" class="label"/>
>+            <label value="&newevent.attendees.tab.label;" class="label"/>
Don't reuse a tab label for somethnig that isn't a tab.

>-            <label value="Completed" class="label"/>
>+            <label value="&newevent.status.completed.label;" class="label"/>
Again, use something like newevent.completed.label. Be consistent.

>-            <label value="Repeat" class="label"/>
>+            <label value="&newevent.repeat.label;" class="label"/>

Switching from 'Repeat' to 'Repeat Every'. Why?

>Index: resources/content/eventDialog.xul
>                 <menuitem label="&repeat.units.days;"
>-                          label1="&repeat.units.days.singular;"
>-                          label2="&repeat.units.days;"
>                           id="repeat-length-days"
>                           value="DAILY" />

If you make this change, you should also change the javascript. I expect it do
die without the label1 and label2 defined.

>Index: resources/locale/en-US/calendar.dtd
>+<!ENTITY repeat.units.days    "Day(s)" >
>+<!ENTITY repeat.units.weeks   "Week(s)" >
>+<!ENTITY repeat.units.months  "Month(s)" >
>+<!ENTITY repeat.units.years   "Year(s)" >

Are you sure a trick like this exists in all languages?
Attachment #198914 - Flags: second-review?(mvl) → second-review-
Comment on attachment 198491 [details] [diff] [review]
calendar/lightning part - patch v2

Looks good; just a few minor tweaks required.

>Index: calendar/resources/locale/en-US/calendar.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/resources/locale/en-US/calendar.dtd,v
>retrieving revision 1.111
>diff -u -6 -r1.111 calendar.dtd
>--- calendar/resources/locale/en-US/calendar.dtd	22 Sep 2005 01:51:04 -0000	1.111
>+++ calendar/resources/locale/en-US/calendar.dtd	4 Oct 2005 20:03:14 -0000
>@@ -263,13 +263,15 @@
> <!ENTITY calendar.gototoday.button.tooltip        "Go to today" >
> <!ENTITY calendar.choosedate.button.tooltip       "Choose date to go to" >
> 
> <!ENTITY calendar.dayview.button.tooltip          "Switch to day view" >
> <!ENTITY calendar.weekview.button.tooltip         "Switch to week view" >
> <!ENTITY calendar.monthview.button.tooltip        "Switch to month view" >
>-<!ENTITY calendar.multiweekview.button.tooltip        "Switch to multiweek view" >
>+<!ENTITY calendar.multiweekview.button.tooltip    "Switch to multiweek view" >

As mvl said, please check with Mostafa about whitespace changes here (or just
get rid of the whitespace change).

>Index: calendar/lightning/locale/lightning.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/lightning/locale/lightning.dtd,v
>retrieving revision 1.1
>diff -u -6 -r1.1 lightning.dtd
>--- calendar/lightning/locale/lightning.dtd	15 Feb 2005 21:45:45 -0000	1.1
>+++ calendar/lightning/locale/lightning.dtd	4 Oct 2005 20:03:28 -0000
>@@ -1,2 +1,27 @@
> <!-- Tools menu -->
> <!ENTITY lightning.taskLabel "Lightning">
>+
>+<!-- Agenda Tree View -->
>+<!ENTITY agenda.treeview.label           "View:">
>+<!ENTITY agenda.treeview.menu.label      "View">
>+<!ENTITY agenda.treeview.all.label       "All">
>+<!ENTITY agenda.treeview.events.label    "Events">
>+<!ENTITY agenda.treeview.tasks.label     "Tasks">
>+<!ENTITY agenda.treeview.reminders.label "Reminders">
>+<!ENTITY agenda.treeview.customize.label "Customize...">
>+<!ENTITY agenda.treeview.item.label      "Item">
>+<!ENTITY agenda.treeview.label           "Customize your views!">

agenda.treeview.label has already been used; I'm betting this needs a different
ENTITY name.  Also, can you drop the exclamation point in the text?  The XUL
overlay has the same problem with the re-use of the ENTITY name.

Instead of "messenger.", can we use "lightning." as a prefix?

>+<!-- Messenger Sidebar -->
>+<!ENTITY messenger.toolbar.dayview.label     "Day View">
>+<!ENTITY messenger.toolbar.weekview.label    "Week View">
>+<!ENTITY messenger.toolbar.monthview.label   "Month View">

I bet that comment wants to read "Toolbar" instead of "Sidebar".

Thanks for the patch!
Attachment #198491 - Flags: first-review?(dmose) → first-review-
Attachment #198914 - Flags: first-review?(shaver)
New patch with review comments addressed. Additionally I added license headers
to all the files I touched.
Attachment #198491 - Attachment is obsolete: true
Attachment #199655 - Flags: first-review?(dmose)
Attached patch calendar/base part - patch v2 (obsolete) — Splinter Review
Patch with review comments addressed. I only moved the hardcoded strings to
entities in calendar.dtd. Don't blame me if it's not pretty :-)

I chose to leave resources/content/eventDialog.xul alone.
And like the lightning patch I added a license header to files I touched, when
there was none.
Attachment #198914 - Attachment is obsolete: true
Attachment #198915 - Attachment is obsolete: true
Attachment #199688 - Flags: second-review?(mvl)
Attachment #199688 - Flags: first-review?(dmose)
Comment on attachment 199655 [details] [diff] [review]
calendar/lightning part - patch v3


>+<!ENTITY agenda.treeview.description     "Customize your views">

How about making this just "Customize views"?  Using a personal pronoun in the
UI sounds kind of unusual.  Sorry for not noticing this last time.  

Change that, and you've got r=dmose.
Attachment #199655 - Flags: first-review?(dmose) → first-review+
Comment on attachment 199688 [details] [diff] [review]
calendar/base part - patch v2

r=dmose.  Note that I didn't review the text tweaks themselves closely; it
makes sense to me to land this without really worry ing about those, and let
folks file new bugs on them if there really are issues.
Attachment #199688 - Flags: first-review?(dmose) → first-review+
Comment on attachment 199688 [details] [diff] [review]
calendar/base part - patch v2

Looks good. I trust dmose's review. I didn't check everything, so i can't say i
reviewed it. Lets say I ok-ed it :)
Attachment #199688 - Flags: second-review?(mvl)
Patch with last review comment from dmose addressed. I'm carrying over the
review+ from the v3 patch.
Attachment #200428 - Flags: first-review+
*** Bug 313759 has been marked as a duplicate of this bug. ***
Attachment #199655 - Attachment is obsolete: true
Unfortunately, there was a bit of a mixup here: I somehow was confused and thought you had checkin privs.  As a result, I didn't realize you were expecting me to land them until Joey pinged me yesterday.  So I just tried applying these patches to my local tree and found a couple of issues:

a) The base patch doesn't actually apply because the tree has changed out from under it.  Can you generate a new patch for that one?

b) The lightning patch has multiple XUL files with JS/C/C++-style header boilerplate added, which means that the XML parser can't parse them at all, so it needs a new version as well.  In the future, if you could test patches before requesting review, that would be much appreciated.

Anyway, thanks again for the patch, and sorry for the delay.
(In reply to comment #18)
> Unfortunately, there was a bit of a mixup here: I somehow was confused and
> thought you had checkin privs.

You aren't the first to think that :-)

> a) The base patch doesn't actually apply because the tree has changed out from
> under it.  Can you generate a new patch for that one?

Expect a new patch on Friday or Saturday.

> b) The lightning patch has multiple XUL files with JS/C/C++-style header
> boilerplate added

Ups, sorry for that. I somehow mixed up the commenting style. I'll post a new patch on Friday or Saturday.

> In the future, if you could test patches before requesting review, that would 
> be much appreciated.

I just tested the base patch and didn't bother to test the lighning patch. My fault, won't happen again. Sorry for the hassle.

> Anyway, thanks again for the patch, and sorry for the delay.

No problem. You're much faster than other reviewers in other parts of the tree.
Attaching an updated patch from Bug 313759 as per sipaqs request.
Attachment #200871 - Flags: first-review?(bugzilla)
Comment on attachment 200871 [details] [diff] [review]
calendar-alarm-dialog.xul hardcode-stuff

>Index: mozilla/calendar/resources/locale/en-US/calendar.dtd
>===================================================================
>
>+<!ENTITY calendar.alarm.dismiss.label               "Dismiss" >
>+<!ENTITY calendar.alarm.dismissall.label            "Dismiss All" >
> <!ENTITY calendar.alarm.snooze.label                "Snooze" >
>-<!ENTITY calendar.alarm.title.label                "Calendar Alarm" >
>+<!ENTITY calendar.alarm.title.label                 "Calendar Alarm" >

The calendar.dtd portion of your patch still contains string changes, which apply only to calendar-alarm-widget.xml, which should not be covered by this patch.

Also don't do random whitespace changes (see comment 8 and comment 10). Please post an updated patch.
Attachment #200871 - Flags: first-review?(bugzilla) → first-review-
New version reflecting sipaqs comments. Sorry about the whitespace stuff, however, I thought it was prefered to have the lines aligned.
Attachment #200871 - Attachment is obsolete: true
Attachment #200888 - Flags: first-review?(bugzilla)
Comment on attachment 200888 [details] [diff] [review]
calendar-alarm-dialog.xul hardcode-stuff V2


>Index: mozilla/calendar/resources/locale/en-US/calendar.dtd
>===================================================================
>
> <!-- Calendar Alarm Dialog -->
> <!ENTITY calendar.alarm.acknowledge.label           "Acknowledge" >
> <!ENTITY calendar.alarm.editevent.label             "Edit Event" >
>-<!ENTITY calendar.alarm.acknowledgeall.label        "Acknowledge All Alarms" >
>+<!ENTITY calendar.alarm.dismissall.label            "Dismiss All" >
> <!ENTITY calendar.alarm.snooze.label                "Snooze" >
> <!ENTITY calendar.alarm.title.label                "Calendar Alarm" >

Better! But please leave in calendar.alarm.acknowledgeall.label for now. We should wait with cleanup work until after 0.3a1 and also this bug is not the right place for cleanup work.

Cleanup work should happen in a separate bug.
Attachment #200888 - Flags: first-review?(bugzilla) → first-review-
There you go. This one should do it! =) (I had no idea about the cleanup-in-separate-bug stuff, sorry about that)
Attachment #200888 - Attachment is obsolete: true
Attachment #200890 - Flags: first-review?(bugzilla)
Comment on attachment 200890 [details] [diff] [review]
calendar-alarm-dialog.xul hardcode-stuff V3

r=sipaq
Attachment #200890 - Flags: first-review?(bugzilla) → first-review+
Ok, here's the final patch for checkin purposes, which applies to the current state of the trunk.

It combines the lightning patch, the base patch and the additional patch for calendar-alarm-dialog.xul from Robin Edrenius.

I've changed all the C++ comments to XML style comments. I added myself to the contributors section where I thought it would be warranted (exception: calendar-alarm-dialog.xul, where I added Robin).
Attachment #199688 - Attachment is obsolete: true
Attachment #200428 - Attachment is obsolete: true
Attachment #200890 - Attachment is obsolete: true
In mozilla/calendar/resources/content/calendarProperties.xul the string "Read-only" is hard-coded too. Is this the proper bug to solve this problem? Or do we need a new one?
(In reply to comment #27)
> In mozilla/calendar/resources/content/calendarProperties.xul the string
> "Read-only" is hard-coded too. Is this the proper bug to solve this problem? Or
> do we need a new one?

Stefan, this is not a Lightning issue. Please file a new bug.
Attachment #201327 - Attachment is obsolete: true
Patch checked in; thanks for the work, guys!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Why you guys don't follow the source-l10n path like Fx and Tb?
(In reply to comment #31)
> Why you guys don't follow the source-l10n path like Fx and Tb?

Gandalf, please see bug 281935 and bug 267981 for our ongoing work on that issue. This bug here does in no way relate to that issue.
This has caused bug 317119
QA Contact: nobody → lightning
You need to log in before you can comment on or make changes to this bug.