90524 - The menu for the turbo systray icon doesn't work on Windows 9x/ME

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[Blake Ross]

Comment 1

[Blake Ross]

Attachment: patch

Comment 2

[Blake Ross]

Simple patch. AppendMenuW is for wide-character strings, which won't work on 
9x-based systems.

mscott, sr?

Comment 3

[Viswanath Ramachandran]

Would this affect intl builds, i.e. if the string is not ASCII, would it show up 
right in the menu? 

Also similar to LoadImageW vs LoadImage code used in
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#4559

shd we be trying the Wide version first and if it doesnt work, use the non-wide 
version?

Comment 4

[Blake Ross]

Attachment: Yeah, better.

Comment 5

[:deanis74]

Not that it makes much of a difference, but can you call AppendMenuW() only once 
and then proceed from there depending on the return status?  I'm just thinking 
that if we have ten items in this menu we'll have nine extra calls to 
AppendMenuW() and then an additional ten calls to AppendMenu().

Comment 6

[Blake Ross]

Yeah, I considered that. It doesn't matter too much; it's not as if speed is 
really critical here (especially with only two menuitems), and it would just 
make it a bit harder to read.

Comment 7

[Todd Pringle]

Marking PDT+ as per today's meeting.  Blake, in order to take this fix we really
need a solution that can be localized on all platforms - Win98 included.  Frank
- any ideas?

Comment 8

[Frank Tang]

NS_ConvertUCS2toUTF8(openText).get() won't work. We should do the following
1. clone the GetACPString from nsWindow.cpp as below

4502 static char* GetACPString(const nsString& aStr)
4503 {
4504    int acplen = aStr.Length() * 2 + 1;
4505    char * acp = new char[acplen];
4506    if(acp)
4507    {
4508       int outlen = ::WideCharToMultiByte( CP_ACP, 0, 
4509                       aStr.get(), aStr.Length(),
4510                       acp, acplen, NULL, NULL);
4511       if ( outlen >= 0)
4512          acp[outlen] = '\0';  // null terminate
4513    }
4514    return acp;
4515 }

2. and change the two lines in your patch

+            ::AppendMenu( mTrayIconMenu, MF_STRING, TURBO_OPEN,  
NS_ConvertUCS2toUTF8(openText).get() );
+            ::AppendMenu( mTrayIconMenu, MF_STRING, TURBO_QUIT, 
NS_ConvertUCS2toUTF8(exitText).get() );

to the following 10 lines

char* openACPText = GetACPString(openText.get());
if (openACPText ) {
  ::AppendMenu( mTrayIconMenu, MF_STRING, TURBO_OPEN,  openACPText  );
  delete [] openACPText ;
}
char* quitACPText = GetACPString(quitText.get());
if (quitACPText ) {
  ::AppendMenu( mTrayIconMenu, MF_STRING, TURBO_QUIT,  quitACPText  );
  delete [] quitACPText ;
}

Comment 9

[Frank Tang]

The localized OS does not expect UTF8, they expect charset in ACP. so we need to 
convert from UCS2 to ACP (ANSI Code Page) but not UTF8. 

Comment 10

[Blake Ross]

Frank: thanks! Off to try this now.

Comment 11

[Blake Ross]

Attachment: Frank's proposed patch

Comment 12

[Blake Ross]

Frank, I made one change to your patch; I believe your passing |foo.get()| to 
GetACPString was a typo, since it takes an nsString.

This certainly works fine for me on Windows 2000; can you get someone to test on 
Windows 9x with i18n characters?  Naoki?

Comment 13

[timeless]

so you're not going to use the Wide version ever?  this is very expensive on 
windows nt because you're converting from Wide to Narrow to Wide.

*bad*. scc does the ACP stuff look ok? [could the function take 
an nsAReadableString ?]

fwiw there are examples in extra.c which might be useful (although i'm starting 
to dislike some of its code...)

if (hLib = LoadLibraryEx("user32.dll", NULL, LOAD_WITH_ALTERED_SEARCH_PATH)))
 if (GetProcAddress(hLib, "AppendMenuW"))
 else ...

Comment 14

[Blake Ross]

Speed is not a huge issue here.  We're not trying to make the world's fastest 
popup menu with two items; we're looking for the lowest risk patch.

Yes, we could attempt to do wide-character string appends first if you think it 
actually matters.

Comment 15

[:deanis74]

Let's skin the menu -- just kidding!!!

I don't think we're going to run into speed issues. As I alluded to in my 
2001-07-12 comment, I think that if we add a ton more items to the menu we 
should maybe re-visit the order that things are done in. But for two items, this 
seems fine.

Comment 16

[timeless]

considering the number of bugs asking for more menu items in the system tray i 
think we should address that pretty soon, a quick fix that supports Wide and 
w95osr0 should be implemented... if you need help and i'm awake, pageme.

Comment 17

[nhottanscp]

I think Frank's suggestion was for the case
if (::GetLastError() == ERROR_CALL_NOT_IMPLEMENTED)
When AppendMenuW is available, that should be used.

Comment 18

[nhottanscp]

I can apply the patch and change the property file and find machine to test.
How can I enable the feature? If I just lanch, the tray icon does not show up.

Comment 19

[Blake Ross]

Attachment: And now, the patch you've all been waiting for...

Comment 20

[Blake Ross]

Naoki: run netscp6.exe -turbo

Comment 21

[nhottanscp]

The patch doesn't compile with the trunk.
My branch build is not up to date and it won't show the tray even on NT.
I am pulling the branch now.

Comment 22

[nhottanscp]

I pulled the branch but it crashes when I launch with -turbo option.
Please change profileManager.properties as below and put the build somewhere so
we can test.
startButton=Start\u3042 %brandShortName%
exitButton=Exit\u3044

Comment 23

[Viswanath Ramachandran]

where does it crash?

Comment 24

[nhottanscp]

Attachment: Screen shot of the tray on Win98 JA with the localized property and the patch.

Comment 25

[nhottanscp]

When I have multiple profiles, it asserts at the code below in nsAppRunner.cpp
and quits on my branch debug build. I don't see it with commercial release build.
        // If we are in server mode, profile mgr cannot show UI
        rv = profileMgr->StartupWithArgs(cmdLineArgs, !IsAppInServerMode());
        NS_ASSERTION(NS_SUCCEEDED(rv), "StartupWithArgs failed\n");
        if (NS_FAILED(rv)) return rv;

Comment 26

[nhottanscp]

The patch is working fine on Win98 JA with localized strings (see the screen
shot) as long as there is only have one profile on the machine.

Comment 27

[Blake Ross]

Yeah, the assertion is expected; that's what happens when you have multiple
profiles.

Looking for an r= on this patch.

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

alright. sr=[e-mail]

Comment 29

[Blake Ross]

Fix checked in.

Comment 30

[Blake Ross]

Reopening, I forgot to checkin on the trunk.

Removing PDT+ so this doesn't show up on radar, but please remember to verify
this on win98 Paul.

Comment 31

[Scott MacGregor]

adding vbranch keyword to make sure this shows up on paw's list of branch bugs
to QA.

Comment 32

[Viswanath Ramachandran]

nav triage: Blake could you check this in on the trunk and close the bug! thanks!

Comment 33

[Blake Ross]

Fixed on trunk.