298348 - Lightning needs to be converted to DTDs for localization.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We currently have no l10n story; we don't even really pretend.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This doesn't block 0.8.

Comment 2

[Simon Paquet [:sipaq]]

I'm willing to do this.

Comment 3

[Simon Paquet [:sipaq]]

Attachment: calendar/lightning part - patch v1

Comment 4

[Simon Paquet [:sipaq]]

Attachment: calendar/lightning part - patch v2

Comment 5

[Simon Paquet [:sipaq]]

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.

Comment 6

[Simon Paquet [:sipaq]]

Attachment: calendar/base part - patch v1 (without whitespace changes)

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.

Comment 7

[Simon Paquet [:sipaq]]

Attachment: calendar/base part - patch v1 (with whitespace changes)

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

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 9

[Michiel van Leeuwen (email: mvl+moz@)]

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?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

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!

Comment 11

[Simon Paquet [:sipaq]]

Attachment: calendar/lightning part - patch v3

New patch with review comments addressed. Additionally I added license headers
to all the files I touched.

Comment 12

[Simon Paquet [:sipaq]]

Attachment: calendar/base part - patch v2

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.

Comment 13

[Dan Mosedale (:dmosedale, :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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

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 :)

Comment 16

[Simon Paquet [:sipaq]]

Attachment: calendar/lightning part - patch v4

Patch with last review comment from dmose addressed. I'm carrying over the
review+ from the v3 patch.

Comment 17

[Simon Paquet [:sipaq]]

*** Bug 313759 has been marked as a duplicate of this bug. ***

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 19

[Simon Paquet [:sipaq]]

(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.

Comment 20

[Robin Edrenius]

Attachment: calendar-alarm-dialog.xul hardcode-stuff

Attaching an updated patch from Bug 313759 as per sipaqs request.

Comment 21

[Simon Paquet [:sipaq]]

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.

Comment 22

[Robin Edrenius]

Attachment: calendar-alarm-dialog.xul hardcode-stuff V2

New version reflecting sipaqs comments. Sorry about the whitespace stuff, however, I thought it was prefered to have the lines aligned.

Comment 23

[Simon Paquet [:sipaq]]

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.

Comment 24

[Robin Edrenius]

Attachment: calendar-alarm-dialog.xul hardcode-stuff V3

There you go. This one should do it! =) (I had no idea about the cleanup-in-separate-bug stuff, sorry about that)

Comment 25

[Simon Paquet [:sipaq]]

Comment on attachment 200890 [details] [diff] [review]
calendar-alarm-dialog.xul hardcode-stuff V3

r=sipaq

Comment 26

[Simon Paquet [:sipaq]]

Attachment: Unified patch (lightning and base) for checkin

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).

Comment 27

[Stefan Sitter [:ssitter]]

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?

Comment 28

[Simon Paquet [:sipaq]]

(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.

Comment 29

[Simon Paquet [:sipaq]]

Attachment: Unified patch (lightning and base) for checkin - v2

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Patch checked in; thanks for the work, guys!

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

Why you guys don't follow the source-l10n path like Fx and Tb?

Comment 32

[Simon Paquet [:sipaq]]

(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.

Comment 33

[Robert Kaiser]

This has caused bug 317119