GNU bug report logs - #3303
delete-frame raises old (invisible) frame

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 3303 in the body.
You can then email your comments to 3303 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: emacs-pretest-bug <at> gnu.org
Subject: delete-frame raises old (invisible) frame 
Date: Fri, 15 May 2009 18:09:11 -0700

Emacs -Q under NS, then

(progn
  (make-frame-invisible nil t)
  (make-frame)
  (delete-frame (selected-frame) t))

will unexpectedly leave one frame visible and raised.  It should  
actually hide all frames.

The code that does it come in via the NS port (75f88b1c by arobert on  
2008-07-15), in do_switch_frame():

#ifdef NS_IMPL_COCOA
  /* term gets no other notification of this */
  if (for_deletion)
    Fraise_frame(Qnil);
#endif

	(do_switch_frame): When for_deletion under Cocoa, add
	Fraise_frame(Qnil).


This seems needed in order to raise another (visible) frame -  
otherwise no frame gets raised (or visibly selected).

The change below would address this - but is Fselect_frame() intended  
to make frames visible (on other platforms)?



diff --git a/src/frame.c b/src/frame.c
index de857af..fbef938 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -868,7 +868,7 @@ do_switch_frame (frame, track, for_deletion,  
norecord)

 #ifdef NS_IMPL_COCOA
   /* term gets no other notification of this */
-  if (for_deletion)
+  if (for_deletion && FRAME_VISIBLE_P (XFRAME (frame)))
     Fraise_frame(Qnil);
 #endif

Message #10 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: 3303 <at> debbugs.gnu.org, 3204 <at> debbugs.gnu.org
Date: Fri, 15 May 2009 19:27:55 -0700

Bugs 3303 and 3204 are probably related / same.

Message #15 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Chong Yidong <cyd <at> stupidchicken.com>
To: David Reitter <david.reitter <at> gmail.com>
Cc: 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame 
Date: Sat, 16 May 2009 15:28:29 -0400

> Emacs -Q under NS, then
>
> (progn
>   (make-frame-invisible nil t)
>   (make-frame)
>   (delete-frame (selected-frame) t))
>
> will unexpectedly leave one frame visible and raised.  It should  
> actually hide all frames.

Emacs does this on GNU/Linux as well, at least under the Metacity window
manager.  If we "fix" it---which I'm not sure we should---then the fix
should occur in the platform-independent code.

Message #20 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Chong Yidong <cyd <at> stupidchicken.com>
Cc: 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame
Date: Sat, 16 May 2009 22:55:54 -0400

[Message part 1 (text/plain, inline)]

On May 16, 2009, at 3:28 PM, Chong Yidong wrote:

>> will unexpectedly leave one frame visible and raised.  It should
>> actually hide all frames.
>
> Emacs does this on GNU/Linux as well, at least under the Metacity  
> window
> manager.  If we "fix" it---which I'm not sure we should---then the fix
> should occur in the platform-independent code.

As it stands, Emacs is unable to consistently hide all frames, and  
this behavior of unhiding frames is simply not documented as such.

If it is the window manager that makes a frame visible, then we should  
respect that.  It's not our business.  But forcefully un-hiding some  
other frame as is done specifically in NS is not a great idea.  (We  
can select a frame without unhiding it!)

Hiding a frame is the only workaround that I know that allows me to  
remove all frames.  Running an application without any open frames is  
a perfectly normal thing to do in any environment that displays the  
menu bar on the top.  The code can't handle deleting the last frame  
(as it is assumed in many places that there is a live, selected  
frame).  That's why hiding is important to me.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #25 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <david.reitter <at> gmail.com>
Cc: Adrian Robert  <Adrian.B.Robert <at> gmail.com>,
        3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Sun, 17 May 2009 15:06:17 -0400

> (progn
>   (make-frame-invisible nil t)
>   (make-frame)
>   (delete-frame (selected-frame) t))

> will unexpectedly leave one frame visible and raised.

Under GNU/Linux it also leaves a frame visible and raised, but it's not
the one you think: the (selected-frame) call returns the invisible
frame, not the new frame, because `make-frame' did not select the
new frame.

So you may want to prefer

   (progn
     (make-frame-invisible nil t)
     (with-selected-frame (make-frame)
       (delete-frame (selected-frame) t)))

which at least under GNU/Linux seems to do the right thing.
Still, the

   #ifdef NS_IMPL_COCOA
     /* term gets no other notification of this */
     if (for_deletion)
       Fraise_frame(Qnil);
   #endif

in frame.c looks plain wrong and should probably just be deleted:
frame-selection is never intended to raise (or lower) any frame.
If this `raise' is really necessary, then it needs a much more extensive
comment justifying its presence.
Adrian, could you remove this code, or justify clearly why it's here?


        Stefan

Message #30 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Adrian Robert <Adrian.B.Robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Sun, 17 May 2009 15:12:39 -0400

[Message part 1 (text/plain, inline)]

On May 17, 2009, at 3:06 PM, Stefan Monnier wrote:
>
> Still, the
>
>   #ifdef NS_IMPL_COCOA
>     /* term gets no other notification of this */
>     if (for_deletion)
>       Fraise_frame(Qnil);
>   #endif
>
> in frame.c looks plain wrong and should probably just be deleted:
> frame-selection is never intended to raise (or lower) any frame.
> If this `raise' is really necessary, then it needs a much more  
> extensive
> comment justifying its presence.
> Adrian, could you remove this code, or justify clearly why it's here?

I think it is there because we need to raise another (visible) frame  
when a frame is deleted.  This is standard behavior (and sensible).   
However, this should not occur if the other frame is hidden, i.e. if  
there is no visible and uniconified frame.  Im currently experimenting  
with something like this:

#ifdef NS_IMPL_COCOA
  /* term gets no other notification of this */
  if (for_deletion && FRAME_VISIBLE_P (XFRAME (selected_frame))
      && FRAME_LIVE_P (XFRAME (selected_frame))
      && ! FRAME_ICONIFIED_P (XFRAME (selected_frame)))
    Fraise_frame(Qnil);
#endif

- D

[smime.p7s (application/pkcs7-signature, attachment)]

Message #35 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <david.reitter <at> gmail.com>
Cc: Adrian Robert <Adrian.B.Robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Sun, 17 May 2009 16:43:54 -0400

>> Still, the
>> 
>> #ifdef NS_IMPL_COCOA
>> /* term gets no other notification of this */
>> if (for_deletion)
>> Fraise_frame(Qnil);
>> #endif
>> 
>> in frame.c looks plain wrong and should probably just be deleted:
>> frame-selection is never intended to raise (or lower) any frame.
>> If this `raise' is really necessary, then it needs a much more extensive
>> comment justifying its presence.
>> Adrian, could you remove this code, or justify clearly why it's here?

