247842 - "Help" in the menu bar should be different between Windows and other platforms

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jérôme Schell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040615 Firefox/0.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040615 Firefox/0.9

For the french localisation we need to have a different "Help" menu between
Windows and Unix/Mac.
In french the help menu is translated in "?" on Windows and "Aide" on Unix and Mac.

You can look at the equivalent bug for Firefox:
http://bugzilla.mozilla.org/show_bug.cgi?id=228163

and its resolution:
http://lxr.mozilla.org/mozilla/source/browser/base/locale/browser.dtd#183

Reproducible: Always
Steps to Reproduce:

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Confirming

Comment 2

[Cédric Corazza]

Attachment: Patch for TB Windows help menu

This patch will allow fr and de locales to have different Help Menu label for Windows as it is already the case for Firefox

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 228534 [details] [diff] [review]
Patch for TB Windows help menu

Without thinking about the UI decision here, the patch misses an #endif.

Comment 4

[Cédric Corazza]

Attachment: Adds the missing #endif (Thanks to Pike)

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 228538 [details] [diff] [review]
Adds the missing #endif (Thanks to Pike)

I'm not really the right one to review this, but here is my assessment. fr and it are the two locales that use this on Firefox on the 1.8 branch. (Despite all comments in code etc, de does not.)
r- based on the fact that the ifdef should really only be around the platform dependent parts, and the menupopup is not. Neither in this patch nor in the corresponding Firefox code.

Scott, if you want to take this, please have the localization note fixed, poor Germans shouldn't take the blame for this. Blame the world champions instead.

Cedric, do you have line ending trouble? It seems like you changing some kind of whitespace around your changes.

Sorry, I didn't really look at the patch in detail the first time around.

Comment 6

[Cédric Corazza]

> Cedric, do you have line ending trouble? It seems like you changing some kind
> of whitespace around your changes.
Axel, if you're talking about this :
-      <!-- Help -->  
+      <!-- Help -->
I removed and extra-space that was on this line

> Scott, if you want to take this, please have the localization note fixed, poor
> Germans shouldn't take the blame for this. Blame the world champions instead.
Sorry about that, I believed the localization note in Firefox's code

> r- based on the fact that the ifdef should really only be around the platform
> dependent parts, and the menupopup is not. Neither in this patch nor in the
> corresponding Firefox code.
I can't argue against that, you're certainly right but :
- why having this done in Firefox ?
- a WONTFIX on this bug would have been clear enough two years ago
- Thunderbird is claimed to be the companion of Firefox so it is weird not to have the same rules for both of them

Regards

Comment 7

[Cédric Corazza]

Axel,
After talking with my fellows, I think I totally misunderstood what you said. Did you mean the code must look like this :

#ifdef XP_WIN
      <menu label="&helpMenuWin.label;" accesskey="&helpMenuWin.accesskey;">
#else
      <menu label="&helpMenu.label;" accesskey="&helpMenu.accesskey;">
#endif
     <menupopup id="menu_HelpPopup" onpopupshowing="buildHelpMenu();">
#ifdef XP_MACOSX
....

Comment 8

[Axel Hecht [:Pike]]

(In reply to comment #7)

Yes. I didn't suggest to WONTFIX this, I'm fine with it either way. I just commented on the fix itself, #ifdef'ing too much, and the localization note.

Comment 9

[Benoit]

> fr and it are the two locales that use this on Firefox on the 1.8 branch. 
> (Despite all comments in code etc, de does not.)

As I understand it from  bug #228163 comment #16, it's a deliberate choice from the de team to _not_ follow the platform conventions. They might change their mind any time, and other locales could be affected. Maybe there shouldn't be any explicit reference to a particular locale in the comment.

BTW, don't always assume language==country, we have short locale names for a reason - in fact "my" local team didn't even qualify for the world cup ;)

Comment 10

[Cédric Corazza]

Attachment: Patch modified regarding last comments

Patch modified according to last comment of Pike (on IRC also). I also removed any specific references to any locale (comment #9 from Benoit) : the concerned locales are aware of that, and maybe we forgot some of them. AFAIK, de, fr and it is concerned.

Comment 11

[Cédric Corazza]

Comment on attachment 228739 [details] [diff] [review]
Patch modified regarding last comments

I forgot to mention the reviewer

Comment 12

[Scott MacGregor]

Comment on attachment 228739 [details] [diff] [review]
Patch modified regarding last comments

This patch looks much better to me now. Adding Axel in case he has anymore concerns.

We should get this into the Thunderbird branch too.

Comment 13

[Scott MacGregor]

Comment on attachment 228739 [details] [diff] [review]
Patch modified regarding last comments

approving for thunderbird 2

Comment 14

[Scott MacGregor]

Gavin usually likes to help checkin patches for folks :)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

mozilla/mail/locales/en-US/chrome/messenger/messenger.dtd 1.39
mozilla/mail/base/content/mailWindowOverlay.xul 1.157
mozilla/mail/locales/en-US/chrome/messenger/messenger.dtd 1.17.2.21
mozilla/mail/base/content/mailWindowOverlay.xul 1.116.2.34