bug#3303: delete-frame raises old (invisible) frame

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

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

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

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

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[Lennart Borgman]

On Sun, May 17, 2009 at 10:43 PM, Stefan Monnier
<monnier@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
>
>
>
>
>

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[Adrian Robert]

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.

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

> 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

bug#3303: delete-frame raises old (invisible) frame

[Adrian Robert]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 3175 bytes --]

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)
  /*  
--------------------------------------------------------------------------


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

>>> [ 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);
 

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

> 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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 18 May 2009 22:56:32 -0400, David Reitter <david.reitter@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@math.s.chiba-u.ac.jp

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

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



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 18 May 2009 11:08:39 -0400, David Reitter <david.reitter@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@math.s.chiba-u.ac.jp

bug#3303: delete-frame raises old (invisible) frame

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 17 May 2009 23:33:00 -0400, Stefan Monnier <monnier@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@math.s.chiba-u.ac.jp

bug#3303: delete-frame raises old (invisible) frame

[Chong Yidong]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

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.



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2193 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 5584 bytes --]

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;
  }




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2438 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

> 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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2438 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

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

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

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.



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2438 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

> 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

bug#3303: delete-frame raises old (invisible) frame

[Adrian Robert]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

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



[-- Attachment #2: ChangeLog --]
[-- Type: application/octet-stream, Size: 20127 bytes --]

2008-03-20  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (show_hourglass, hide_hourglass): New functions, no impl
	yet.
	* nsmenu.m (EmacsDialog): Update appearance to be more like standard
	NS alert panel.
	(Fns_yes_or_no_p): Remove.
	* ns-win.el (ns-yes-or-no-p): Add implementation that calls
	yes-or-no-p.

2008-03-17  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_read_socket): Increment/decrement handling_signal when
	experimental_ctrl_g set, due to advice on emacs-devel, although it
	really seems this should be done in keyboard.c (input_available_signal
	et al.) since we have no idea of whether we are being called from a
	signal handler or not.  Also, ignore the 'expected' argument, which
	seems a crock but fixes the input handling in mixed terminal/GUI
	sessions.
	* keymap.c (where-is-internal): Generalize the handling of 'super'
	preference to any modifier.
	* lisp/ns-grabenv.el: Reworked based on David Reitter's version in
	Aquamacs distribution.  Behaves better w/non-csh shells.

2008-03-01  Adrian Robert <Adrian.B.Robert@gmail.com>
	* (various): Update for merge to trunk, and Stefan Monnier comments.

2007-12-14  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsfns.m (ns_set_background_color): Clear frame first (in attempt to
	fix occasional improper text erasing problem).
	* ns-win.el (ns-print-buffer): Reimplement print-buffer confirmation
	to not use advice.

2007-11-25  Adrian Robert <Adrian.B.Robert@gmail.com>
	* ns-win.el: Remove mic-paren, yank-menu-length redefinition, redo
	print-buffer override to not need advice.

2007-11-23  Adrian Robert <Adrian.B.Robert@gmail.com>
	* Version 9.0-rc3 released.

2007-11-22  Adrian Robert <Adrian.B.Robert@gmail.com>
	* configure.in, Makefile.in, src/Makefile.in, src/config.in,
	nextstep/compile: Change logic for GNUstep platforms to handle FHS
	installations in addition to regular.

2007-11-19  Adrian Robert <Adrian.B.Robert@gmail.com>
	* ns-win.el (ns-define-service): Take arg or marked text, and return
	string if arg given.
	* nsfns.m, nsselect.m: Use of Fsignal: send Qquit instead of Qerror,
	since these are more wrong-args type of messages, not serious errors
	requiring the debugger.

2007-11-16  Adrian Robert <Adrian.B.Robert@gmail.com>
	* ns-win.el: Adapt to recent change in buffer menu variable.
	* facemenu.el (facemenu-read-color): Don't require match to color
	list, so ARGB colors can be read.

2007-11-15  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (x_set_window_size): Correct behavior on certain
	toolbar-added resizes by constraining to screen ourselves -- leaving
	it to Cocoa generated inconsistent results.

2007-11-11  Adrian Robert <Adrian.B.Robert@gmail.com>
	* lisp/international/fontset.el (script-representative-chars): Added
	chars for 'symbol' script.

2007-11-08  Adrian Robert <Adrian.B.Robert@gmail.com>
	* (various): Improved multi-TTY integration.
	* nsterm.m (x_set_window_size): Don't return if font width/height has
	changed.

2007-11-01  Adrian Robert <Adrian.B.Robert@gmail.com>
	* (various): Updated for multi-TTY integration.

2007-10-14  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsmenu.m (ns_popup_menu): Set frame in call to mouse_position_hook.
	* nsterm.m (ns_mouse_position): Check for null frame.

2007-10-14  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsmenu.m (EmacsToolbar-changed): Report changed on enablement state
	change.

2007-10-13  Adrian Robert <Adrian.B.Robert@gmail.com>
	* image.c (xpm_load_image): Work around an apparent bug in NSImage
	alpha management (avoids blocky appearance of startup image on black bg).
	* nsimage.m (-setXBMColor:): Only set color for non-alpha-masked pixels.
	Shouldn't have to do this, but... (Fixes "fat" rendering w/colored or
	dark fringe backgrounds).
	* nsterm.m (ns_dumpglyphs_image): Fix minor glitch w/background drawing.
	(-setMiniwindowImage:): Add argument to set to optionally set nil (for
	miniaturized version of window).
	* nsfns.m (ns_implicitly_set_icon_type): Use it.

2007-10-12  Adrian Robert <Adrian.B.Robert@gmail.com>
	* lisp/ns-menu-bar.el: Cut down to minimal code, eliminating most
	remaining redundancy with menu-bar.el, and move the remnant to
	ns-win.el.

2007-10-10  Adrian Robert <Adrian.B.Robert@gmail.com>
	* Patch 20071010_rc2a released.

2007-10-10  Peter Dyballa <dyballa@gmx.de>
	* nextstep/compile: Add --local-lisp-path=<dir> option to prepend a
	path to epaths.h to search for lisp before other directories.

2007-10-10  Seiji Zenitani <zenitani@mac.com>
	* nsterm.m (ns_init_paths): Switch order from lisp, leim, site-lisp to
	site-lisp, lisp, leim.
	* ns-win.el (ns-utf8-nfd-post-read-conversion etc.): Define coding
	system utf-nfd to handle decomposed (normal form D) UTF-8 in MacOSX
	filenames.  Based on initial contribution by Carsten Bormann.  (This
	change went into rc2 but was not noted in the ChanngeLog there.)

2007-09-26  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_pending_files, -openFile:, ns_read_socket): Handle
	openFile requests by storing to a queue and acting on later when
	events can be processed.

2007-09-25  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_draw_window_cursor, EmacsView-windowDidResignKey):
	Be more clever about drawing background (to avoid flicker with
	blinking cursor).  Also, rename cursor type line -> bar, bar ->
	underscore.
	* frame.c (do_switch_frame): Explicitly call Fraise_frame() on
	for_deletion case (to fully focus the frame).
	* nsterm.m (x_make_frame_visible): Make into a no-op (unneeded).

2007-09-21  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_draw_fringe_bitmap): Expand bitmap image cache as needed.
	* image.c: Be more careful about retain/release on image backgrounds.

2007-09-20  Adrian Robert <Adrian.B.Robert@gmail.com>
	* Version 9.0-rc2a released.

2007-09-18  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsimage.m (initFromSkipXBM, setXBMColor): Support colored bitmap
	rendering.
	* ns-win.el (print-buffer advice): Advice function to request confirm
	before print-buffer.  Due to Kevin Rodgers.

2007-09-17  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (EmacsWindow):  New class to handle resize drags, which are
	directly handed to it by EmacsApp-sendEvent:.  Allows continuous
	display update.  Only enabled under Cocoa as resize handles vary under
	GNUstep.

2007-09-16  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsmenu.m (ns_popup_menu):  Fix bug in location computation with
	horizontally-split windows.
	* nsmenu_common.c (find_and_return_menu_selection): Fix initialization
	bug that could cause crash on menu selection.
	* nsterm.m (cursor_blink_rate, cursor_blink_mode): Added
	cursor_blink_mode variable and set in ns-win.el:blink-cursor-mode.

2007-09-15  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsfont.m (nsfont_open):  Improve font width calculation.
	* nsterm.m (ns_clear_frame_area): Check for non-null default face.

2007-09-14  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (EmacsView -drawRect:):  Incorporate an expose-frame call
	following suggestion of YAMAMOTO Mitsuharu.
	* lisp/faces.el (frame-set-background-mode): Undo a change by Daiki
	Ueno that made every face seem "locally-modified" such that it will
	not be updated for background color.

2007-09-13  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_draw_relief): Determine relief colors dynamically.

2007-09-12  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsfns.m (ns_set_background_color): Fixed crash bug when frame's
	default face not set yet.
	* nsmenu.m (EmacsMenu): Set action nil on items w/submenu.
	* nsfont.m (draw): Take account of s->gidx when determining chars to
	render.

2007-09-11  Adrian Robert <Adrian.B.Robert@gmail.com>
	* Version 9.0-rc2 released.

2007-09-09  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (EmacsView-mouseDown:): Ignore scrollwheel events with
	delta == 0.  Also, send scrollwheel events from scrollbars here.

2007-09-08  Adrian Robert <Adrian.B.Robert@gmail.com>
	* lisp/ns-mark-nav.el: Replaced with simplified implementation that
	also includes global functionality, due to Andrew L. Moore.

2007-09-07  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nextstep/compile: Add support for installing lisp in shared location
	such as /usr/local/share/emacs/23.0.0 (--enable-shared 'prefix' option).

2007-09-06  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsfont.m (ns_uni_to_glyphs, ns_glyph_metrics): Implemented on-demand
	loading of metrics cached by 256-glyph blocks for each font.
	* (various): Fixed a few memory leaks.

2007-09-03  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsmenu.m, nsmenu_common.c: New menu implementation, following mac
	and X terms more closely.
	* ns-menu-bar.el: Use the common update buffers function.

2007-08-29  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (EmacsScroller): Change scrolling to set bar position
	driven by emacs only and never internally -- prevents adjustment
	cycles causing jumpiness.
	* ns-win.el (ns-scroll-bar-move, ns-handle-scroll-bar-event): Work
	with the above change, and also use pager commands for slot clicks.
	Pager commands now use vertical-motion instead of forward-lines in
	order to avoid problems scrolling buffers with long lines.

2007-08-27  Adrian Robert <Adrian.B.Robert@gmail.com>
	* ns-win.el (ns-in-echo-area, ns-insert-working-text),
	(ns-delete-working-text, ns-echo-working-text, ns-unecho-working-text):
	Added/modified so composed editing works in weird cases when in
	minibuffer but not really in minibuffer (e.g., isearch).  Also pick up
	deleteBackward: request when aborting a compose.
	* nsterm.m (EmacsView -keyDown et al.): If command is 'super', use
	system-given key to follow, e.g., dvorak/qwerty-shortcuts map
	correctly.  Also, use charactersIgnoringModifers as criterion for
	determining whether we have a modifier sequence (instead of a
	composition).
	* cus-start.el: Add 'ns to the long list of window systems ignoring
	gtk variables.

2007-08-26  Adrian Robert <Adrian.B.Robert@gmail.com>
	* nsterm.m (ns_draw_glyph_string, ns_draw_box_or_relief)
	(ns_dumpglyphs): Improve box border and background coordinate
	computations, drop non-font-backend code.
	* nsterm.m (ns_compute_glyph_string_overhangs): Implemented for real.
	* nsfont.m (open, text_extents, draw): Compute proper bearings in
	open(), use them in text_extents.  In draw(), now handle introductory
	matters previously done in ns_dumpglyphs(), which is no longer used.
	* font.c (font_load_for_face), nsfont.m (draw): Support overstrike for
	families with no bold member.

2007-08-25  Adrian Robert <Adrian.B.Robert@gmail.com>
	* image.c (xpm_load_image): Use lisp strings instead of ints for
	passing NSColor pointers.  (Patch by David M. Cooke.)
	* nsimage.m: Clean up getPixel/setPixel methods, cache a reference to
	an NSBitmapImageRep as well as pixmap data, minor fixes to
	allocInitFromFile, and explicitly cache NSColor colorWithPatternImage
	used for stipple rendering rather than image rep.

2006-12-29  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsfont.m, nsterm.m: Use '_' to substitute for spaces in font names.
	* nsfns.m (ns-font-name): New function to extract font name from XLFD
	for pref-saving purposes.
	* ns-win.el (ns-save-preferences): Use it.  Also, save
	ns-control-modifier, ns-function-modifier.

2006-12-24  Adrian Robert <arobert@cogsci.ucsd.edu>
	* Version NS 9.0-rc1 released: major feature enhancements and bug
	fixes.

2006-12-22  Adrian Robert <arobert@cogsci.ucsd.edu>
	* src/nsterm.m, nsfont.m: Improvements/fixes to clip rect calculation
	and certain drawing cases: fringe now goes to top, and top-row,
	full-width blocks look better.  Also, (EV_BUTTON), make right mouse
	button be mouse-3 to emulate other emacsen.  Finally, recognize
	dead-key modifiers accessible only when SHIFT is pressed (-keyDown).
	* nextstep/compile: Link MacOS/libexec items into MacOS/bin as some
	lisp packages seem to look for them there.

2006-12-21  Adrian Robert <arobert@cogsci.ucsd.edu>
	* lisp/ns-menu-bar.el: Updated menus to Emacs 21+ conventions.

2006-11-27  Adrian Robert <arobert@cogsci.ucsd.edu>
	* src/nsfns.m (x-create-frame): Correctly heed frame geometry
	specifications.
	* src/nsterm.m, nextstep/Cocoa/preferences.nib: Added
	ns-option-modifier (alias for ns-alternate-modifier),
	ns-control-modifier, ns-function-modifier settings.
	* src/ns-win.el (before-make-frame-hook): Behave correctly with
	strange negative-number symbol lists.

2006-11-20  Adrian Robert <arobert@cogsci.ucsd.edu>
	* src/nsfont.m: New file, implementing driver for Kenichi Handa's
	new font back-end.  Font handling code is now simplified, consolidated,
	and more reliable.  Fontsets now implemented and auto-created if a
	font is set for a frame instead of a fontset.
	* lisp/term/ns-win.el: Supporting changes for above.

2006-09-22  Adrian Robert <arobert@cogsci.ucsd.edu>
	* lib-src/Makefile.in: Use autoconf-determined $INSTALL_SCRIPT instead
	of $INSTALL_PROGRAM to install scripts (HAVE_NS only).

2006-09-19  Adrian Robert <arobert@cogsci.ucsd.edu>
	* src/emacs.c: #include GSConfig.h on GNUstep to pick up fake main
	definition to gnustep_base_user_main.
	* src/config.in: Add -I($GNUSTEP_SYSROOT) to C_SWITCH_X_SYSTEM so that
	this works.
	* src/alloc.c: Don't call 'asm("ta 3")' on Sparc / GNUstep systems,
	although maybe this should be Sparc / FreeBSD only.
	* configure.in: Add sparc-*-freebsd* to machine recognition -- though
	not sure why the OS is used in the pattern for machines.

2006-09-07  Adrian Robert <arobert@cogsci.ucsd.edu>
	* src/nsfns.m (ns-yes-or-no-p): Fixed function prototype.

2006-08-27  Adrian Robert <arobert@cogsci.ucsd.edu>
	* lisp/composite.el, loaddefs.el: Do not rely on emacs-basic-display
	being set when initializing global-auto-compose-mode, as it will not
	be when running in CANNOT_DUMP situation.
	* nextstep/compile: Check for 'info' under 'share' (put there in
	some gnustep builds).
	* nextstep/GNUstep/Images/emacs-orange-64.tiff: Make smaller.
	* nextstep/GNUstep/preferences.gorm: Updated for variable name change.
	* nsterm.m (EmacsPrefsController -setValuesFromPanel): Fixed bug in
	setting ExpandSpace.

2006-08-01  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_xlfd_to_fontname): Be more robust against malformed
	XLFD input.
	(ns_get_color): Return calibrated RGB versions of named colors.
	(syms_of_nsterm): Update documentation for modifier key variables.
	* etc/Emacs.clr: Add new colors from recent X11 rgb.txt files.
	(Courtesy of Peter Dyballa <Peter_Dyballa@web.de>.)

2006-06-09  Adrian Robert <arobert@cogsci.ucsd.edu>
	* fontset.c (load_font_get_repertory): Avert a crash by checking
	whether get_font_repertory_func is defined.
	* nsterm.m (ns_draw_window_cursor): Adjust to changes in core.
	(ns_find_ccl_program): New function copied verbatim from xterm.c.
	(-keyDown) Accept alt(=Qnone)-fnKey as fnKey, not mapped characters.

2006-06-08  Adrian Robert <arobert@cogsci.ucsd.edu>
	* Version NS 9.0-pre3 released: major feature enhancements and bug
	fixes.

2006-06-07  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsselect.m (ns_string_from_pasteboard): Convert line endings (EOL)
	on incoming text.

2006-06-06  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (-performDragOperation:): Correct coordinate calculation.
	* ns-win.el (ns-face-at-pos): Correct coordinate calculation.
	(all): Cleaned up / organized file.

2006-06-05  Adrian Robert <arobert@cogsci.ucsd.edu>
	* ns-win.el (ns-working-text-face, ns-working-overlay,
	ns-working-overlay-len): New variables supporting keyboard composition.
	(ns-insert-working-text, ns-delete-working-text): New functions
	supporting keyboard composition.
	* nsterm.m (-keyDown: -insertText:, -deleteWorkingText,
	-setMarkedText:selectedRange): New/updated methods supporting keyboard
	composition.
	(ns_working_text): New variable supporting keyboard composition.

2006-06-03  Adrian Robert <arobert@cogsci.ucsd.edu>
	* ns-win.el (ns-set-background-alpha)
	* nsfns.m (Fns_set_alpha): Add user function ns-set-background-alpha
	for setting window transparency, backed by ns-set-alpha.

2006-06-02  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_dumpglyphs): Further hackery to work around box borders.
	(-windowShouldZoom): New delegate method implementation needed on OS X
	to move window to origin on zoom.
	(ns_set_vertical_scroll_bar): Realize that window->total_lines is a
	lisp integer.

2006-06-01  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m ([EmacsView -keyDown]): Differentiate Backspace, Delete,
	and KP-Delete; also, report Ctrl-(code < 0x20) keys by code
	conversion, rather than solely by modifier flags.

2006-05-30  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_term_init): Set a Windows menu in NSApp so dock menu
	shows window list.

2006-05-29  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_draw_window_cursor): Check in right place for need to
	draw in fringe.
	(ns_load_font): Set rbearings a bit better so italic overhangs
	rendered more or less correctly.

2006-05-28  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsfns.m, nsterm.m, nsmenu.m: Finished toolbar support.
	* nsfns.m, nsterm.h, nsmenu.m: Tooltip support.
	* fontset.c (face_for_char): Shortcircuit a codepath, uneeded under NS,
	that was causing crashes when displaying non-ASCII characters.
	* nsterm.m (ns_define_frame_cursor): Act so cursor correctly updates
	when leaving window.

2006-04-22  Adrian Robert <arobert@cogsci.ucsd.edu>
	* Version NS 9.0-pre2a released: Stopgap release to sync w/latest
	unicode-2 CVS.  XPM + partial toolbar support included.

2006-04-22  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsfns.m, nsterm.m, nsmenu.m: Toolbar support (incomplete).

2006-03-22  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsimage.m, image.c: XPM support.

2005-12-15  Adrian Robert <arobert@cogsci.ucsd.edu>
	* lread.c (init_lread): Stamp out path warning under NS.

2005-12-13  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_init_paths): Fixed bug where .app-internal lisp load
	path was being set even when it did not exist.

2005-11-11  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.h: Set selection color when system default not used to emacs
	default LightGoldenRod2. (Can be changed by customizing 'region'
	face.)
	* man/ns-emacs.texi: Added a node describing Preferences Panel usage
	written by Adam Ratcliffe <adam@prema.co.nz>.
	* nsterm.m ([EmacsPrefsController -runHelp]): Go to this prefs node
	specifically; also, update display.

2005-11-11  Adrian Robert <arobert@cogsci.ucsd.edu>
	* Version NS 9.0-pre2 released: bugfixes and minor feature enhancements.
	* nextstep/compile, **/*.in: Revamped build process to place greater
	load on configure, less on C/CPPFLAGS env variables.  For GNUstep,
	only the GNUSTEP_SYSTEM_ROOT variable must be set, and the compile
	script attempts to get this from /etc/GNUstep/GNUstep.conf.

