calendar locales file contains #ifdef

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: kairo, Assigned: mattwillis)

Tracking

({l12y})

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
http://lxr.mozilla.org/mozilla/source/calendar/locales/en-US/chrome/calendar/prefs.dtd#57 contains a #ifdef that excludes a section of the file from the build.
If someone (like me) uses a non-Sunbird calendar-en-US.jar file as the base for his L10n, that section will be missing from the L10n and Sunbird will fail to work.
OTOH, if the #ifdef would just not be there, the entities not used in non-Sunbird would just silently be ignored there.

That's some of the reasons why there is a general guideline of never using preprocessor macros in locale files, except in very special cases, see http://developer.mozilla.org/en/docs/Writing_localizable_code#Guidelines

This ifdef should just be removed, there is no harm in having those strings in the file, even if they might not be used everywhere.
(Reporter)

Comment 1

12 years ago
Created attachment 224763 [details] [diff] [review]
just remove the #ifdef

This should work (not tested yet as I don't build Sunbird or Lightning, reviewer please test before granting review), it just removes the #ifdef.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #224763 - Flags: first-review?(mattwillis)
(Assignee)

Comment 2

12 years ago
This was fixed with one of the checkins for bug 267981.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #224763 - Flags: first-review?(mattwillis) → first-review+
(Assignee)

Comment 3

12 years ago
Comment on attachment 224763 [details] [diff] [review]
just remove the #ifdef

This is necessary to avoid entity collisions with Thunderbird in Lightning.
Attachment #224763 - Flags: first-review+ → first-review-
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 4

12 years ago
Upon rereading and testing this, we NEED that ifdef.

I believe the need for this ifdef will disappear after we move to toolkit-based prefs, and after Lightning and Sunbird share some prefs

Since we need the ifdef to prevent the collision, I'm wontfixing this. Like I said, hopefully we'll avoid the need for the ifdef in nuprefs.

-> WONTFIX 
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → WONTFIX

Comment 5

12 years ago
Iff you really need those #ifdefs, please put them in XML comments?

<!--
#ifndef...
-->

That way the DTD will still be standards compatible and can be read by tools,
and even localized, if the tools pay attention to being a good editor. Some do,
potools may actually handle that well.

Note, one way to circumvent the #ifdef in the localization code, which you 
should try to achieve by all means, is to put the #ifdef in the source file
and include a separate DTD by build config containing just these strings.
I didn't fully investigate this, but this seems like a feasible technical
solution.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 6

12 years ago
Just as a note, the translate toolkit (potools) can handle this file as it is
Of course, that's no reason not to improve the situation...
(Assignee)

Comment 7

12 years ago
Created attachment 225026 [details] [diff] [review]
en-US: places the ifdef in comments

Since this dtd will be replaced when toolkit-based preferences land, I'm doing the quick and ugly fix for this only. I'll be sure it's done correctly in the new prefs.
Assignee: kairo → mattwillis
Attachment #224763 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #225026 - Flags: first-review?
(Assignee)

Updated

12 years ago
Attachment #225026 - Flags: first-review? → first-review?(kairo)
(Assignee)

Comment 8

12 years ago
Created attachment 225027 [details] [diff] [review]
l10n: does the same for the two locales that are up-to-date enough to have the ifdef already

I checked through all l10n locales having calendar. Only these had been updated to include the #ifdef
Attachment #225027 - Flags: first-review?(kairo)

Comment 9

12 years ago
Comment on attachment 225026 [details] [diff] [review]
en-US: places the ifdef in comments

I'll do that review.

Which was the bug where you wanted to do the real fix?
Attachment #225026 - Flags: first-review?(kairo) → first-review+
(Assignee)

Comment 10

12 years ago
(In reply to comment #9)
> Which was the bug where you wanted to do the real fix? 

bug 333923
(Assignee)

Comment 11

12 years ago
Comment on attachment 225027 [details] [diff] [review]
l10n: does the same for the two locales that are up-to-date enough to have the ifdef already

Adding Cedric for the fr review. KaiRo remains for the de review unless he wants to give it to someone else.
Attachment #225027 - Flags: second-review?(cedric.corazza)
(Assignee)

Comment 12

12 years ago
en-US version checked in to MOZILLA_1_8_BRANCH and trunk

Updated

12 years ago
Attachment #225027 - Flags: second-review?(cedric.corazza) → second-review+
(Assignee)

Comment 13

12 years ago
fr checked in on MOZILLA_1_8_BRANCH and trunk
lilmatt, i did the german localization of Sunbird and give you a review+ for the de changes.
(Assignee)

Comment 15

12 years ago
Comment on attachment 225027 [details] [diff] [review]
l10n: does the same for the two locales that are up-to-date enough to have the ifdef already

r+ from thorsten
Attachment #225027 - Flags: first-review?(kairo) → first-review+
(Assignee)

Comment 16

12 years ago
de checked in on trunk. It doesn't yet exist on branch. 

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
I just added the commented out #ifdefs to pl in prefs.dtd on trunk and on the branch. 

We based pl calendar localization on our Sunbird 0.3a2 localization (which was based on the content of Sundbird 0.3a2 en-US xpi) and updated it to the current state and put in the l10n CVS directory structure, with compare-locales showing no errors.

Is there anything else that compare-locales won't detect that is different in the 'compiled' en-US Calendar localization and we may have missed it this way? Any more #ifdefs? Or whatever?
(Assignee)

Comment 18

12 years ago
(In reply to comment #17)
> Is there anything else that compare-locales won't detect that is different in
> the 'compiled' en-US Calendar localization and we may have missed it this way?
> Any more #ifdefs? Or whatever?

Not that I know of.

You need to log in before you can comment on or make changes to this bug.