bug#32637: 27.0.50; window-size-change-functions not run from local hook

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

0. In emacs -Q open two windows: one with the *scratch* buffer,
another window with the *Messages* buffer with e.g. ‘C-h e’
(view-echo-area-messages).

1. In *scratch* eval:

(add-hook 'window-size-change-functions
	  (lambda (frame) (message "%S" frame))
	  nil t)

2. Resize windows with e.g. ‘C-x {’ (shrink-window-horizontally)
   Check the message with the frame name in *Messages*, fine.

3. Select the window with *Messages* with ‘C-x o’ (other-window),
   and resize again ‘C-x }’

No message in *Messages*, because the hook was not called,
but actually the size of the *scratch* buffer was changed.

window-size-change-functions needs to notify the buffer
that requested such notifications via this hook.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

 > 0. In emacs -Q open two windows: one with the *scratch* buffer,
 > another window with the *Messages* buffer with e.g. ‘C-h e’
 > (view-echo-area-messages).
 >
 > 1. In *scratch* eval:
 >
 > (add-hook 'window-size-change-functions
 > 	  (lambda (frame) (message "%S" frame))
 > 	  nil t)
 >
 > 2. Resize windows with e.g. ‘C-x {’ (shrink-window-horizontally)
 >     Check the message with the frame name in *Messages*, fine.
 >
 > 3. Select the window with *Messages* with ‘C-x o’ (other-window),
 >     and resize again ‘C-x }’
 >
 > No message in *Messages*, because the hook was not called,
 > but actually the size of the *scratch* buffer was changed.
 >
 > window-size-change-functions needs to notify the buffer
 > that requested such notifications via this hook.

This is a legitimate request but I'm not sure whether we should do
that for two reasons:

The first reason is that we already run a buffer-local part of
'window-configuration-change-hook' and I dislike it.  Selecting a
window and making its buffer current for the sake of running a hook is
a bad idea IMO because both, selected window and current buffer, are
vital informations and a function run by a hook should be aware of
them.  Worse even, we already make the selected window's buffer
current first which might defeat the expectations of a function run by
the global hook.

Also the window in question might not have changed at all.  The thing
that did change is the window configuration of a frame and it would be
much more interesting if the hook told me what really has changed
instead of telling me that a buffer's window might have changed.

The second reason is a purely technical one: We'd have to walk the
window tree to find out which buffer may have been affected.  However,
the function run by the local hook would still have to find out which
window(s) shouwing the buffer was (were) potentially affected.  This
means that the buffer-local function would have to walk the window
tree a second time anyway.

But I admit that from the application programmer's POV the behavior
you ask for might be convenient, so patches are welcome.  See the code
of run_window_configuration_change_hook in window.c for how to do that
but please do _not_ introduce any side effects when running the global
'window-size-change-functions'.

martin

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Eli Zaretskii]

> Date: Wed, 05 Sep 2018 09:47:24 +0200
> From: martin rudalics <rudalics@gmx.at>
> 
>  > window-size-change-functions needs to notify the buffer
>  > that requested such notifications via this hook.
> 
> This is a legitimate request but I'm not sure whether we should do
> that for two reasons:
> 
> The first reason is that we already run a buffer-local part of
> 'window-configuration-change-hook' and I dislike it.  Selecting a
> window and making its buffer current for the sake of running a hook is
> a bad idea IMO because both, selected window and current buffer, are
> vital informations and a function run by a hook should be aware of
> them.  Worse even, we already make the selected window's buffer
> current first which might defeat the expectations of a function run by
> the global hook.
> 
> Also the window in question might not have changed at all.  The thing
> that did change is the window configuration of a frame and it would be
> much more interesting if the hook told me what really has changed
> instead of telling me that a buffer's window might have changed.
> 
> The second reason is a purely technical one: We'd have to walk the
> window tree to find out which buffer may have been affected.  However,
> the function run by the local hook would still have to find out which
> window(s) shouwing the buffer was (were) potentially affected.  This
> means that the buffer-local function would have to walk the window
> tree a second time anyway.

I actually don't understand why we need to support buffer-local hooks
in window-size-change-functions.  This hook clearly applies to the
entire frame, so anything buffer-local sounds inappropriate there.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

> I actually don't understand why we need to support buffer-local hooks
> in window-size-change-functions.  This hook clearly applies to the
> entire frame, so anything buffer-local sounds inappropriate there.

