340723 - calendar locales file contains #ifdef

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Robert Kaiser]

Attachment: 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.

Comment 2

[Matthew (lilmatt) Willis]

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

-> FIXED

Comment 3

[Matthew (lilmatt) Willis]

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

This is necessary to avoid entity collisions with Thunderbird in Lightning.

Comment 4

[Matthew (lilmatt) Willis]

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 

Comment 5

[Axel Hecht [:Pike]]

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.

Comment 6

[David Fraser]

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

Comment 7

[Matthew (lilmatt) Willis]

Attachment: 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.

Comment 8

[Matthew (lilmatt) Willis]

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

Comment 9

[Axel Hecht [:Pike]]

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?

Comment 10

[Matthew (lilmatt) Willis]

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

bug 333923

Comment 11

[Matthew (lilmatt) Willis]

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.

Comment 12

[Matthew (lilmatt) Willis]

en-US version checked in to MOZILLA_1_8_BRANCH and trunk

Comment 13

[Matthew (lilmatt) Willis]

fr checked in on MOZILLA_1_8_BRANCH and trunk

Comment 14

[Thorsten 'nightrat' Fritz]

lilmatt, i did the german localization of Sunbird and give you a review+ for the de changes.

Comment 15

[Matthew (lilmatt) Willis]

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

Comment 16

[Matthew (lilmatt) Willis]

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

-> FIXED

Comment 17

[Marek Stępień [:marcoos, inactive]]

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?

Comment 18

[Matthew (lilmatt) Willis]

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