> I think it is there because we need to raise another (visible) frame when
> a frame is deleted.  This is standard behavior (and sensible).

This is a behavior which depends on the window-management policy, so
it's the responsibility of the window-manager (which may even decide
that the focus should return to some other application, which would make
a lot of sense if the frame was created via $EDITOR=emacsclient).

So, I'd still want to know what undesirable behavior would happen under
NS if we don't call Fraise_frame here (and also, why it needs to be
called here rather than elsewhere).


        Stefan

Message #40 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Lennart Borgman <lennart.borgman <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, 3303 <at> debbugs.gnu.org
Cc: David Reitter <david.reitter <at> gmail.com>,
        Adrian Robert <Adrian.B.Robert <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 00:27:07 +0200

On Sun, May 17, 2009 at 10:43 PM, Stefan Monnier
<monnier <at> iro.umontreal.ca> wrote:
>>> Still, the
>>>
>>> #ifdef NS_IMPL_COCOA
>>> /* term gets no other notification of this */
>>> if (for_deletion)
>>> Fraise_frame(Qnil);
>>> #endif
>>>
>>> in frame.c looks plain wrong and should probably just be deleted:
>>> frame-selection is never intended to raise (or lower) any frame.
>>> If this `raise' is really necessary, then it needs a much more extensive
>>> comment justifying its presence.
>>> Adrian, could you remove this code, or justify clearly why it's here?
>
>> I think it is there because we need to raise another (visible) frame when
>> a frame is deleted.  This is standard behavior (and sensible).
>
> This is a behavior which depends on the window-management policy, so
> it's the responsibility of the window-manager (which may even decide
> that the focus should return to some other application, which would make
> a lot of sense if the frame was created via $EDITOR=emacsclient).

Shouldn't then frames created via emacsclient be a special case? That
would of course make more sense if the frame was deleted together with
the buffer opened through emacsclient...

For other frames both leaving it to the window manager and staying in
Emacs makes sense to me. Maybe a user option (if it is possible to
stay in Emacs at all)?


> So, I'd still want to know what undesirable behavior would happen under
> NS if we don't call Fraise_frame here (and also, why it needs to be
> called here rather than elsewhere).
>
>
>        Stefan
>
>
>
>
>

Message #45 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Adrian Robert <adrian.b.robert <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 08:16:22 +0700

On May 18, 2009, at 3:43 AM, Stefan Monnier wrote:

>>> in frame.c looks plain wrong and should probably just be deleted:
>>> frame-selection is never intended to raise (or lower) any frame.
>>> If this `raise' is really necessary, then it needs a much more  
>>> extensive
>>> comment justifying its presence.
>>> Adrian, could you remove this code, or justify clearly why it's  
>>> here?
>
>> I think it is there because we need to raise another (visible)  
>> frame when
>> a frame is deleted.  This is standard behavior (and sensible).
>
> This is a behavior which depends on the window-management policy, so
> it's the responsibility of the window-manager (which may even decide
> that the focus should return to some other application, which would  
> make
> a lot of sense if the frame was created via $EDITOR=emacsclient).
>
> So, I'd still want to know what undesirable behavior would happen  
> under
> NS if we don't call Fraise_frame here (and also, why it needs to be
> called here rather than elsewhere).

The behavior that arose was that no other frame would be selected or  
have focus after a frame was deleted.  NS is click-to-focus, and the  
user would have to click another one manually, which is wrong on this  
platform.  There is no behavior built into the NS window manager to  
choose another application window to be active after the active one  
has been removed -- this is left to application code.

This can be put in the comment if it would clarify -- the thing is I  
did not and don't understand why other terms, W32 at least, don't  
need it as well.

Message #50 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lennart Borgman <lennart.borgman <at> gmail.com>
Cc: 3303 <at> debbugs.gnu.org, David Reitter <david.reitter <at> gmail.com>,
        Adrian Robert <Adrian.B.Robert <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Sun, 17 May 2009 23:26:46 -0400

>> This is a behavior which depends on the window-management policy, so
>> it's the responsibility of the window-manager (which may even decide
>> that the focus should return to some other application, which would make
>> a lot of sense if the frame was created via $EDITOR=emacsclient).

> Shouldn't then frames created via emacsclient be a special case? That
> would of course make more sense if the frame was deleted together with
> the buffer opened through emacsclient...

> For other frames both leaving it to the window manager and staying in
> Emacs makes sense to me. Maybe a user option (if it is possible to
> stay in Emacs at all)?

Part of what you quoted was meant to imply that it's not for Emacs to
decide (both that it shouldn't and that it can't: at least under X11
it's usually out of Emacs's control).


        Stefan

Message #55 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Adrian Robert <adrian.b.robert <at> gmail.com>
Cc: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Sun, 17 May 2009 23:33:00 -0400

> There is no behavior built into the NS window manager to  choose another
> application window to be active after the active one  has been removed --
> this is left to application code.

[ So, nothing gets focus?  Keyboard events are just dropped on the floor
  in such a case?  Sounds odd: it should be easy for Apple to provide
  a sensible default behavior without any negative impact.  ]

Thanks.  That explains why calling Fraise_frame might be necessary.
It's still not clear why this place is the right place for it.

> This can be put in the comment if it would clarify

Yes, please do so, it would help a lot.

> -- the thing is I did not and don't understand why other terms, W32 at
> least, don't  need it as well.

Don't know about W32, but under X11, window-managers are expected to
(and do) make such decisions.


        Stefan

Message #60 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Adrian Robert <adrian.b.robert <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 15:05:35 +0700

On May 18, 2009, at 10:33 AM, Stefan Monnier wrote:

>> There is no behavior built into the NS window manager to  choose  
>> another
>> application window to be active after the active one  has been  
>> removed --
>> this is left to application code.
>
> [ So, nothing gets focus?  Keyboard events are just dropped on the  
> floor
>   in such a case?  Sounds odd: it should be easy for Apple to provide
>   a sensible default behavior without any negative impact.  ]

Yes, KB events only get sent to a focused window, except for menu  
shortcut invocations.  It might be that in NSDocument-based apps  
(which Emacs.app isn't, but would be conceptually similar to if 1- 
buffer=1-frame) the NSDocument architecture would autofocus the most  
recently available doc window, but I guess NeXT/Apple decided not to  
make assumptions otherwise about sensible focus-sequencing.


> Thanks.  That explains why calling Fraise_frame might be necessary.
> It's still not clear why this place is the right place for it.
>
>> This can be put in the comment if it would clarify
>
> Yes, please do so, it would help a lot.