A buffer-local hook is useful when a mode needs to reformat its
buffer content on window resizing.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: martin rudalics <rudalics@gmx.at>,  32637@debbugs.gnu.org
> Date: Thu, 06 Sep 2018 00:56:38 +0300
> 
> > I actually don't understand why we need to support buffer-local hooks
> > in window-size-change-functions.  This hook clearly applies to the
> > entire frame, so anything buffer-local sounds inappropriate there.
> 
> A buffer-local hook is useful when a mode needs to reformat its
> buffer content on window resizing.

We have no control on what each hook could or could not do.  We give
programmers a rope, but cannot prevent them from hanging themselves
(and take down Emacs with them).

E.g., what happens if two buffers displayed on the same frame have
different hooks there, which just happen to have conflicting ideas of
what should be done when windows are resized?  How will they be able
to reconcile their conflict?

Makes very little sense to me.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

>> > I actually don't understand why we need to support buffer-local hooks
>> > in window-size-change-functions.  This hook clearly applies to the
>> > entire frame, so anything buffer-local sounds inappropriate there.
>> 
>> A buffer-local hook is useful when a mode needs to reformat its
>> buffer content on window resizing.
>
> We have no control on what each hook could or could not do.  We give
> programmers a rope, but cannot prevent them from hanging themselves
> (and take down Emacs with them).
>
> E.g., what happens if two buffers displayed on the same frame have
> different hooks there, which just happen to have conflicting ideas of
> what should be done when windows are resized?  How will they be able
> to reconcile their conflict?

The problem is that currently Emacs entangles programmers with too much rope
for hanging themselves.  See for example how programmers are currently
struggling to emulate buffer-local window-size-change-functions:

  (define-derived-mode bs-mode nil "Buffer-Selection-Menu"
    ...
    (add-hook 'window-size-change-functions 'bs--track-window-changes)
    (add-hook 'kill-buffer-hook 'bs--remove-hooks nil t)
    (add-hook 'change-major-mode-hook 'bs--remove-hooks nil t))

  (defun bs--remove-hooks ()
    "Remove `bs--track-window-changes' and auxiliary hooks."
    (remove-hook 'window-size-change-functions 'bs--track-window-changes)
    ;; Remove itself
    (remove-hook 'kill-buffer-hook 'bs--remove-hooks t)
    (remove-hook 'change-major-mode-hook 'bs--remove-hooks t))

where "Remove itself" indeed sounds like "Hang itself".

We can disentangle programmers from rope to avoid such fragile workarounds
by allowing buffer-local hooks with just

  (define-derived-mode bs-mode nil "Buffer-Selection-Menu"
    ...
    (add-hook 'window-size-change-functions 'bs--track-window-changes nil t))

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

>> window-size-change-functions needs to notify the buffer
>> that requested such notifications via this hook.
>
> This is a legitimate request but I'm not sure whether we should do
> that for two reasons:
>
> The first reason is that we already run a buffer-local part of
> 'window-configuration-change-hook'

Aha, I didn't know that window-configuration-change-hook already
supports this useful feature!

> Selecting a window and making its buffer current for the sake of
> running a hook is a bad idea IMO because both, selected window and
> current buffer, are vital informations and a function run by a hook
> should be aware of them.  Worse even, we already make the selected
> window's buffer current first which might defeat the expectations of
> a function run by the global hook.

> Also the window in question might not have changed at all.  The thing
> that did change is the window configuration of a frame and it would be
> much more interesting if the hook told me what really has changed
> instead of telling me that a buffer's window might have changed.
>
> The second reason is a purely technical one: We'd have to walk the
> window tree to find out which buffer may have been affected.  However,
> the function run by the local hook would still have to find out which
> window(s) shouwing the buffer was (were) potentially affected.  This
> means that the buffer-local function would have to walk the window
> tree a second time anyway.

It seems what you describe is inefficiency of the current implementation,
but currently I have nothing to say about it, sorry.  What I see is that
the same logic needs to be applied to both window-configuration-change-hook
and window-size-change-functions.

All the doubts that you raised above can be solved only when these hooks
will get more usage, and you will get more reports asking for improvements.
This is one of such reports, thanks for your help!

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

 > It seems what you describe is inefficiency of the current implementation,
 > but currently I have nothing to say about it, sorry.  What I see is that
 > the same logic needs to be applied to both window-configuration-change-hook
 > and window-size-change-functions.

 From an application programmer's view the expected behavior of running
'window-size-change-functions' buffer-locally would be to call the
function (1) only if a window displays the buffer now and (2) only if
the size of that window actually changed.  And it should call the
function with that window as argument.  Which means that for any
buffer the function could be called as many times as a window showing
the buffer has changed its size.  It also means that we don't call the
function when a buffer has disappeared from the frame.

