The ::QuitRequested method always issues a CLOSEWINDOW event. This is a problem when the ui.key.accelKey is the same as the BeOS shortcut key, and you are using tabs. The "close tab" key sequence is "accelKey + W", and the BeOS default Quit key-sequence is "shortcutkey + W", so, if you go to close a tab, the whole window closes. Though, simply not issuing the CLOSEWINDOW event will not work, because then other apps, i.e. the deskbar, cannot issue B_QUIT_REQUESTED messages, and have the window be closed. So, what we need to do is somehow determine if the B_QUIT_REQUESTED was a result of a key sequence, or a message generated by another app. This may require a "hack" to get it to work properly.

-> arougthopher@lizardland.net

Does Mozilla require unified shortcuts for all platforms? If there is regular way to define BeOS-specific shortcuts, i rather prefer such way.

Works here - Alt+w closes app only if last tab in last window.

We also need to handle close events external to Mozilla, i.e. from the Tracker. I am working on this now.

Are you sure, that this patch may stay on way of Tracker requests? It filters only Mozilla internal key-pressings.

Actually, all we need to do, for now, is just always return false for nsWindowBeOS::QuitRequested. The problem with that, however, is if the user has the alt key as the accel key in BeOS, and the ctrl key as the modifier key in mozilla, alt-w won't close the window, but, if you look at the shortcuts under the File menu in this scenario, ctrl-w is supposed to close the window, not alt-w. We must remember, this is not a native app, and, we will not always be able to make it act as one, though, I have an idea to help fix this: At the app startup, examine the current keymap used by BeOS, to see what the accel key is (ctrl or alt), and set the user preference accordingly. As for closing from tracker, in your build, go to the DeskBar, and select "Close All" for the mozilla-bin application. Mozilla does not quit. What is happenning is Deskbar/Tracker is sending the B_QUIT_REQUESTED message to the application. However, at least on my build, the BApplication object in mozilla does not seem to communicate well with its BWindows. I have a feeling this is because the BApplication object's thread is "spawned", instead of using the main thread's thread, in a normal, native application. This is also why I think I am running into problems with the ArgvReceived method in BApplication, and, am looking into a way to resolve it.

Well, about B_QUIT_REQUESTED and ArgsReceived, - as i wrote in comment to other bug, i implemented hook in BWindow derivative, and it works ok. And definitely for dropping and scripting (sending args) it has sense - because if we drop something on window, we excpect that it be opened in THAT window. What about B_QUIT - it also can be hooked in BWindow - and then resent to be_app. I did that once in BeAndSee program when fixed that crash-on-exit hell. But there is another alternative - if you read recent BeDevTalk about "undocumented" B_QUIT_ON_WINDOW_CLOSE flag. In this case we need to determine message sender in some way.

Sergei, don't you mean RefsReceived? That's what's called when you drop something. ArgsReceived is called when the app is already running, and someone runs it a second time from the command line. Also, RefsReceived needs to be implemented to be able to open a file from tracker, w/o dragging (i.e. Open With ...) Regardless, as you stated, there is a problem sending reliably from the BApplication to the windows, hence, the work around you spoke of.

btw, this bug and bug 157348 should form BeZilla "meta"-bug:) - "how to separate B_CONTROL, B_COMMAND, B_OPTION etc from Mozilla's isAlt, isMeta, isControl" - as I said, we cannot be sure that we don't meet again in future some confusion like this 'w'

Comment on attachment 91296 [details] [diff] [review] Patch - prevent closing app on Mozilla's close tab/window shortcut Well, I tried many other ways to determine how QuitRequested was called, be it key sequence, window close button, or from an outside call (i.e. deskbar). It seems that there is no one tried and true way to do this, except how you have done it (or at least that I could find :) The trunk is frozen. Sergei, you will need to ask drivers for approval to check this in.

Comment on attachment 91296 [details] [diff] [review] Patch - prevent closing app on Mozilla's close tab/window shortcut a=asa (on behalf of drivers) for checkin to 1.1

checked in

*** Bug 280367 has been marked as a duplicate of this bug. ***

Problem still appears if user presses Cmd+W twoce with very little interval between presses. Discussing reopening

How about the same style of hack, but instead of a boolean value we use an integer that is incremented when the key is pressed and decremented when the message is processed? I've managed to reproduce the bug (when my CPU is maxed out compiling :)) and this seems to fix it.

Ignore the last idea, this is a much better way of doing it. This patch hooks in a bit further up (BWindow::DispatchMessage), and doesn't call the base class version (which is what calls QuitRequested) at all. This patch also removes the previous hacky method completely. I see no reason not to get a quick review and commit on this one.

Comment on attachment 173530 [details] [diff] [review] Modified hack Obselete earlier idea

Comment on attachment 173539 [details] [diff] [review] Better patch >+ if((!strcmp(bytes.String(),"w")) || (!strcmp(bytes.String(),"W"))) >+ AltW = true; > } > } >- BWindow::DispatchMessage(msg, handler); >+ //Take the default action unless Alt-W pressed >+ if(!AltW) >+ BWindow::DispatchMessage(msg, handler); Why don't you just simply write if((strcmp(bytes.String(),"w")||(strcmp(bytes.String(),"W") BWindwow::DispatchMessage(msg, handler); ? This would save the cost of adding a boolean.

Re-opening. Thanks for working on this Simon. I'm CCing you, as you might have missed Ludovic Hirlimann's regarding your patch. Prog.

