Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[Po Lu]

martin rudalics <rudalics@gmx.at> writes:

> Thanks, the problem seems fixed here.  Could you please also add a
> one-liner like

Shouldn't we not run hooks for such events with dead frames?

> --- a/lisp/x-dnd.el
> +++ b/lisp/x-dnd.el
> @@ -600,8 +600,8 @@ x-dnd-init-xdnd-for-frame
>  (defun x-dnd-after-move-frame (frame)
>    "Handle FRAME moving to a different position.
>  Clear any cached root window position."
> -  (set-frame-parameter frame 'dnd-root-window-position
> -                       nil))
> +  (when (frame-live-p frame)
> +    (set-frame-parameter frame 'dnd-root-window-position nil)))
>
>  (add-hook 'move-frame-functions #'x-dnd-after-move-frame)
>
> since otherwise I'm getting (with my customizations) a backtrace like
>
> Debugger entered--Lisp error: (wrong-type-argument frame-live-p #<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>)
>   modify-frame-parameters(#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58> ((dnd-root-window-position)))
>   set-frame-parameter(#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58> dnd-root-window-position nil)
>   x-dnd-after-move-frame(#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>)
>   run-hook-with-args(x-dnd-after-move-frame #<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>)
>   handle-move-frame((move-frame (#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>)))
>   funcall-interactively(handle-move-frame (move-frame (#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>)))
>   call-interactively(handle-move-frame nil [(move-frame (#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>))])
>   command-execute(handle-move-frame nil [(move-frame (#<dead frame *scratch* - GNU Emacs at restno 0x2e1dc58>))] t)

Thanks.

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[martin rudalics]

 >> Thanks, the problem seems fixed here.  Could you please also add a
 >> one-liner like
 >
 > Shouldn't we not run hooks for such events with dead frames?

You mean something like

diff --git a/lisp/frame.el b/lisp/frame.el
index 56a982562d..c7181d9162 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -239,7 +239,8 @@ handle-move-frame
  This function runs the abnormal hook `move-frame-functions'."
    (interactive "e")
    (let ((frame (posn-window (event-start event))))
-    (run-hook-with-args 'move-frame-functions frame)))
+    (when (frame-live-p frame)
+      (run-hook-with-args 'move-frame-functions frame))))
  \f
  ;;;; Arrangement of frames at startup

and you're certainly right.

martin

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[Po Lu via Emacs development discussions.]

martin rudalics <rudalics@gmx.at> writes:

> You mean something like
>
> diff --git a/lisp/frame.el b/lisp/frame.el
> index 56a982562d..c7181d9162 100644
> --- a/lisp/frame.el
> +++ b/lisp/frame.el
> @@ -239,7 +239,8 @@ handle-move-frame
>  This function runs the abnormal hook `move-frame-functions'."
>    (interactive "e")
>    (let ((frame (posn-window (event-start event))))
> -    (run-hook-with-args 'move-frame-functions frame)))
> +    (when (frame-live-p frame)
> +      (run-hook-with-args 'move-frame-functions frame))))
>  \f
>  ;;;; Arrangement of frames at startup
>
> and you're certainly right.
>
> martin

Right, but maybe deeper in keyboard.c, before `handle-move-frame' is
called.  But then, other event types might also be involved, so I don't
quite know how deeply rooted the problem is.

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[martin rudalics]

 > Right, but maybe deeper in keyboard.c, before `handle-move-frame' is
 > called.  But then, other event types might also be involved, so I don't
 > quite know how deeply rooted the problem is.

If we make it too "deep", the frame might still be alive and thus escape
the check.

martin

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[Po Lu]

martin rudalics <rudalics@gmx.at> writes:

> If we make it too "deep", the frame might still be alive and thus escape
> the check.

How so?

I'm thinking of the place where keyboard.c calls handle-focus-in, namely
here:

  /* Process special events within read_char
     and loop around to read another event.  */
  save = Vquit_flag;
  Vquit_flag = Qnil;
  tem = access_keymap (get_keymap (Vspecial_event_map, 0, 1), c, 0, 0, 1);
  Vquit_flag = save;

  if (!NILP (tem))
    {
      struct buffer *prev_buffer = current_buffer;
      last_input_event = c;
      call4 (Qcommand_execute, tem, Qnil, Fvector (1, &last_input_event), Qt); <===

      if (CONSP (c) && !NILP (Fmemq (XCAR (c), Vwhile_no_input_ignore_events))
	  && !end_time)
	/* We stopped being idle for this event; undo that.  This
	   prevents automatic window selection (under
	   mouse-autoselect-window) from acting as a real input event, for
	   example banishing the mouse under mouse-avoidance-mode.  */
	timer_resume_idle ();

#ifdef HAVE_NS
      if (CONSP (c)
          && (EQ (XCAR (c), intern ("ns-unput-working-text"))))
        input_was_pending = input_pending;
#endif

      if (current_buffer != prev_buffer)
	{
	  /* The command may have changed the keymaps.  Pretend there
	     is input in another keyboard and return.  This will
	     recalculate keymaps.  */
	  c = make_fixnum (-2);
	  goto exit;
	}
      else
	goto retry;
    }

Re: master 05705454d5: Don't transfer window attributes trying to find the XM drag window

[martin rudalics]

 > I'm thinking of the place where keyboard.c calls handle-focus-in, namely
 > here:
...
 >        call4 (Qcommand_execute, tem, Qnil, Fvector (1, &last_input_event), Qt); <===

What would you want to do here?  Bow out if the first element is a dead
frame or window?  That still doesn't protect us from having two
functions on such a hook where the one running first deletes the frame.

So while I think that your suggestion probably won't harm, it might
still not catch us all use cases.  We could invent things like
'run-frame-hook-with-args' that checks whether the frame is still live
before calling the next function on the hook.  But ultimately my first
proposal to have 'x-dnd-after-move-frame' check whether the frame is
live might still be the safest solution.  Plus adding some text that all
functions on any of these hooks should check whether their frame,
window, ... argument is live.  In particular when they are installed by
default as 'x-dnd-after-move-frame'.

Incidentally, the problem I see seems to show up because I'm using a
stand-alone minibuffer frame and when that is made in
'frame-notice-user-settings', master kills the initial frame.  I haven't
tried to find out who moves that initial frame before that though.

martin