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

Previous Next

Full log

View this message in rfc822 format

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: bug#3303: 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

Previous Next

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