Closed Bug 340723 Opened 14 years ago Closed 14 years ago

calendar locales file contains #ifdef

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: mattwillis)

Details

(Keywords: l12y)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch just remove the #ifdef (obsolete) — Splinter Review
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)
This was fixed with one of the checkins for bug 267981.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #224763 - Flags: first-review?(mattwillis) → first-review+
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-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Closed: 14 years ago14 years ago
Resolution: --- → WONTFIX
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 → ---
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...
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?
Attachment #225026 - Flags: first-review? → first-review?(kairo)
I checked through all l10n locales having calendar. Only these had been updated to include the #ifdef
Attachment #225027 - Flags: first-review?(kairo)
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+
(In reply to comment #9)
> Which was the bug where you wanted to do the real fix? 

bug 333923
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)
en-US version checked in to MOZILLA_1_8_BRANCH and trunk
Attachment #225027 - Flags: second-review?(cedric.corazza) → second-review+
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.
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+
de checked in on trunk. It doesn't yet exist on branch. 

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 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?
(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.