Done, and I've committed David's fix along with it.  (I tried it and  
observed no problems -- hope that's OK David.)

Message #65 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, 3303 <at> debbugs.gnu.org
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>,
        David Reitter <david.reitter <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 17:19:04 +0900

>>>>> On Sun, 17 May 2009 23:33:00 -0400, Stefan Monnier <monnier <at> iro.umontreal.ca> said:

>> There is no behavior built into the NS window manager to  choose another
>> application window to be active after the active one  has been removed --
>> this is left to application code.

> [ So, nothing gets focus?  Keyboard events are just dropped on the floor
>   in such a case?  Sounds odd: it should be easy for Apple to provide
>   a sensible default behavior without any negative impact.  ]

I'm not sure about GNUstep, but at least Cocoa AppKit does what you
expect if the main (or possibly some) event loop is running.
Actually, the Carbon+AppKit port doesn't do anything special in
do_switch_frame.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #70 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Adrian Robert <adrian.b.robert <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 11:08:39 -0400

[Message part 1 (text/plain, inline)]

On May 18, 2009, at 4:05 AM, Adrian Robert wrote:

>
> On May 18, 2009, at 10:33 AM, Stefan Monnier wrote:
>
>>> There is no behavior built into the NS window manager to  choose  
>>> another
>>> application window to be active after the active one  has been  
>>> removed --
>>> this is left to application code.
>>
>> [ So, nothing gets focus?  Keyboard events are just dropped on the  
>> floor
>>  in such a case?  Sounds odd: it should be easy for Apple to provide
>>  a sensible default behavior without any negative impact.  ]
>
> Yes, KB events only get sent to a focused window, except for menu  
> shortcut invocations.  It might be that in NSDocument-based apps  
> (which Emacs.app isn't, but would be conceptually similar to if 1- 
> buffer=1-frame) the NSDocument architecture would autofocus the most  
> recently available doc window, but I guess NeXT/Apple decided not to  
> make assumptions otherwise about sensible focus-sequencing.

Precisely for this reason is the patch not sufficient.
When there is a hidden frame, and you delete the only other existing  
frame, we end up in a situation where there is no key window to  
receive the event, and all events (including menu items) are simply  
dropped.

I've experimented with NSWindow's makeKeyWindow, which should leave  
the order of the windows so the frame stays hidden.

However, this alone doesn't do the job.  Events still get dropped when  
you do

(progn
  (make-frame-invisible (selected-frame) t)
  (make-frame)
  (delete-frame (selected-frame) t))


Note that I had to add FRAME_SAMPLE_VISIBILITY compared to the last  
patch I sent.



diff --git a/src/frame.c b/src/frame.c
index de857af..144b8ac 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -778,6 +778,9 @@ affects all frames on the same terminal device.  */)
    is dead.

    The value of NORECORD is passed as argument to Fselect_window.  */
+#ifdef HAVE_NS
+extern  void x_make_key_frame (struct frame *f);
+#endif

 Lisp_Object
 do_switch_frame (frame, track, for_deletion, norecord)
@@ -868,8 +871,12 @@ do_switch_frame (frame, track, for_deletion,  
norecord)

 #ifdef NS_IMPL_COCOA
   /* term gets no other notification of this */
+  FRAME_SAMPLE_VISIBILITY (XFRAME (selected_frame));
   if (for_deletion)
-    Fraise_frame(Qnil);
+    if (FRAME_VISIBLE_P (XFRAME (selected_frame)))
+      Fraise_frame(Qnil);
+    else
+      x_make_key_frame(XFRAME (selected_frame));
 #endif

   /* We want to make sure that the next event generates a frame-switch
diff --git a/src/nsterm.m b/src/nsterm.m
index 7e3028a..0994f7d 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -910,6 +910,20 @@ ns_raise_frame (struct frame *f)
 }


+void
+x_make_key_frame (struct frame *f)
+/*  
--------------------------------------------------------------------------
+     Make window active
+    
-------------------------------------------------------------------------- */
+{
+  NSView *view = FRAME_NS_VIEW (f);
+  check_ns ();
+  BLOCK_INPUT;
+  [[view window] makeKeyWindow];
+  UNBLOCK_INPUT;
+}
+
+
 static void
 ns_lower_frame (struct frame *f)
 /*  
--------------------------------------------------------------------------

[smime.p7s (application/pkcs7-signature, attachment)]

Message #75 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <david.reitter <at> gmail.com>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 16:12:32 -0400

>>> [ So, nothing gets focus?  Keyboard events are just dropped on the floor
>>> in such a case?  Sounds odd: it should be easy for Apple to provide
>>> a sensible default behavior without any negative impact.  ]
>> 
>> Yes, KB events only get sent to a focused window, except for menu shortcut
>> invocations.  It might be that in NSDocument-based apps  (which Emacs.app
>> isn't, but would be conceptually similar to if 1- 
>> buffer=1-frame) the NSDocument architecture would autofocus the most
>> recently available doc window, but I guess NeXT/Apple decided not to  make
>> assumptions otherwise about sensible focus-sequencing.

> Precisely for this reason is the patch not sufficient.

I do not understand.

> When there is a hidden frame, and you delete the only other existing frame,
> we end up in a situation where there is no key window to receive the event,
> and all events (including menu items) are simply dropped.

Could you explain concretely why it's a problem.


        Stefan


PS: Another problem I see is that I don't think raise-frame should make
an invisible frame visible.  It's right for iconified frames, but
I don't think it's right for invisible frames.  I.e. a patch like the
one below might be a good change (not for 23.1, tho, obviously).


=== modified file 'src/frame.c'
--- src/frame.c	2009-05-05 14:50:29 +0000
+++ src/frame.c	2009-05-18 20:09:22 +0000
@@ -2020,7 +2020,7 @@
   if (FRAME_TERMCAP_P (f))
     /* On a text-only terminal select FRAME.  */
     Fselect_frame (frame, Qnil);
-  else
+  else if (FRAME_ICONIFIED_P (f))
     /* Do like the documentation says. */
     Fmake_frame_visible (frame);
 

Message #80 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 19:00:17 -0400

[Message part 1 (text/plain, inline)]

On May 18, 2009, at 4:12 PM, Stefan Monnier wrote:
>> Precisely for this reason is the patch not sufficient.
>
> I do not understand.
>
>> When there is a hidden frame, and you delete the only other  
>> existing frame,
>> we end up in a situation where there is no key window to receive  
>> the event,
>> and all events (including menu items) are simply dropped.
>
> Could you explain concretely why it's a problem.

Well, if you have only hidden frames this way, you will receive no key  
events:

