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

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

> 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