2005-11-10  Adrian Robert <arobert@cogsci.ucsd.edu>
	* frame.c (syms_of_frame): Default scrollbar to right side under Cocoa.

2005-11-09  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m: Support flexible remapping of Alt/Opt and Command keys.

2005-11-08  Adrian Robert <arobert@cogsci.ucsd.edu>
	* nsterm.m (ns_load_font): Improvement to 11/07 change: new height
	metrics calculation.  Changed "ShrinkSpace" default to "ExpandSpace"
	to reflect its role.  Updated prefs panel.
	* nexstep/compile: Added detection of failure and made configure
	invocation more robust.  Based on suggestion by Peter Dyballa.

2005-11-07  Adrian Robert <arobert@cogsci.ucsd.edu>
        * nsimage.m ([EmacsImage prepareForStippling]): Only cache if both
	height and width less than max.
	* nsterm.m (ns_load_font()): Set font height in XFontStruct (which is
	used for frame line height) to be same as was being set in
	max_bounds.  This makes (window-text-height) correct, as well as
	estimates made in nsterm.m during resizes.

2005-11-06  Adrian Robert <arobert@cogsci.ucsd.edu>
        * nsterm.m ([EmacsView windowDidDeminiaturize]): Check emacs_event,
	since can be invoked asynchronously.

[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#3303: delete-frame raises old (invisible) frame

[Stefan Monnier]

> 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

bug#3303: delete-frame raises old (invisible) frame

[Adrian Robert]

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.

bug#3303: delete-frame raises old (invisible) frame

[David Reitter]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2438 bytes --]