(progn
  (make-frame-invisible (selected-frame) t)
  (make-frame)
  (delete-frame (selected-frame) t))

The Lisp level doesn't even see menu events.

A little more investigation shows that we get the event in keyDown:,  
but we discard it in this code:

 if (![[self window] isKeyWindow])
   {
     /* XXX: There is an occasional condition in which, when Emacs  
display
         updates a different frame from the current one, and  
temporarily
         selects it, then processes some interrupt-driven input
         (dispnew.c:3878), OS will send the event to the correct  
NSWindow, but
         for some reason that window has its first responder set to  
the NSView
         most recently updated (I guess), which is not the correct  
one. */
     if ([[theEvent window] isKindOfClass: [EmacsWindow class]])
	      [(EmacsView *)[[theEvent window] delegate] keyDown: theEvent];
     return;
   }

The outer if condition is true, presumably for the weird reason stated  
in the comment.
The inner if condition is false, so the event doesn't get passed on,  
and we just discard it.

Sticking the "return" into the inner if helps.  Of course I'm not so  
sure if that is the right fix.

Even with this workaround/fix, now we're back to the other problem  
with this bit of code:

(progn
  (make-frame-invisible (selected-frame) t)
  (make-frame)
  (delete-frame (selected-frame) t)
  (make-frame)
  (sit-for 0)
  (delete-frame (selected-frame) t))

This will leave a frame visible, i.e. in the last `delete-frame', the  
frame is deleted, but the other one is made visible.

That happens because the FRAME_VISIBLE_P check in do_switch_frame does  
not return nil for frames that are actually supposed to be hidden.    
It shouldn't do that...
f->visible and f->async_visible are both 1, even at the beginning of  
do_switch_frame.
I don't understand why.

Note that this does NOT happen if you run it without the `sit-for'  
call, e.g. in a single `progn' form.

Ideas?

[smime.p7s (application/pkcs7-signature, attachment)]

Message #85 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Tue, 19 May 2009 09:58:42 +0900

>>>>> On Mon, 18 May 2009 11:08:39 -0400, David Reitter <david.reitter <at> gmail.com> said:

>>>> There is no behavior built into the NS window manager to choose
>>>> another application window to be active after the active one has
>>>> been removed -- this is left to application code.
>>> 
>>> [ So, nothing gets focus?  Keyboard events are just dropped on the
>>> floor in such a case?  Sounds odd: it should be easy for Apple to
>>> provide a sensible default behavior without any negative impact.
>>> ]
>> 
>> Yes, KB events only get sent to a focused window, except for menu
>> shortcut invocations.  It might be that in NSDocument-based apps
>> (which Emacs.app isn't, but would be conceptually similar to if 1-
>> buffer=1-frame) the NSDocument architecture would autofocus the
>> most recently available doc window, but I guess NeXT/Apple decided
>> not to make assumptions otherwise about sensible focus-sequencing.

> Precisely for this reason is the patch not sufficient.

As I mentioned in (*), neither the above consideration about
NSDocument-based vs. non-NSDocument-based nor the updated comment in
frame.c is correct.

*: http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-05/msg00373.html

If you don't observe the problems on the Carbon+AppKit port, then it
indicates that they can most likely be solved without touching the
platform-independent code.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #90 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <david.reitter <at> gmail.com>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 22:46:19 -0400

>>> Precisely for this reason is the patch not sufficient.
>> 
>> I do not understand.
>> 
>>> When there is a hidden frame, and you delete the only other existing
>>> frame,
>>> we end up in a situation where there is no key window to receive the
>>> event,
>>> and all events (including menu items) are simply dropped.
>> 
>> Could you explain concretely why it's a problem.

> Well, if you have only hidden frames this way, you will receive no key
> events:

> (progn
>   (make-frame-invisible (selected-frame) t)
>   (make-frame)
>   (delete-frame (selected-frame) t))

> The Lisp level doesn't even see menu events.

You just repeated what you had already written.  It's not concrete
enough for me to understand.  What means "receive no key event" or "Lisp
doesn't even see menu events"?

> A little more investigation shows that we get the event in keyDown:, but we
> discard it in this code:

>  if (![[self window] isKeyWindow])
>    {
>      /* XXX: There is an occasional condition in which, when Emacs display
>          updates a different frame from the current one, and temporarily
>          selects it, then processes some interrupt-driven input
>          (dispnew.c:3878), OS will send the event to the correct NSWindow,
> but
>          for some reason that window has its first responder set to the
> NSView
>          most recently updated (I guess), which is not the correct one. */
>      if ([[theEvent window] isKindOfClass: [EmacsWindow class]])
> 	      [(EmacsView *)[[theEvent window] delegate] keyDown: theEvent];
>      return;
>    }

I think I understand: you mean that when there's only one frame and it's
invisible, we actively ignore *all* events (rather than the OS refusing
to deliver them to us), and hence Emacs becomes completely unresponsive.
Yes, that bug would need to be fixed.

> Even with this workaround/fix, now we're back to the other problem with this
> bit of code:

> (progn
>   (make-frame-invisible (selected-frame) t)
>   (make-frame)
>   (delete-frame (selected-frame) t)
>   (make-frame)
>   (sit-for 0)
>   (delete-frame (selected-frame) t))

I still don't understand the above code, for the reason already
explained: you use `selected-frame' in a way that seems to imply that
you expect make-frame to change the selected-frame, where its docstring
says explicitly that it doesn't.


        Stefan

Message #95 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 22:56:32 -0400

[Message part 1 (text/plain, inline)]

On May 18, 2009, at 10:46 PM, Stefan Monnier wrote:
> You just repeated what you had already written.  It's not concrete
> enough for me to understand.  What means "receive no key event" or  
> "Lisp
> doesn't even see menu events"?

I enter a key sequence, e.g. C-x 5 2, and Emacs does not react.
I select a menu item from the Options menu, and Emacs does not react.

> I think I understand: you mean that when there's only one frame and  
> it's
> invisible, we actively ignore *all* events (rather than the OS  
> refusing
> to deliver them to us), and hence Emacs becomes completely  
> unresponsive.
> Yes, that bug would need to be fixed.

This does not happen all the time when there is only one frame and it  
is invisible, but it is perfectly reproducible for me with the code I  
sent.