Two notices 1)I wish Prognathous test build with that patch before i put review. Also for possible side effect 2)Please notice, that in BeOS Cmd+W closes window disregarding not only register, but also keymap. So if keymap is cyrillic or hebrew, pressing Cmd + "key which generates w in lating keymap" also should close window. So this patch must be tested for 2) before reviewing. Also, if you still use bool variable, rename please it to CmdW. AltW is confusing name - ask Prognathous - why (i'm but tired to explain it again and again:)

(In reply to comment #18) > Why don't you just simply write > if((strcmp(bytes.String(),"w")||(strcmp(bytes.String(),"W") > BWindwow::DispatchMessage(msg, handler); > > ? > This would save the cost of adding a boolean. Because "bytes" is only initialised when the message is about a KeyDown (look at the scoping) - DispatchMessage also handles other types of messages. I suppose bytes could be declared before all the ifs, but a BString constructor is probably more overhead (for the cases where the message is not a KeyDown) than a bool is.

(In reply to comment #20) > 1)I wish Prognathous test build with that patch before i put review. Also for > possible side effect I'll test it as soon as I get back home, to my BeOS machine. Actually, I'm very excited and happy to see this fixed! > Also, if you still use bool variable, rename please it to CmdW. AltW is > confusing name - ask Prognathous - why (i'm but tired to explain it again and > again:) I'm sure Simon knows the difference between Cmd and Alt. The fact the x86 BeOS uses Alt as Cmd just makes this a common trap, especially for those who choose not to change the system's default pref. Prog.

> 2)Please notice, that in BeOS Cmd+W closes window disregarding not only > register, but also keymap. So if keymap is cyrillic or hebrew, pressing > Cmd + "key which generates w in lating keymap" also should close window. I've just tried this out (swtiched to Russian Keymap) and the Alt-W combination didn't close any app windows (StyledEdit, Tracker windows, BeMail, mozilla tabs). In fact no shortcuts seemed to work at all. So it seems the patch is fine in that respect. > Also, if you still use bool variable, rename please it to CmdW. AltW is > confusing name - ask Prognathous - why (i'm but tired to explain it again and > again:) Yes, sorry. I understand the difference and will change it if the patch works for Prog (I'm 99% sure that it will though). The previous patch attempted to do exactly the same thing - ignore CLOSEWINDOW events if Cmd-W was pressed. This patch just prevents those events happening at all after that key combination.

"Switched to Russian Keymap) and the Alt-W combination didn't close any app windows" Yeah, that is:(( I forgot that i investigated this thing somewhere in year 2000 - btw, this is VERY annoying BeOS bug/issue. As all other shortcuts (as Cmd+C/Cmd+W) tend also to be non-functional. I fixed it for myself by creating proper keymap for russian, and thus forgot this idiotic BeOS issue.

As I wrote at blog: "The window doesn't close if I press Cmd+W multiple times, and this is a huge improvement for me. I don't see any regressions, the only issue that I have with pressing Cmd+W multiple times is that when closing tabs with content, the process sometimes takes long enough on this P3-450 that the browser seems to ignore the additional Cmd+W presses (that is, two quick presses only close one tab, the current one). It doesn't always happen, and it never happens with blank tabs. My initial impression is that this is not going to be a significant problem, but I'll probably need to surf more with it to be sure. Thanks again, Prog."

OK, hopefully this is the final patch. I have removed the extra bool from the method now. The idea is the same as before. Prog's issues with events being ignored is likely to be an event queue problem, nothing to do with this patch.

Comment on attachment 173632 [details] [diff] [review] Final patch (I hope) r=sergei_d

BeOS-only code in BeOS-specific folder. Have you got CVS write access to get this committed Sergei?

"Have you got CVS write access to get this committed Sergei?" Yes and no, yes - in theory, no - in reality. I have problems with that since mozilla.org moved to cvs over ssh system. wasted whole day today in next attempt to sort it out. With help of Christian Biesinger we get partial success, at least i can now log in with ssh to cvs.mozilla.org. But cvs over ssh seems to behave very weird. I think that our ports of ssh (or maybe cvs) suxx heavily. Or maybe BeOS itself with its indistinctive equivalence of stdout and stderr

(In reply to comment #29) > "Have you got CVS write access to get this committed Sergei?" > > Yes and no, > yes - in theory, > no - in reality. > I have problems with that since mozilla.org moved to cvs over ssh system. > > wasted whole day today in next attempt to sort it out. > With help of Christian Biesinger we get partial success, at least i can now log > in with ssh to cvs.mozilla.org. > > But cvs over ssh seems to behave very weird. > > I think that our ports of ssh (or maybe cvs) suxx heavily. > Or maybe BeOS itself with its indistinctive equivalence of stdout and stderr Ah I see. Axel mentions on the Haiku homepage that they will be providing a new ssh for R5 for their move over to Subversion - hopefully that one will work for CVS too. I'll send an email to the Haiku list to see if there is a build of that ssh already made.

patch is landed - 2005-02-10 10:5 rev 1.89 though, in attempt to use StyledEdit instead vi for comments i lost description. So looks like "dummy" checkin. and i haven't cvs admin's rights to add description now.

checkin comment must be: "bug 155702 fix. r=sergei_d. a=mkaply. BeOS-only, no sr required" as tree was frozen for 1.8beta "release" and i got approval from mkaply

Is this really reopened, and why? Or should it be set to fixed?

Too bad about that comment...

Remember the old problem of "Pressing Cmd+W twice closes the whole window (rather than just the last two tabs)"? Well, either it's back as a regression, or this patch didn't make it into the build I'm using: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.8b2) Gecko/20050514 Firefox/1.0+ Since opening that bug, I switched to a faster computer, where this problem is less obvious. Nevertheless, it's still easy to reproduce if you follow the instructions in Bug 280367, Comment #0. Re-opening bug. Prog.

No, it's fixed. As that is an experimental build to test the drag'n'drop it has an older nsWindow because I have a lot D'n'D code in it. This CVS-change happened after the nsWindow I started working on.