bug#32720: term-mode ignores certain window size changes

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Gary Fredericks
Affects: version 26 (git-bisected to commit 8e7712c7afc)

Note: I have only tried this on --without-x emacs

Steps to reproduce
  1. start emacs
  2. start a term buffer with M-x term
  3. run `seq 1000` at the bash prompt to fill the screen
  4. enlarge the terminal window that emacs is running in, so that the window size changes as well
  5. run `seq 1000` again, and observe that the new space at the bottom of the buffer is not being used
Analysis notes

term-mode does pick up changes after more explicit window configurations, like splits; my workaround for months has been to split and join the terminal window whenever I've resized it.

As best I can tell, term-mode subscribes to window size changes by adding advice to the window-adjust-process-window-size-function variable, and the 8e7712c7afc reduced the set of situations in which that function is called.

I've developed a more automated workaround with a term-load-hook of this form:

(add-hook 'window-size-change-functions (lambda (_frame) (window--adjust-process-windows)))

It might be that adding this line to the term-mode setup steps would be sufficient, but I'm not familiar enough with the window.el code to have a guess whether that's actually a good approach.

Gary Fredericks
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
> *Affects*: version 26 (git-bisected to commit 8e7712c7afc)
>
> Note: I have only tried this on --without-x emacs
>
> *Steps to reproduce*
>
>     1. start emacs
>     2. start a term buffer with M-x term
>     3. run `seq 1000` at the bash prompt to fill the screen
>     4. enlarge the terminal window that emacs is running in, so that the
>     window size changes as well
>     5. run `seq 1000` again, and observe that the new space at the bottom of
>     the buffer is not being used
>
> *Analysis notes*
>
> term-mode *does* pick up changes after more explicit window configurations,
> like splits; my workaround for months has been to split and join the
> terminal window whenever I've resized it.
>
> As best I can tell, term-mode subscribes to window size changes by adding
> advice to the window-adjust-process-window-size-function variable, and the
> 8e7712c7afc reduced the set of situations in which that function is called.
>
> I've developed a more automated workaround with a term-load-hook of this
> form:
>
> (add-hook 'window-size-change-functions (lambda (_frame)
> (window--adjust-process-windows)))
>
> It might be that adding this line to the term-mode setup steps would be
> sufficient, but I'm not familiar enough with the window.el code to have a
> guess whether that's actually a good approach.

Emacs no more runs 'window-configuration-change-hook' when the frame
size changes so your workaround should indeed work around that case.
But you should see a similar problem before commit 8e7712c7afc with a
frame containing a window showing process output and that window's
size gets changed.

In its current form, 'window--adjust-process-windows' is a gross hack.
Putting a function by default on 'window-configuration-change-hook'
(or 'window-size-change-functions') is a bad idea IMO.  There should
be no place for such functions in window.el - a special mode should
take care of them.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
> Date: Thu, 13 Sep 2018 10:07:06 +0200
> From: martin rudalics <[hidden email]>
>
> In its current form, 'window--adjust-process-windows' is a gross hack.
> Putting a function by default on 'window-configuration-change-hook'
> (or 'window-size-change-functions') is a bad idea IMO.

Can you elaborate on why do you think these are bad ideas?



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 >> In its current form, 'window--adjust-process-windows' is a gross hack.
 >> Putting a function by default on 'window-configuration-change-hook'
 >> (or 'window-size-change-functions') is a bad idea IMO.
 >
 > Can you elaborate on why do you think these are bad ideas?

(1) Preempting a hook is like preempting an option.  And options
should be pristine when starting Emacs.  Users who never run any
processes should not be obliged to remove anything from a hook to
obtain a clean run.  Even if we say that it does not matter much in
the case at hand, this code sets a precedent which others may follow.

(2) I don't know what a "logical" window size is.  Reading the
doc-string of 'window-adjust-process-window-size-function', I
understand that the function this variable is set to should pass the
size of some window showing output of a process to that process.  If
this interpretation is correct, then not hooking into
'window-size-change-functions' will fail to capture explicit resizing
of any such window - usually the most prominent case when a window
size changes.  So what's the aim of 'window--adjust-process-windows'?