> I still don't understand the above code, for the reason already
> explained: you use `selected-frame' in a way that seems to imply that
> you expect make-frame to change the selected-frame, where its  
> docstring
> says explicitly that it doesn't.

Ah, now I see why you don't understand.

The doc string says that the system may select it, and that's exactly  
what happens here on Cocoa/OSX.
This reproduces the problem just as well:

(progn
  (make-frame-invisible (selected-frame) t)
  (select-frame (make-frame))
  (delete-frame (selected-frame) t)
  (select-frame (make-frame))
  (sit-for 0)
  (delete-frame (selected-frame) t))

i.e. we end up with a visible frame, the frame that we hid initially.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #100 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <david.reitter <at> gmail.com>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 23:09:12 -0400

> i.e. we end up with a visible frame, the frame that we hid initially.

As mentioned, part of the reason is that raise-frame incorrectly makes
the frame visible.

Admittedly, the correctness is debatable, since the docstring says "If
frame is iconified, make it visible" whereas the Elisp manual says "If
FRAME is invisible or iconified, this makes it visible".  So the code is
correct w.r.t the Elisp manual but not the docstring.  I actually
believe the docstring describes the behavior we want.


        Stefan

Message #105 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>, 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Mon, 18 May 2009 23:15:14 -0400

[Message part 1 (text/plain, inline)]

On May 18, 2009, at 11:09 PM, Stefan Monnier wrote:

>> i.e. we end up with a visible frame, the frame that we hid initially.
>
> As mentioned, part of the reason is that raise-frame incorrectly makes
> the frame visible.
>
> Admittedly, the correctness is debatable, since the docstring says "If
> frame is iconified, make it visible" whereas the Elisp manual says "If
> FRAME is invisible or iconified, this makes it visible".  So the  
> code is
> correct w.r.t the Elisp manual but not the docstring.  I actually
> believe the docstring describes the behavior we want.

What I described happens with your path installed just the same, so it  
is really only part of the problem.

One issue I can see there in NS is that NS hides frames by moving them  
behind the desktop in the view hierarchy, so raising them implies  
making them visible.  Of course we could (and should) add a check for  
frame visibility to the appropriate NS functions.

As far as I remember, raising implied making a frame visible in Emacs  
22/Carbon.

I'm not sure if a change to the behavior of raise-frame would be a  
good idea at this stage; the bug at hand needs fixing of course.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #110 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,
        Adrian Robert <adrian.b.robert <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Tue, 19 May 2009 17:20:20 +0900

>>>>> On Mon, 18 May 2009 22:56:32 -0400, David Reitter <david.reitter <at> gmail.com> said:

>> I still don't understand the above code, for the reason already
>> explained: you use `selected-frame' in a way that seems to imply
>> that you expect make-frame to change the selected-frame, where its
>> docstring says explicitly that it doesn't.

> Ah, now I see why you don't understand.

> The doc string says that the system may select it, and that's
> exactly what happens here on Cocoa/OSX.

That's due to the following NS-specific code in term/ns-win.el:

  ;; frame will be focused anyway, so select it
  ;; (if this is not done, modeline is dimmed until first interaction)
  (add-hook 'after-make-frame-functions 'select-frame)

I would read this comment as "this is a workaround".  TRT is to
investigate why the modeline is dimmed until the first interaction and
fix it, of course.  Otherwise, the NS port will face hard-to-find
incompatibilities in various elisp packages.

The docstring says "The previously selected frame remains selected.
However, the window system may select the new frame for its own
reasons, for instance ...".  In (non-NS) click-to-focus environments,
the selection of the new frame does not happen immediately but does
when the Lisp interpreter goes back to the command loop and reads
events (cf. docstring of select-frame).

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

Message #115 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Cc: David Reitter <david.reitter <at> gmail.com>, 3303 <at> debbugs.gnu.org,
        Adrian Robert <adrian.b.robert <at> gmail.com>
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Tue, 19 May 2009 10:30:26 -0400

>>> I still don't understand the above code, for the reason already
>>> explained: you use `selected-frame' in a way that seems to imply
>>> that you expect make-frame to change the selected-frame, where its
>>> docstring says explicitly that it doesn't.

>> Ah, now I see why you don't understand.

>> The doc string says that the system may select it, and that's
>> exactly what happens here on Cocoa/OSX.

> That's due to the following NS-specific code in term/ns-win.el:

>   ;; frame will be focused anyway, so select it
>   ;; (if this is not done, modeline is dimmed until first interaction)
>   (add-hook 'after-make-frame-functions 'select-frame)

> I would read this comment as "this is a workaround".  TRT is to
> investigate why the modeline is dimmed until the first interaction and
> fix it, of course.  Otherwise, the NS port will face hard-to-find
> incompatibilities in various elisp packages.

Agreed.  Could someone remove the above line and install a proper fix
for it?


        Stefan

Message #120 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <david.reitter <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>,
        Adrian Robert <adrian.b.robert <at> gmail.com>
Cc: 3303 <at> debbugs.gnu.org
Subject: Re: bug#3303: delete-frame raises old (invisible) frame
Date: Tue, 19 May 2009 22:07:00 -0400

[Message part 1 (text/plain, inline)]

On May 18, 2009, at 10:56 PM, David Reitter wrote:
> The doc string says that the system may select it, and that's  
> exactly what happens here on Cocoa/OSX.
> This reproduces the problem just as well:
>
> (progn
>  (make-frame-invisible (selected-frame) t)
>  (select-frame (make-frame))
>  (delete-frame (selected-frame) t)
>  (select-frame (make-frame))
>  (sit-for 0)
>  (delete-frame (selected-frame) t))
>
> i.e. we end up with a visible frame, the frame that we hid initially.


OK, I have traced this a bit further and reduced it to this:

(progn
  (setq aa1 (selected-frame))
  (make-frame-invisible (selected-frame) t)
  (setq aa2 (selected-frame)
	aa2v (frame-visible-p (selected-frame)))
  (sit-for 1)
  (setq aa3 (selected-frame)
	aa3v (frame-visible-p (selected-frame))))

(list aa1 aa2 aa2v aa3 aa3v)


After evaluating the first sexp (Emacs -Q), one can see that the  
selected frame doesn't change at any point.  But aa2v is nil  
(correctly so), and aa3v is suddenly t.

This does not happen with Emacs 22/Appkit.  Both aa2v and aa3v are  
nil, as they should.

Could someone check what the behavior is in another port (of 23)?

I believe that this is the cause of the problems we're after.
In the previous examples, when delete_frame calls do_switch_frame, the  
frame is already deemed visible, even though it hasn't been made  
visible through the appropriate system call.  The above example  
demonstrates that something (in the event loop, presumably) sets frame  
visibility.
It is not a call to FRAME_SAMPLE_VISIBILITY, which is done in `frame- 
visible-p' anyways.  (The NS port always sets f->async_visible and  
lets this macro set ->visible later.)

[smime.p7s (application/pkcs7-signature, attachment)]

