155702 - nsWindowBeOS::QuitRequested always issues CLOSEWINDOW event

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Paul]

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.

Comment 1

[Matthias Versen [:Matti]]

-> [e-mail]

Comment 2

[Sergei Dolgov]

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

Comment 3

[Sergei Dolgov]

Attachment: Patch - prevent closing app on Mozilla's close tab/window shortcut

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

Comment 4

[Paul]

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

Comment 5

[Sergei Dolgov]

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

Comment 6

[Paul]

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.

Comment 7

[Sergei Dolgov]

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.

Comment 8

[Paul]

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.

Comment 9

[Sergei Dolgov]

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 10

[Paul]

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 11

[Asa Dotzler [:asa]]

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

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 13

[Sergei Dolgov]

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

Comment 14

[Sergei Dolgov]

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

Comment 15

[Simon]

Attachment: Modified hack

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.

Comment 16

[Simon]

Attachment: Better patch

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 17

[Simon]

Comment on attachment 173530 [details] [diff] [review]
Modified hack

Obselete earlier idea

Comment 18

[Ludovic Hirlimann [:Usul]]

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.

Comment 19

[Prognathous]

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

Prog.

Comment 20

[Sergei Dolgov]

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

Comment 21

[Simon]

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

Comment 22

[Prognathous]

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

Comment 23

[Simon]

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

Comment 24

[Sergei Dolgov]

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

Comment 25

[Prognathous]

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

Comment 26

[Simon]

Attachment: Final patch (I hope)

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 27

[Sergei Dolgov]

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

r=sergei_d

Comment 28

[Simon]

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

Comment 29

[Sergei Dolgov]

"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

Comment 30

[Simon]

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

Comment 31

[Sergei Dolgov]

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.

Comment 32

[Sergei Dolgov]

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

Comment 33

[tqh]

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

Comment 34

[Niels Reedijk]

Too bad about that comment...

Comment 35

[Prognathous]

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.

Comment 36

[tqh]

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.