Now since the argument of 'window-size-change-functions' is the
associated frame, I agree with Eli that running it buffer-locally
doesn't make much sense.  The apparent gain in convenience is dwarfed
by the fact that one would still have to find the window(s) showing
the buffer.

And I still think that running 'window-configuration-change-hook'
buffer-locally in its current from hardly makes sense either: For
example, we don't call it for a buffer when that buffer has been
removed from a window which incidentally is the case that would allow
Man to remove its function from 'window-configuration-change-hook'.

martin

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

> From an application programmer's view the expected behavior of running
> 'window-size-change-functions' buffer-locally would be to call the
> function (1) only if a window displays the buffer now and (2) only if
> the size of that window actually changed.  And it should call the
> function with that window as argument.  Which means that for any
> buffer the function could be called as many times as a window showing
> the buffer has changed its size.  It also means that we don't call the
> function when a buffer has disappeared from the frame.
>
> Now since the argument of 'window-size-change-functions' is the
> associated frame, I agree with Eli that running it buffer-locally
> doesn't make much sense.  The apparent gain in convenience is dwarfed
> by the fact that one would still have to find the window(s) showing
> the buffer.

But in bug#32536 you agreed that Man-window-size-change has to
take care of cases where buffer-local window-size-change-functions
needs to find all Man windows on the frame to compare their sizes
and reformat the buffer with the minimim width from all its windows.

So the window-size-change-functions hook should call the function not
with the window as argument, but with the whole frame, as it already
does now.  And to call the buffer-local hook only once for all
affected windows, as it already does now (the hook has responsibility
to find all its windows from the frame).

> And I still think that running 'window-configuration-change-hook'
> buffer-locally in its current from hardly makes sense either: For
> example, we don't call it for a buffer when that buffer has been
> removed from a window which incidentally is the case that would allow
> Man to remove its function from 'window-configuration-change-hook'.

There is no need to remove function from the buffer-local hook,
because it is called only when the buffer is displayed
in a window.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

 > But in bug#32536 you agreed that Man-window-size-change has to
 > take care of cases where buffer-local window-size-change-functions
 > needs to find all Man windows on the frame to compare their sizes
 > and reformat the buffer with the minimim width from all its windows.
 >
 > So the window-size-change-functions hook should call the function not
 > with the window as argument, but with the whole frame, as it already
 > does now.  And to call the buffer-local hook only once for all
 > affected windows, as it already does now (the hook has responsibility
 > to find all its windows from the frame).

I meant that running 'window-size-change-functions' in a buffer-local
fashion when no window showing that buffer has changed its size might
be misleading.

But put a buffer-local function on 'window-configuration-change-hook'
and show the buffer in two windows.  The function gets called twice
with the respective window selected.  So if we implemented
'window-size-change-functions' in the same way as you suggested
earlier, you would "find all Man windows on the frame to compare their
sizes and reformat the buffer with the minimim width from all its
windows" twice.  How would you deal with that?

 >> And I still think that running 'window-configuration-change-hook'
 >> buffer-locally in its current from hardly makes sense either: For
 >> example, we don't call it for a buffer when that buffer has been
 >> removed from a window which incidentally is the case that would allow
 >> Man to remove its function from 'window-configuration-change-hook'.
 >
 > There is no need to remove function from the buffer-local hook,
 > because it is called only when the buffer is displayed
 > in a window.

As you noted earlier, a buffer-local 'window-size-change-functions'
function gets called regardless of whether the buffer is shown in a
window on the frame in question.  So I suppose that's a function you
wanted to remove at least as long as we do not change the present
code.  And if we wanted to fix that, we probably need something better
than imitating 'window-configuration-change-hook'.

martin

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

>> But in bug#32536 you agreed that Man-window-size-change has to
>> take care of cases where buffer-local window-size-change-functions
>> needs to find all Man windows on the frame to compare their sizes
>> and reformat the buffer with the minimim width from all its windows.
>>
>> So the window-size-change-functions hook should call the function not
>> with the window as argument, but with the whole frame, as it already
>> does now.  And to call the buffer-local hook only once for all
>> affected windows, as it already does now (the hook has responsibility
>> to find all its windows from the frame).
>
> I meant that running 'window-size-change-functions' in a buffer-local
> fashion when no window showing that buffer has changed its size might
> be misleading.

window-size-change-functions calling code could detect if a window with
a buffer-local hook changed its size, and not to call its hook in this
case.  This would be even better than using global hook where you can't
implement such optimization.