Message #125 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <reitter <at> cmu.edu>
To: 3303 <at> debbugs.gnu.org
Cc: Adrian Robert <adrian.b.robert <at> gmail.com>,
        Stefan Monnier <monnier <at> iro.umontreal.ca>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Thu, 21 May 2009 23:57:08 -0400

[Message part 1 (text/plain, inline)]

OK, I think I have a set of fixes now for this bug.
The problems included swallowed events, setting async_visible and  
async_iconified from drawRect under the false assumption that this  
proves that the frame view is visible, and attendant failure to set  
visibility correctly when creating the frame (as the other ports,  
including Appkit, do).  Also, we separate visibility from frame order  
in the NS layer.

Description, test cases and patch below.
Can someone look this over to see if there is anything obviously  
wrong?  I'll check it in pending feedback.

- David

PS.: I agree with the proposed change to after-make-frame-functions  
(not selecting the frame immediately), but this doesn't relate to the  
bug at hand.  Also, I don't know why the modeline isn't updated.



Desc:

frame.c:
Fraise_frame: do not make invisible frames visible (Stefan Monnier).

nsterm.m:
ns_raise_frame(): only raise frame if visible.
x_make_frame_visible(): move frame to front rather than calling  
ns_raise_frame().
keyDown: do not swallow events that aren't re-sent if frame isn't key  
window.
drawRect: do not set visibility/iconified flags because drawRect may  
be called by NSView even if the frame is hidden.

nsfns.m:
Fx_create_frame(): follow other ports in determining visibility;  
default to t. Ensure async_visible is set.

Test cases:


;; test case
;; second time around aaa is t
(let ((f (selected-frame)))
  (make-frame-invisible f t)
  (setq	aa2v (frame-visible-p f))
  (redisplay)
  (setq	aa3v (frame-visible-p f))
)
;; frame should be gone
(list aa2v aa3v)
;; should be nil nil

;; test case
(progn
 (make-frame-invisible (selected-frame) t)
 (select-frame (make-frame))
 (delete-frame (selected-frame) t)
 (select-frame (make-frame))
 (sit-for 0)
 (delete-frame (selected-frame) t))
;; there should be no frame



Patch:


diff --git a/src/frame.c b/src/frame.c
index afcc96c..fbb00ba 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -2024,7 +2024,7 @@ doesn't support multiple overlapping frames,  
this function selects FRAME.  */)
   if (FRAME_TERMCAP_P (f))
     /* On a text-only terminal select FRAME.  */
     Fselect_frame (frame, Qnil);
-  else
+  else if (FRAME_ICONIFIED_P (f))
     /* Do like the documentation says. */
     Fmake_frame_visible (frame);

diff --git a/src/nsfns.m b/src/nsfns.m
index 01ffcf1..25d9b30 100644
--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -1317,13 +1317,20 @@ be shared by the new frame.  */)

   if (! f->output_data.ns->explicit_parent)
     {
-        tem = x_get_arg (dpyinfo, parms, Qvisibility, 0, 0,  
RES_TYPE_BOOLEAN);
-        if (EQ (tem, Qunbound))
-            tem = Qnil;
-
-        x_set_visibility (f, tem, Qnil);
-        if (EQ (tem, Qt))
-            [[FRAME_NS_VIEW (f) window] makeKeyWindow];
+      tem = x_get_arg (dpyinfo, parms, Qvisibility, 0, 0,  
RES_TYPE_SYMBOL);
+      if (EQ (tem, Qunbound))
+	tem = Qt;
+      x_set_visibility (f, tem, Qnil);
+      if (EQ (tem, Qicon))
+	x_iconify_frame (f);
+      else if (! NILP (tem))
+	{
+	  x_make_frame_visible (f);
+	  f->async_visible=1;
+	  [[FRAME_NS_VIEW (f) window] makeKeyWindow];
+	}
+      else
+	  f->async_visible=0;
     }

   if (FRAME_HAS_MINIBUF_P (f)
diff --git a/src/nsterm.m b/src/nsterm.m
index 9ca74e8..d7a8f81 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -895,9 +895,14 @@ ns_raise_frame (struct frame *f)
 {
   NSView *view = FRAME_NS_VIEW (f);
   check_ns ();
-  BLOCK_INPUT;
-  [[view window] makeKeyAndOrderFront: NSApp];
-  UNBLOCK_INPUT;
+
+  FRAME_SAMPLE_VISIBILITY (f);
+  if (FRAME_VISIBLE_P (f))
+    {
+      BLOCK_INPUT;
+      [[view window] makeKeyAndOrderFront: NSApp];
+      UNBLOCK_INPUT;
+    }
 }


@@ -979,11 +984,17 @@ x_make_frame_visible (struct frame *f)
     
-------------------------------------------------------------------------- */
 {
   NSTRACE (x_make_frame_visible);
+  NSView *view = FRAME_NS_VIEW (f);
   /* XXX: at some points in past this was not needed, as the only  
place that
      called this (frame.c:Fraise_frame ()) also called raise_lower;
      if this ends up the case again, comment this out again. */
   if (!FRAME_VISIBLE_P (f))
-    ns_raise_frame (f);
+    {
+      BLOCK_INPUT;
+      [[view window] makeKeyAndOrderFront: NSApp];
+      UNBLOCK_INPUT;
+    }
+  f->async_visible = 1;
 }


@@ -4461,7 +4472,8 @@ extern void update_window_cursor (struct window  
*w, int on);
   if (!emacs_event)
     return;

- if (![[self window] isKeyWindow])
+ if (![[self window] isKeyWindow]
+     && [[theEvent window] isKindOfClass: [EmacsWindow class]])
    {
      /* XXX: There is an occasional condition in which, when Emacs  
display
          updates a different frame from the current one, and  
temporarily
@@ -4469,8 +4481,7 @@ extern void update_window_cursor (struct window  
*w, int on);
          (dispnew.c:3878), OS will send the event to the correct  
NSWindow, but
          for some reason that window has its first responder set to  
the NSView
          most recently updated (I guess), which is not the correct  
one. */
-     if ([[theEvent window] isKindOfClass: [EmacsWindow class]])
-         [(EmacsView *)[[theEvent window] delegate] keyDown: theEvent];
+     [(EmacsView *)[[theEvent window] delegate] keyDown: theEvent];
      return;
    }

@@ -5466,8 +5477,6 @@ extern void update_window_cursor (struct window  
*w, int on);

   ns_clear_frame_area (emacsframe, x, y, width, height);
   expose_frame (emacsframe, x, y, width, height);
-  emacsframe->async_visible = 1;
-  emacsframe->async_iconified = 0;
 }

[smime.p7s (application/pkcs7-signature, attachment)]