In either case, it should not be the task of window.el to find process
windows.  It does not find "Man" or "Info" windows either and calls a
hook in a hook when resizing them although someone might find that
convenient - compare Bug#32536.  Window groups are the most prominent
other example of code that usurpated window.el.  I never understood
why code which pertains to 'follow-mode' was added to window.el.
Adding such code makes the already largest code file in the Lisp
directory more and more difficult to navigate.

BTW, the info text "When windows that display buffers associated with
process change their dimensions, the affected processes should be told
about these changes" seems to lack an "a" before "process".

And the text "If the process has the `adjust-window-size-function'
property (*note Process Information::), its value overrides the global
and buffer-local values of
`window-adjust-process-window-size-function'." is misleading.  Section
38.6 Process Information does not mention any such property and I have
no idea what it's supposed to accomplish.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
In reply to this post by Gary Fredericks
 > *Affects*: version 26 (git-bisected to commit 8e7712c7afc)
 >
 > Note: I have only tried this on --without-x emacs
 >
 > *Steps to reproduce*
 >
 >     1. start emacs
 >     2. start a term buffer with M-x term
 >     3. run `seq 1000` at the bash prompt to fill the screen
 >     4. enlarge the terminal window that emacs is running in, so that the
 >     window size changes as well
 >     5. run `seq 1000` again, and observe that the new space at the bottom of
 >     the buffer is not being used
 >
 > *Analysis notes*
 >
 > term-mode *does* pick up changes after more explicit window configurations,
 > like splits; my workaround for months has been to split and join the
 > terminal window whenever I've resized it.
 >
 > As best I can tell, term-mode subscribes to window size changes by adding
 > advice to the window-adjust-process-window-size-function variable, and the
 > 8e7712c7afc reduced the set of situations in which that function is called.
 >
 > I've developed a more automated workaround with a term-load-hook of this
 > form:
 >
 > (add-hook 'window-size-change-functions (lambda (_frame)
 > (window--adjust-process-windows)))
 >
 > It might be that adding this line to the term-mode setup steps would be
 > sufficient, but I'm not familiar enough with the window.el code to have a
 > guess whether that's actually a good approach.

While all you write above is correct and I agree with the fix you
propose, I tend to restore the old behavior as with the attached
patch.  The reason is that I have no idea in how many more yet
unrevealed occasions my commit had adverse effects so I'd rather be on
the safe side for Emacs 26.2.  Please try the patch and tell me
whether it indeed restores the old behavior.

Thanks, martin

run-window-configuration-change.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Gary Fredericks
Yep, it seems to work with the patch applied.

Gary Fredericks
(803)-295-0195
[hidden email]
gfredericks.com


On Sun, Nov 4, 2018 at 3:56 AM martin rudalics <[hidden email]> wrote:
 > *Affects*: version 26 (git-bisected to commit 8e7712c7afc)
 >
 > Note: I have only tried this on --without-x emacs
 >
 > *Steps to reproduce*
 >
 >     1. start emacs
 >     2. start a term buffer with M-x term
 >     3. run `seq 1000` at the bash prompt to fill the screen
 >     4. enlarge the terminal window that emacs is running in, so that the
 >     window size changes as well
 >     5. run `seq 1000` again, and observe that the new space at the bottom of
 >     the buffer is not being used
 >
 > *Analysis notes*
 >
 > term-mode *does* pick up changes after more explicit window configurations,
 > like splits; my workaround for months has been to split and join the
 > terminal window whenever I've resized it.
 >
 > As best I can tell, term-mode subscribes to window size changes by adding
 > advice to the window-adjust-process-window-size-function variable, and the
 > 8e7712c7afc reduced the set of situations in which that function is called.
 >
 > I've developed a more automated workaround with a term-load-hook of this
 > form:
 >
 > (add-hook 'window-size-change-functions (lambda (_frame)
 > (window--adjust-process-windows)))
 >
 > It might be that adding this line to the term-mode setup steps would be
 > sufficient, but I'm not familiar enough with the window.el code to have a
 > guess whether that's actually a good approach.

While all you write above is correct and I agree with the fix you
propose, I tend to restore the old behavior as with the attached
patch.  The reason is that I have no idea in how many more yet
unrevealed occasions my commit had adverse effects so I'd rather be on
the safe side for Emacs 26.2.  Please try the patch and tell me
whether it indeed restores the old behavior.

Thanks, martin
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 > Yep, it seems to work with the patch applied.

Thanks for the feedback.  But after more carefully looking into the
'window-adjust-process-windows' code I am now almost convinced that
your original proposal is the way to go.  That is, I nowhere found in
that code anything that depends on the change of the window
configuration.  The only thing that code is interested in is tracing
window size changes which 'window-configuration-change-hook' does not
handle particularly well.

So if you already have run Emacs for some time with your

(add-hook
  'window-size-change-functions
  (lambda (_frame) (window--adjust-process-windows)))

and did not see any adversary effects I am inclined to install that
for Emacs 26.2.  For Emacs 27 I would then (after the release of Emacs
26.2) remove the

(add-hook 'window-configuration-change-hook 'window--adjust-process-windows)

call.

Eli would that be OK with you as well?  I'd still have to amend the
doc of 'window-configuration-change-hook' but the net change would be
considerably smaller than the one I proposed in the other thread.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Gary Fredericks
Confirmed that I've been using that add-hook call for a while (ever since I originally reported this) and haven't noticed any adverse effects.

Gary Fredericks
(803)-295-0195
[hidden email]
gfredericks.com


On Fri, Nov 16, 2018 at 6:58 AM martin rudalics <[hidden email]> wrote:
 > Yep, it seems to work with the patch applied.

Thanks for the feedback.  But after more carefully looking into the
'window-adjust-process-windows' code I am now almost convinced that
your original proposal is the way to go.  That is, I nowhere found in
that code anything that depends on the change of the window
configuration.  The only thing that code is interested in is tracing
window size changes which 'window-configuration-change-hook' does not
handle particularly well.

So if you already have run Emacs for some time with your

(add-hook
  'window-size-change-functions
  (lambda (_frame) (window--adjust-process-windows)))

and did not see any adversary effects I am inclined to install that
for Emacs 26.2.  For Emacs 27 I would then (after the release of Emacs
26.2) remove the

(add-hook 'window-configuration-change-hook 'window--adjust-process-windows)

call.

Eli would that be OK with you as well?  I'd still have to amend the
doc of 'window-configuration-change-hook' but the net change would be
considerably smaller than the one I proposed in the other thread.

martin
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
In reply to this post by martin rudalics
> Date: Fri, 16 Nov 2018 13:57:51 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email], "[hidden email]" <[hidden email]>
>
> So if you already have run Emacs for some time with your
>
> (add-hook
>   'window-size-change-functions
>   (lambda (_frame) (window--adjust-process-windows)))
>
> and did not see any adversary effects I am inclined to install that
> for Emacs 26.2.  For Emacs 27 I would then (after the release of Emacs
> 26.2) remove the
>
> (add-hook 'window-configuration-change-hook 'window--adjust-process-windows)
>
> call.
>
> Eli would that be OK with you as well?

I don't think I have a clear idea of what is being proposed for
emacs-26.



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 > I don't think I have a clear idea of what is being proposed for
 > emacs-26.

I propose the attached changes.

martin

window-configuration-change-hook.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
> Date: Sat, 17 Nov 2018 10:20:55 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email], [hidden email]
>
>  > I don't think I have a clear idea of what is being proposed for
>  > emacs-26.
>
> I propose the attached changes.

OK (although I don't like putting functions on hooks in the core).

> +;; Remove the following call in Emacs 27, running
> +;; 'window-size-change-functions' should suffice.
>  (add-hook 'window-configuration-change-hook 'window--adjust-process-windows)

Why not do that now?



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 > OK (although I don't like putting functions on hooks in the core).

Neither do I.

 >> +;; Remove the following call in Emacs 27, running
 >> +;; 'window-size-change-functions' should suffice.
 >>   (add-hook 'window-configuration-change-hook 'window--adjust-process-windows)
 >
 > Why not do that now?

Because Gary has not tested it ;-)

More seriously: I have no idea how 'adjust-window-size-function' and
the function specified by 'window-adjust-process-window-size-function'
work in practice.  If they rely on anything beyond a window's
dimensions (for example, on which other buffers are displayed on a
frame), then not calling 'window--adjust-process-windows' for
'window-configuration-change-hook' might miss something.  I'd rather
reserve such subtleties for Emacs 27.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
> Date: Sat, 17 Nov 2018 19:40:01 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email], [hidden email]
>
>  >> +;; Remove the following call in Emacs 27, running
>  >> +;; 'window-size-change-functions' should suffice.
>  >>   (add-hook 'window-configuration-change-hook 'window--adjust-process-windows)
>  >
>  > Why not do that now?
>
> Because Gary has not tested it ;-)
>
> More seriously: I have no idea how 'adjust-window-size-function' and
> the function specified by 'window-adjust-process-window-size-function'
> work in practice.  If they rely on anything beyond a window's
> dimensions (for example, on which other buffers are displayed on a
> frame), then not calling 'window--adjust-process-windows' for
> 'window-configuration-change-hook' might miss something.  I'd rather
> reserve such subtleties for Emacs 27.

I agree with doing this for Emacs 27, I'm asking why not do this now
on master?



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
In reply to this post by Gary Fredericks
fixed 32720 26.2

 > Confirmed that I've been using that add-hook call for a while (ever since I
 > originally reported this) and haven't noticed any adverse effects.

I pushed your fix now to the release branch as commit
88762b4063a42a69234bda74b1626b646734715a.  If you are interested in
the further development of this, please run Emacs with the line

(add-hook 'window-configuration-change-hook 'window--adjust-process-windows)

removed and tell us whether it causes any problems.

Closing this bug (hopefully).

Thanks, martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
In reply to this post by Eli Zaretskii
 > I agree with doing this for Emacs 27, I'm asking why not do this now
 > on master?

Because most people run master and I'm interested in testing whether
the interaction between 'window-configuration-change-hook' and
'window-size-change-functions' could cause any problems.  As soon as
Emacs 26.2 is released (and if Gary doesn't see any problems in the
meantime) I make the change on master.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
> Date: Sun, 18 Nov 2018 10:22:40 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email], [hidden email]
>
>  > I agree with doing this for Emacs 27, I'm asking why not do this now
>  > on master?
>
> Because most people run master and I'm interested in testing whether
> the interaction between 'window-configuration-change-hook' and
> 'window-size-change-functions' could cause any problems.

Indeed, doing this on master now will allow us to collect testing
experience sooner, so I think it's a win.



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 >> Because most people run master and I'm interested in testing whether
 >> the interaction between 'window-configuration-change-hook' and
 >> 'window-size-change-functions' could cause any problems.
 >
 > Indeed, doing this on master now will allow us to collect testing
 > experience sooner, so I think it's a win.

You mean doing what on master?  Remove 'window--adjust-process-windows'
from 'window-configuration-change-hook'?  But then nobody will test
the way we do that on the release branch now.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Eli Zaretskii
> Date: Sun, 18 Nov 2018 20:37:07 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email], [hidden email]
>
>  >> Because most people run master and I'm interested in testing whether
>  >> the interaction between 'window-configuration-change-hook' and
>  >> 'window-size-change-functions' could cause any problems.
>  >
>  > Indeed, doing this on master now will allow us to collect testing
>  > experience sooner, so I think it's a win.
>
> You mean doing what on master?  Remove 'window--adjust-process-windows'
> from 'window-configuration-change-hook'?

Yes.

> But then nobody will test the way we do that on the release branch
> now.

Those who use Emacs 26 will.



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

martin rudalics
 >> You mean doing what on master?  Remove 'window--adjust-process-windows'
 >> from 'window-configuration-change-hook'?
 >
 > Yes.

OK.  I'll do it in a week or so.  Maybe Gary can test it till then.

martin



Reply | Threaded
Open this post in threaded view
|

bug#32720: term-mode ignores certain window size changes

Gary Fredericks
What should I be testing exactly?

On Mon, Nov 19, 2018, 03:41 martin rudalics <[hidden email] wrote:
 >> You mean doing what on master?  Remove 'window--adjust-process-windows'
 >> from 'window-configuration-change-hook'?
 >
 > Yes.

OK.  I'll do it in a week or so.  Maybe Gary can test it till then.

martin
123