> But put a buffer-local function on 'window-configuration-change-hook'
> and show the buffer in two windows.  The function gets called twice
> with the respective window selected.  So if we implemented
> 'window-size-change-functions' in the same way as you suggested
> earlier, you would "find all Man windows on the frame to compare their
> sizes and reformat the buffer with the minimim width from all its
> windows" twice.  How would you deal with that?

window-size-change-functions calling code could call it only once
for every frame, even if the same buffer is displayed in multiple
windows.

>>> And I still think that running 'window-configuration-change-hook'
>>> buffer-locally in its current from hardly makes sense either: For
>>> example, we don't call it for a buffer when that buffer has been
>>> removed from a window which incidentally is the case that would allow
>>> Man to remove its function from 'window-configuration-change-hook'.
>>
>> There is no need to remove function from the buffer-local hook,
>> because it is called only when the buffer is displayed
>> in a window.
>
> As you noted earlier, a buffer-local 'window-size-change-functions'
> function gets called regardless of whether the buffer is shown in a
> window on the frame in question.

I see that it's not called when the buffer is not displayed in any
window on the frame - this is correct.  But what I noted earlier is that
it's not called when the buffer with the buffer-local hook is not the
current-buffer in the selected window (but it's displayed in a
non-selected window on the frame) - this should be fixed to call the hook
regardless if its window is currently selected or not.

> And if we wanted to fix that, we probably need something better than
> imitating 'window-configuration-change-hook'.

I think window-configuration-change-hook is a step forward in the
right direction.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 09 Sep 2018 02:56:35 +0300
> Cc: 32637@debbugs.gnu.org
> 
> > I meant that running 'window-size-change-functions' in a buffer-local
> > fashion when no window showing that buffer has changed its size might
> > be misleading.
> 
> window-size-change-functions calling code could detect if a window with
> a buffer-local hook changed its size, and not to call its hook in this
> case.  This would be even better than using global hook where you can't
> implement such optimization.

This might be very hard to implement, given how these hooks are called
today.  In fact, it might be impossible to implement without changing
the strategy of running this hook, and that could very well change the
semantics of the hook, and break some existing packages that use them.

> > But put a buffer-local function on 'window-configuration-change-hook'
> > and show the buffer in two windows.  The function gets called twice
> > with the respective window selected.  So if we implemented
> > 'window-size-change-functions' in the same way as you suggested
> > earlier, you would "find all Man windows on the frame to compare their
> > sizes and reformat the buffer with the minimim width from all its
> > windows" twice.  How would you deal with that?
> 
> window-size-change-functions calling code could call it only once
> for every frame, even if the same buffer is displayed in multiple
> windows.

AFAIK, window-size-change-functions is already called once per frame,
so I'm not sure what you are saying here.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

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

 > window-size-change-functions calling code could detect if a window with
 > a buffer-local hook changed its size, and not to call its hook in this
 > case.  This would be even better than using global hook where you can't
 > implement such optimization.

Agreed.

 > window-size-change-functions calling code could call it only once
 > for every frame, even if the same buffer is displayed in multiple
 > windows.

Agreed.

 > I see that it's not called when the buffer is not displayed in any
 > window on the frame - this is correct.  But what I noted earlier is that
 > it's not called when the buffer with the buffer-local hook is not the
 > current-buffer in the selected window (but it's displayed in a
 > non-selected window on the frame) - this should be fixed to call the hook
 > regardless if its window is currently selected or not.

Agreed.  I attach an untested and undocumented patch.  Please have a
look.

martin