Message #130 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: David Reitter <reitter <at> cmu.edu>
Cc: 3303 <at> debbugs.gnu.org, Adrian Robert <adrian.b.robert <at> gmail.com>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Mon, 25 May 2009 11:17:17 -0400

> OK, I think I have a set of fixes now for this bug.

Good, thanks.

> PS.: I agree with the proposed change to after-make-frame-functions (not
> selecting the frame immediately), but this doesn't relate to the  bug at
> hand.  Also, I don't know why the modeline isn't updated.

Agreed, this is a separate problem.

I'm not familiar enough with the NS code to judge if the patch is OK,
but here are some comments:

> frame.c:
> Fraise_frame: do not make invisible frames visible (Stefan Monnier).

This is not OK for 23.1.  It might be good to try it for 23.2.
Also I think your other changes should make it unnecessary for the
problem we're trying to fix.

> nsterm.m:
> ns_raise_frame(): only raise frame if visible.

Since you say that "invisible" is implemented by lowering the window
below the background, this seems like a plain bug-fix, yes.

> x_make_frame_visible(): move frame to front rather than calling
> ns_raise_frame().

Sounds right.

> keyDown: do not swallow events that aren't re-sent if frame isn't
> key window.

If you say so.

> drawRect: do not set visibility/iconified flags because drawRect may be
> called by NSView even if the frame is hidden.

Do you happen to know why/when NSView might be called even for a frame
that's not visible?

> nsfns.m:
> Fx_create_frame(): follow other ports in determining visibility; default to
> t. Ensure async_visible is set.

Sounds good.

> +	  f->async_visible=1;
[...]
> +	  f->async_visible=0;

Please put spaces around infix operators (like `=' above).

> @@ -895,9 +895,14 @@ ns_raise_frame (struct frame *f)
>  {
>    NSView *view = FRAME_NS_VIEW (f);
>    check_ns ();
> -  BLOCK_INPUT;
> -  [[view window] makeKeyAndOrderFront: NSApp];
> -  UNBLOCK_INPUT;
> +
> +  FRAME_SAMPLE_VISIBILITY (f);
> +  if (FRAME_VISIBLE_P (f))
> +    {
> +      BLOCK_INPUT;
> +      [[view window] makeKeyAndOrderFront: NSApp];
> +      UNBLOCK_INPUT;
> +    }
>  }

Shouldn't we BLOCK_INPUT around the whole thing, just in case
async_visible gets changed at the wrong time?  It probably
doesn't matter.

>  {
>    NSTRACE (x_make_frame_visible);
> +  NSView *view = FRAME_NS_VIEW (f);
>    /* XXX: at some points in past this was not needed, as the only place
> that
>       called this (frame.c:Fraise_frame ()) also called raise_lower;
>       if this ends up the case again, comment this out again. */
>    if (!FRAME_VISIBLE_P (f))
> -    ns_raise_frame (f);
> +    {
> +      BLOCK_INPUT;
> +      [[view window] makeKeyAndOrderFront: NSApp];
> +      UNBLOCK_INPUT;
> +    }
> +  f->async_visible = 1;
>  }

I think I'd prefer:

      if (!FRAME_VISIBLE_P (f))
   +    {
   +      f->async_visible = 1;
          ns_raise_frame (f);
   +    }

> @@ -5466,8 +5477,6 @@ extern void update_window_cursor (struct window *w,
> int on);

>    ns_clear_frame_area (emacsframe, x, y, width, height);
>    expose_frame (emacsframe, x, y, width, height);
> -  emacsframe->async_visible = 1;
> -  emacsframe->async_iconified = 0;
>  }

In this kind of change, it's often good to keep the old code commented
out, with a comment explaining why it was removed (and ideally why it
was there before as well).


        Stefan

Message #135 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <reitter <at> cmu.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3303 <at> debbugs.gnu.org, Adrian Robert <adrian.b.robert <at> gmail.com>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Tue, 26 May 2009 14:20:22 -0400

[Message part 1 (text/plain, inline)]

Thanks Stefan for reviewing these changes.  Agreed on all of your  
points.
I've adopted them and checked this in now since I didn't get any other  
feedback.

>> PS.: I agree with the proposed change to after-make-frame-functions  
>> (not
>> selecting the frame immediately), but this doesn't relate to the   
>> bug at
>> hand.  Also, I don't know why the modeline isn't updated.
>
> Agreed, this is a separate problem.

OK, are you keeping track of it, should we file another bug report to  
do so?

>> frame.c:
>> Fraise_frame: do not make invisible frames visible (Stefan Monnier).
>
> This is not OK for 23.1.  It might be good to try it for 23.2.
> Also I think your other changes should make it unnecessary for the
> problem we're trying to fix.

As above.


>> keyDown: do not swallow events that aren't re-sent if frame isn't
>> key window.
>
> If you say so.

What does Adrian say?? I'm just fixing the workaround, even though I  
don't fully understand the bug that leads to the problem.

>> drawRect: do not set visibility/iconified flags because drawRect  
>> may be
>> called by NSView even if the frame is hidden.
>
> Do you happen to know why/when NSView might be called even for a frame
> that's not visible?

Unfortunately not, but invisibility does not guarantee absence of  
drawRect messages unless this is promised somewhere in the NS API.
I do think that my patch here is right, though, as other ports set  
visibility in more obvious places.

Perhaps we should make sure that we don't get a lot of drawRect calls  
for totally obscured frames, which would possibly be a performance- 
eater.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #140 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <reitter <at> cmu.edu>
Cc: 3303 <at> debbugs.gnu.org, Adrian Robert <adrian.b.robert <at> gmail.com>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Tue, 26 May 2009 15:37:12 -0400

>>> PS.: I agree with the proposed change to after-make-frame-functions (not
>>> selecting the frame immediately), but this doesn't relate to the  bug at
>>> hand.  Also, I don't know why the modeline isn't updated.
>> Agreed, this is a separate problem.
> OK, are you keeping track of it, should we file another bug report to do so?

Please make another bug report for it.

>>> frame.c:
>>> Fraise_frame: do not make invisible frames visible (Stefan Monnier).
>> This is not OK for 23.1.  It might be good to try it for 23.2.
>> Also I think your other changes should make it unnecessary for the
>> problem we're trying to fix.
> As above.

I intend to install it myself for Emacs-23.2.  No need for a bug report
for that, I think.

>>> drawRect: do not set visibility/iconified flags because drawRect may be
>>> called by NSView even if the frame is hidden.
>> Do you happen to know why/when NSView might be called even for a frame
>> that's not visible?
> Unfortunately not, but invisibility does not guarantee absence of drawRect
> messages unless this is promised somewhere in the NS API.
> I do think that my patch here is right, though, as other ports set
> visibility in more obvious places.

Yes, the patch looks OK, but I'd still like to know why drawRect gets
called in such cases, as well as why the visibility settings were
modified in that routine.

> Perhaps we should make sure that we don't get a lot of drawRect calls for
> totally obscured frames, which would possibly be a performance-eater.

Indeed.


        Stefan

Message #145 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <reitter <at> cmu.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3303 <at> debbugs.gnu.org, Adrian Robert <adrian.b.robert <at> gmail.com>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Tue, 26 May 2009 16:15:34 -0400

[Message part 1 (text/plain, inline)]

On May 26, 2009, at 3:37 PM, Stefan Monnier wrote:
>
>>>> drawRect: do not set visibility/iconified flags because drawRect  
>>>> may be
>>>> called by NSView even if the frame is hidden.
>>> Do you happen to know why/when NSView might be called even for a  
>>> frame
>>> that's not visible?
>> Unfortunately not, but invisibility does not guarantee absence of  
>> drawRect
>> messages unless this is promised somewhere in the NS API.
>> I do think that my patch here is right, though, as other ports set
>> visibility in more obvious places.
>
> Yes, the patch looks OK, but I'd still like to know why drawRect gets
> called in such cases, as well as why the visibility settings were
> modified in that routine.

In this vein, is it possible to get the development history of the NS  
port merged with the other Emacs history?
I mean, everything begins (ends) with the merge, with all code there  
appearing to have been authored by 'arobert'.  In this particular case  
I would have wanted to know when these lines of code were added (e.g.,  
pre-22?), and what the comments were.
I could probably try to find this in Emacs.app, but I'm wondering if  
we can have this history (perhaps in a separate branch) in the Bzr/Git  
repositories.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #150 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: David Reitter <reitter <at> cmu.edu>
Cc: 3303 <at> debbugs.gnu.org, Adrian Robert <adrian.b.robert <at> gmail.com>,
        Chong Yidong <cyd <at> stupidchicken.com>,
        YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Subject: Re: delete-frame raises old (invisible) frame
Date: Tue, 26 May 2009 17:30:45 -0400

> I could probably try to find this in Emacs.app, but I'm wondering if
> we can have this history (perhaps in a separate branch) in the
> Bzr/Git repositories.

It would be good, but I don't think much of this history even exists.


        Stefan

Message #155 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Adrian Robert <adrian.b.robert <at> gmail.com>
To: David Reitter <reitter <at> cmu.edu>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame
Date: Wed, 27 May 2009 11:51:48 +0700

[Message part 1 (text/plain, inline)]

>>> keyDown: do not swallow events that aren't re-sent if frame isn't
>>> key window.
>>
>> If you say so.
>
> What does Adrian say?? I'm just fixing the workaround, even though  
> I don't fully understand the bug that leads to the problem.

The symptom I observed that led me to add that section of code was  
that, when two frames are open, both displaying different buffers,  
and you hold the cursor-down or page-down key down in one, the focus  
would shift back and forth between the windows, and the cursor would  
do some movement in each.  It's possible this no longer occurs due to  
other changes in focus handling both on NS and core sides, but it's  
worth testing.

Regarding the history question, there was no use of CVS during my  
maintainership (or before, I believe), but there was a ChangeLog.  It  
got removed in the merge, but I'm attaching it here.

In addition, when bzr was being used for a couple of months just  
prior to the merge, the person that did the import put in history  
entries for each release, about a dozen all together.  I'm not sure  
if they were full changelog segments or just the release notes, but  
anyway I believe this is gone now too, as the eventual CVS add to  
trunk was done separately from the bzr tree.

[ChangeLog (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #160 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Adrian Robert <adrian.b.robert <at> gmail.com>
Cc: David Reitter <reitter <at> cmu.edu>, 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame
Date: Wed, 27 May 2009 10:36:34 -0400

> The symptom I observed that led me to add that section of code was that,
> when two frames are open, both displaying different buffers,  and you hold
> the cursor-down or page-down key down in one, the focus  would shift back
> and forth between the windows, and the cursor would  do some movement in
> each.  It's possible this no longer occurs due to  other changes in focus
> handling both on NS and core sides, but it's  worth testing.

Then it's good to remove the workaround, even if the symptom re-appears:
it should be fixed elsewhere.


        Stefan

Message #165 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David Reitter <reitter <at> cmu.edu>
To: Adrian Robert <adrian.b.robert <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame
Date: Wed, 27 May 2009 11:28:01 -0400

[Message part 1 (text/plain, inline)]

On May 27, 2009, at 12:51 AM, Adrian Robert wrote:
> The symptom I observed that led me to add that section of code was  
> that, when two frames are open, both displaying different buffers,  
> and you hold the cursor-down or page-down key down in one, the focus  
> would shift back and forth between the windows, and the cursor would  
> do some movement in each.  It's possible this no longer occurs due  
> to other changes in focus handling both on NS and core sides, but  
> it's worth testing.

OK.  This has not cropped up after my changes, so that's good.  We  
should test if the workaround as a whole is still needed (but many of  
the bugs on the long list for package NS are more important now).

> Regarding the history question, there was no use of CVS during my  
> maintainership (or before, I believe), but there was a ChangeLog.   
> It got removed in the merge, but I'm attaching it here.

Thanks for sending this.

[smime.p7s (application/pkcs7-signature, attachment)]

Message #170 received at 3303 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Adrian Robert <adrian.b.robert <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: David Reitter <reitter <at> cmu.edu>, 3303 <at> debbugs.gnu.org
Subject: Re: delete-frame raises old (invisible) frame
Date: Mon, 1 Jun 2009 16:37:51 +0700

On May 27, 2009, at 9:36 PM, Stefan Monnier wrote:

>> The symptom I observed that led me to add that section of code was  
>> that,
>> when two frames are open, both displaying different buffers,  and  
>> you hold
>> the cursor-down or page-down key down in one, the focus  would  
>> shift back
>> and forth between the windows, and the cursor would  do some  
>> movement in
>> each.  It's possible this no longer occurs due to  other changes  
>> in focus
>> handling both on NS and core sides, but it's  worth testing.
>
> Then it's good to remove the workaround, even if the symptom re- 
> appears:
> it should be fixed elsewhere.

I put a FIXME comment in xdisp.c redisplay_internal() (line 11551) at  
the same time I made my workaround.  I thought the problem originated  
there, but wasn't certain enough to pursue it further.   
(consider_all_windows set true causes each FRAME to be temporarily  
selected in turn later in this function.)

Since the problem seems to be gone, I'll take a look through the  
history once savannah is back to see what might have changed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.