[-- Attachment #2: window.c.diff --]
[-- Type: text/plain, Size: 2501 bytes --]

--- a/src/window.c
+++ b/src/window.c
@@ -3443,7 +3443,11 @@ depends on the value of (window-start WINDOW), so if calling this
 {
   struct frame *f = XFRAME (frame);
   struct window *r = XWINDOW (FRAME_ROOT_WINDOW (f));
-  Lisp_Object functions = Vwindow_size_change_functions;
+
+  if (NILP (Vrun_hooks)
+      || !(f->can_x_set_window_size)
+      || !(f->after_make_frame))
+    return;
 
   if (FRAME_WINDOW_CONFIGURATION_CHANGED (f)
       /* Here we implicitly exclude the possibility that the height of
@@ -3451,11 +3455,42 @@ depends on the value of (window-start WINDOW), so if calling this
 	 of FRAME's root window alone.  */
       || window_size_changed (r))
     {
-      while (CONSP (functions))
+      Lisp_Object globals = Fdefault_value (Qwindow_size_change_functions);
+      Lisp_Object windows = Fwindow_list (frame, Qlambda, Qnil);
+      /* The buffers for which the local hook was already run.  */
+      Lisp_Object buffers = Qnil;
+
+      for (; CONSP (windows); windows = XCDR (windows))
+	{
+	  Lisp_Object window = XCAR (windows);
+	  Lisp_Object buffer = Fwindow_buffer (window);
+
+	  /* Run local hook only if the window really changed size
+	     and only once for each buffer in case the buffer is
+	     shown in more than one window of that frame.  */
+	  if (window_size_changed (XWINDOW (window))
+	      && !Fmemq (buffer, buffers)
+	      && Flocal_variable_p (Qwindow_size_change_functions, buffer))
+	    {
+	      Lisp_Object locals
+		= Fbuffer_local_value (Qwindow_size_change_functions, buffer);
+
+	      while (CONSP (locals))
+		{
+		  if (!EQ (XCAR (locals), Qt))
+		    safe_call1 (XCAR (locals), frame);
+		  locals = XCDR (locals);
+		}
+
+	      buffers = Fcons (buffer, buffers);
+	    }
+	}
+
+      while (CONSP (globals))
 	{
-	  if (!EQ (XCAR (functions), Qt))
-	    safe_call1 (XCAR (functions), frame);
-	  functions = XCDR (functions);
+	  if (!EQ (XCAR (globals), Qt))
+	    safe_call1 (XCAR (globals), frame);
+	  globals = XCDR (globals);
 	}
 
       window_set_before_size_change_sizes (r);
@@ -7557,6 +7592,7 @@ Value is a list of the form (WIDTH COLUMNS VERTICAL-TYPE HEIGHT LINES
   Fput (Qscroll_down, Qscroll_command, Qt);
 
   DEFSYM (Qwindow_configuration_change_hook, "window-configuration-change-hook");
+  DEFSYM (Qwindow_size_change_functions, "window-size-change-functions");
   DEFSYM (Qwindowp, "windowp");
   DEFSYM (Qwindow_configuration_p, "window-configuration-p");
   DEFSYM (Qwindow_live_p, "window-live-p");

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Eli Zaretskii]

> Date: Sun, 09 Sep 2018 10:40:33 +0200
> From: martin rudalics <rudalics@gmx.at>
> Cc: 32637@debbugs.gnu.org
> 
> +	  /* Run local hook only if the window really changed size
> +	     and only once for each buffer in case the buffer is
> +	     shown in more than one window of that frame.  */

I'm not sure this is TRT.  In particular, for buffers that are shown
in more than one window, this will run the hook in a window that is
basically randomly chosen out of those displaying the buffer.  How can
we justify such a behavior?

Thanks.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

 > I'm not sure this is TRT.  In particular, for buffers that are shown
 > in more than one window, this will run the hook in a window that is
 > basically randomly chosen out of those displaying the buffer.  How can
 > we justify such a behavior?

By design, the hook is run in the frame and not in a window showing
the buffer.  The designer of a buffer-local function for that hook now
can be sure that

(1) whenever something happens the function should care about, the
function gets called, and

(2) whenever the function gets called, something happened the function
should care about.

That's all a patch for this can accomplish given the design limits of
'window-size-change-functions'.

martin

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[Juri Linkov]

>> window-size-change-functions calling code could detect if a window with
>> a buffer-local hook changed its size, and not to call its hook in this
>> case.  This would be even better than using global hook where you can't
>> implement such optimization.
>
> Agreed.
>
>> window-size-change-functions calling code could call it only once
>> for every frame, even if the same buffer is displayed in multiple
>> windows.
>
> Agreed.
>
>> I see that it's not called when the buffer is not displayed in any
>> window on the frame - this is correct.  But what I noted earlier is that
>> it's not called when the buffer with the buffer-local hook is not the
>> current-buffer in the selected window (but it's displayed in a
>> non-selected window on the frame) - this should be fixed to call the hook
>> regardless if its window is currently selected or not.
>
> Agreed.  I attach an untested and undocumented patch.  Please have a
> look.

Thank you for the patch.  I tried it and it works without problems.
I'm testing it on the image window resizing in bug#32672
and it calls window-size-change-functions regardless if the current
buffer is in image-mode, or if the image buffer in another window.

bug#32637: 27.0.50; window-size-change-functions not run from local hook

[martin rudalics]

tags 32637 fixed
close 32637 27.1
quit

 > Thank you for the patch.  I tried it and it works without problems.

Installed on master, bug closed.

Thanks for trying, martin