bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

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

Steps to reproduce:

1) Have a frame with such width and height that when there are 4 equal
windows inside it, a `pop-to-window' invocation reuses an existing
window.

2) Start with just 1 window, do C-x 3, then C-x 2 in both resulting
windows.

3) Open a VC-backed file in one of the windows, make a tiny change,
save, do `vc-diff'. Diff will open in one of the existing windows, and
it will be shrunk to the height of the buffer.

4) Quit the diff buffer with `q'. See that the window height stays
shrunk.

In a similar scenario, open a file from a repository with tiny history
(1 commit or so), do `vc-print-log', observe the same behavior.
Thankfully, this is a much more rare occurrence.

Simple patch attached.

Commands messing up existing window configuration is one of my top
Emacs annoyances, and AFAIK it confuses the new users, too.
Maybe nil check for (window-prev-buffers) should be instead included in
`shrink-window-if-larger-than-buffer', with a way to override it?

[-- Attachment #2: 0001-vc.el-vc-diff-finish-vc-log-internal-common-Never-sh.patch --]
[-- Type: text/plain, Size: 1257 bytes --]

From e1f5971c81fabac74280f798802b7958a44db08c Mon Sep 17 00:00:00 2001
From: Dmitry Gutov <dgutov@yandex.ru>
Date: Thu, 28 Jun 2012 23:18:41 +0400
Subject: [PATCH] * vc.el (vc-diff-finish, vc-log-internal-common): Never
 shrink   pre-existing windows

---
 lisp/vc/vc.el |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 87e4e1c..697b212 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1531,7 +1531,7 @@ to override the value of `vc-diff-switches' and `diff-switches'."
 	       (message "%s" (cdr messages))))
 	(diff-setup-whitespace)
 	(goto-char (point-min))
-	(when window
+	(when (and window (not (window-prev-buffers)))
 	  (shrink-window-if-larger-than-buffer window)))
       (when (and messages (not emptyp))
 	(message "%sdone" (car messages))))))
@@ -2147,7 +2147,8 @@ Not all VC backends support short logs!")
     (vc-exec-after
      `(let ((inhibit-read-only t))
 	(funcall ',setup-buttons-func ',backend ',files ',retval)
-	(shrink-window-if-larger-than-buffer)
+	(unless (window-prev-buffers)
+	  (shrink-window-if-larger-than-buffer))
 	(funcall ',goto-location-func ',backend)
 	(setq vc-sentinel-movepoint (point))
 	(set-buffer-modified-p nil)))))
-- 
1.7.10.msysgit.1

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Andreas Schwab]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Commands messing up existing window configuration is one of my top
> Emacs annoyances, and AFAIK it confuses the new users, too.
> Maybe nil check for (window-prev-buffers) should be instead included in
> `shrink-window-if-larger-than-buffer', with a way to override it?

I think there should at least be an option to eliminate it completely.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 29.06.2012 2:32, Andreas Schwab wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> Commands messing up existing window configuration is one of my top
>> Emacs annoyances, and AFAIK it confuses the new users, too.
>> Maybe nil check for (window-prev-buffers) should be instead included in
>> `shrink-window-if-larger-than-buffer', with a way to override it?
>
> I think there should at least be an option to eliminate it completely.

Eliminate the check, or the shrinking behavior?

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Andreas Schwab]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 29.06.2012 2:32, Andreas Schwab wrote:
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>>
>>> Commands messing up existing window configuration is one of my top
>>> Emacs annoyances, and AFAIK it confuses the new users, too.
>>> Maybe nil check for (window-prev-buffers) should be instead included in
>>> `shrink-window-if-larger-than-buffer', with a way to override it?
>>
>> I think there should at least be an option to eliminate it completely.
>
> Eliminate the check, or the shrinking behavior?

The shrinking behavior.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > Commands messing up existing window configuration is one of my top
 > Emacs annoyances, and AFAIK it confuses the new users, too.
 > Maybe nil check for (window-prev-buffers) should be instead included in
 > `shrink-window-if-larger-than-buffer', with a way to override it?

This problem has been previously discussed here

http://lists.gnu.org/archive/html/emacs-devel/2010-08/msg00638.html

If people want an option here, please decide on

- the values it should take, and

- how to meld it with `temp-buffer-resize-mode'.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

Hi Martin,

On 29.06.2012 11:12, martin rudalics wrote:
>  > Commands messing up existing window configuration is one of my top
>  > Emacs annoyances, and AFAIK it confuses the new users, too.
>  > Maybe nil check for (window-prev-buffers) should be instead included in
>  > `shrink-window-if-larger-than-buffer', with a way to override it?
>
> This problem has been previously discussed here
>
> http://lists.gnu.org/archive/html/emacs-devel/2010-08/msg00638.html
>
> If people want an option here, please decide on
>
> - the values it should take, and
>
> - how to meld it with `temp-buffer-resize-mode'.

I think renaming and reusing `even-window-heights' is a good thing to 
do. I'd even suggest changing the default value, because, as you can 
see, virtually nobody among users knows about this variable:

http://stackoverflow.com/questions/4716855/how-can-i-prevent-emacs-resizing-my-windows

(Usually folks at SO give fairly comprehensive answers).
And personally, I'd have been very happy to know about it about 1-2 
years ago, before `pop-to-window' behavior strongly conditioned me 
against manually resizing windows.

I don't think I've ever used `temp-buffer-resize-mode', but if it's a 
minor mode that a user has to enable explicitly, it should be fine if it 
overrides the value of `resize-windows-for-display'.
Please note that `temp-buffer-resize-mode' already does the sane thing: 
it only resizes the window if the window is new and not reused.

To answer you question in the last message:

 >> !       (let ((resize-windows-for-display nil))
 >> !  (pop-to-buffer (current-buffer)))
 >
 > Here you explicitly override the user option - is that intentional?

It's intentional, because `vc-diff-internal' calls `pop-to-buffer' 
before the diff command returns its full output, so the window height 
adjustment happens in `vc-diff-finish' which runs after the process 
returns. So you might want to account for this usage.

Regarding "two similar approaches", I think just having an off by 
default `even-window-heights` variable and `temp-buffer-resize-mode' may 
satisfy more or less everyone, except there'd at least need to be a way 
to make shrinking asynchronous, as per above.

-- Dmitry

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > I think renaming and reusing `even-window-heights' is a good thing to
 > do. I'd even suggest changing the default value, because, as you can
 > see, virtually nobody among users knows about this variable:
 >
 > http://stackoverflow.com/questions/4716855/how-can-i-prevent-emacs-resizing-my-windows
 >
 >
 > (Usually folks at SO give fairly comprehensive answers).
 > And personally, I'd have been very happy to know about it about 1-2
 > years ago, before `pop-to-window' behavior strongly conditioned me
 > against manually resizing windows.

`even-window-heights' is one of these options pertaining to the classic
two windows one-above-the-other frame layout.  It should be redesigned
to handle side-by-side windows and is largely obsolete anyway because
you can now customize `window-combination-resize' instead.

 > I don't think I've ever used `temp-buffer-resize-mode', but if it's a
 > minor mode that a user has to enable explicitly, it should be fine if it
 > overrides the value of `resize-windows-for-display'.
 > Please note that `temp-buffer-resize-mode' already does the sane thing:
 > it only resizes the window if the window is new and not reused.

`temp-buffer-resize-mode' adds `resize-temp-buffer-window' to
`temp-buffer-window-show-hook' so it resizes the window even when it's
reused.  Or do I miss something?

 > To answer you question in the last message:
 >
 >  >> !       (let ((resize-windows-for-display nil))
 >  >> !  (pop-to-buffer (current-buffer)))
 >  >
 >  > Here you explicitly override the user option - is that intentional?
 >
 > It's intentional, because `vc-diff-internal' calls `pop-to-buffer'
 > before the diff command returns its full output, so the window height
 > adjustment happens in `vc-diff-finish' which runs after the process
 > returns. So you might want to account for this usage.

I don't recall the details but we always had a chicken-and-egg problem
here: When you fill a buffer before displaying it you can't make its
line breaks suit the window used for it.  When you display it first, you
can't make the window suit the line breaks of the buffer.  And the
number of line breaks determines the height of the region to display.

 > Regarding "two similar approaches", I think just having an off by
 > default `even-window-heights` variable and `temp-buffer-resize-mode' may
 > satisfy more or less everyone, except there'd at least need to be a way
 > to make shrinking asynchronous, as per above.

Since I don't use `vc-diff' I can't really comment on how it should
behave (I practically always use side-by-side windows for watching
diffs).  Is anyone interested in the resizing behavior at all?  Couldn't
we use the `quit-restore' window parameter to restore the original size
of the window?

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Juanma Barranquero]

On Sat, Jun 30, 2012 at 11:09 AM, martin rudalics <rudalics@gmx.at> wrote:

> Is anyone interested in the resizing behavior at all?

IIUC what's being discussed, yes, definitely.

    Juanma

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 30.06.2012 13:09, martin rudalics wrote:
> `even-window-heights' is one of these options pertaining to the classic
> two windows one-above-the-other frame layout.  It should be redesigned
> to handle side-by-side windows and is largely obsolete anyway because
> you can now customize `window-combination-resize' instead.

How is it obsolete? As far as I can see, neither of the two possible 
values of `window-combination-resize' says "don't resize my existing 
windows until specifically asked".

>  > I don't think I've ever used `temp-buffer-resize-mode', but if it's a
>  > minor mode that a user has to enable explicitly, it should be fine if it
>  > overrides the value of `resize-windows-for-display'.
>  > Please note that `temp-buffer-resize-mode' already does the sane thing:
>  > it only resizes the window if the window is new and not reused.
>
> `temp-buffer-resize-mode' adds `resize-temp-buffer-window' to
> `temp-buffer-window-show-hook' so it resizes the window even when it's
> reused.  Or do I miss something?

Actually yes, I haven't tested it properly the last time.
It can shrink the window to make it fit the buffer, but it restores the 
window's size when it's done, which is the important thing.

>  > To answer you question in the last message:
>  >
>  >  >> !       (let ((resize-windows-for-display nil))
>  >  >> !  (pop-to-buffer (current-buffer)))
>  >  >
>  >  > Here you explicitly override the user option - is that intentional?
>  >
>  > It's intentional, because `vc-diff-internal' calls `pop-to-buffer'
>  > before the diff command returns its full output, so the window height
>  > adjustment happens in `vc-diff-finish' which runs after the process
>  > returns. So you might want to account for this usage.
>
> I don't recall the details but we always had a chicken-and-egg problem
> here: When you fill a buffer before displaying it you can't make its
> line breaks suit the window used for it.  When you display it first, you
> can't make the window suit the line breaks of the buffer.  And the
> number of line breaks determines the height of the region to display.

Do you mean auto-filling, as in, "inserting hard newlines"? I don't 
think diff-mode does that. It does pop a window immediately, but 
probably just because it would be weird to see a window pop up only 
after the diff command finishes (if that happens after some time has 
passed).

>  > Regarding "two similar approaches", I think just having an off by
>  > default `even-window-heights` variable and `temp-buffer-resize-mode' may
>  > satisfy more or less everyone, except there'd at least need to be a way
>  > to make shrinking asynchronous, as per above.
>
> Since I don't use `vc-diff' I can't really comment on how it should
> behave (I practically always use side-by-side windows for watching
> diffs).  Is anyone interested in the resizing behavior at all?  Couldn't
> we use the `quit-restore' window parameter to restore the original size
> of the window?

We probably could, if solving only this particular case is okay. As far 
as VC package goes, we'd need to do that in `vc-diff-internal' and in 
all (?) callers of `vc-log-internal-common'.

-- Dmitry

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > How is it obsolete? As far as I can see, neither of the two possible
 > values of `window-combination-resize' says "don't resize my existing
 > windows until specifically asked".

You would customize `window-combination-resize' only with
`even-window-heights' implicitly nil.

 >> `temp-buffer-resize-mode' adds `resize-temp-buffer-window' to
 >> `temp-buffer-window-show-hook' so it resizes the window even when it's
 >> reused.  Or do I miss something?
 >
 > Actually yes, I haven't tested it properly the last time.
 > It can shrink the window to make it fit the buffer, but it restores the
 > window's size when it's done, which is the important thing.

It explicitly checks whether `temp-buffer-resize-mode' is on when the
buffer shall be removed.

 >> I don't recall the details but we always had a chicken-and-egg problem
 >> here: When you fill a buffer before displaying it you can't make its
 >> line breaks suit the window used for it.  When you display it first, you
 >> can't make the window suit the line breaks of the buffer.  And the
 >> number of line breaks determines the height of the region to display.
 >
 > Do you mean auto-filling, as in, "inserting hard newlines"? I don't
 > think diff-mode does that. It does pop a window immediately, but
 > probably just because it would be weird to see a window pop up only
 > after the diff command finishes (if that happens after some time has
 > passed).

Must have been a different problem indeed, probably in the context of
man or woman, IIRC.

 >> Couldn't
 >> we use the `quit-restore' window parameter to restore the original size
 >> of the window?
 >
 > We probably could, if solving only this particular case is okay. As far
 > as VC package goes, we'd need to do that in `vc-diff-internal' and in
 > all (?) callers of `vc-log-internal-common'.

We could simply remove the `temp-buffer-resize-mode' check in
`quit-window'.  I hardly understand what implications this might have.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 >> Is anyone interested in the resizing behavior at all?
 >
 > IIUC what's being discussed, yes, definitely.

I was unclear.  I meant resizing the window when it gets _reused_.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 02.07.2012 11:00, martin rudalics wrote:
>  >  From what I understand in `window-combination-resize' docstring, its
>  > value decides whether splitting a window borrows space only from
>  > adjacent window, or from all windows in a "combination" (so they all get
>  > resized). For that to happen, a window splitting should occur, right?
>  > But when we're displaying a buffer in pre-existing window, no splitting
>  > occurs, just resizing.
>  > And `even-window-heights' controls whether it should be resized at all.
>
> IIUC, the case we're talking about is that of a split frame where one
> window gets reused.  Now, with `window-combination-resize' non-nil,
> `display-buffer' practically always makes a new window in such a case
> because it can steal space from both existing windows.  Quitting or
> deleting the new window will restore the sizes of the original windows
> regardless of whether `temp-buffer-resize-mode' has been used or not.

I see. Still, I'm primarily interested in the scenario where the windows 
don't get split (with appropriate split-*-threshold values), and in this 
case `even-window-heights' value has effect.

Note that if the widow does get split, `vc-diff' behaves okay, because 
`q' deletes the used window, and the configuration becomes as it was (at 
least with `window-combination-resize' nil).

>  >> We could simply remove the `temp-buffer-resize-mode' check in
>  >> `quit-window'.  I hardly understand what implications this might have.
>  >
>  > I think it's just trying to making sure that (nth 3 quad) is of the
>  > right type, so that the comparison doesn't blow up.
>
> No (at least not as far as I originally conceived it).  My idea was as
> follows:
>
> (1) When `temp-buffer-resize-mode' is non-nil and the window size has
>      changed, chances are that the change was caused by
>      `temp-buffer-resize-mode' so it seems pretty safe to rescind that
>      change.

I don't think so. Like I said, the value saved in 'quit-restore 
parameter is the same in this case, whether `temp-buffer-resize-mode' is 
on or not.
So even if `temp-buffer-resize-mode' is on, we can't confidently decide 
that the value was set by it.

> (2) When `temp-buffer-resize-mode' is nil and the window size has
>      changed, the change was probably due to some manual intervention
>      probably needed to see more of a buffer originally present and it
>      seems better to leave that change alone.
>
> But maybe my conception was flawed.

Would it be harder to *not set* the 'quit-restore parameter in cases 
when we don't want the configuration restored, instead?

But as it is, I'm not sure what's the problem with always using its 
value when present. I've been running Emacs with the tiny patch I posted 
for a couple of days, and it seems fine.

Could you describe the scenario with "seeing more of a buffer originally 
present"?

>  > If I enable temp-buffer-resize-mode before doing vc-diff, after I quit
>  > the window, its size does get restored.
>
> It should be sufficient to set the variable `temp-buffer-resize-mode'
> locally in that buffer.

Yes, but that's not what I meant.

>  > Also, (nth 3 quad) is integer at that point even temp-buffer-resize-mode
>  > is disabled, so the mode check doesn't make sense.
>  > To be safe, though, we could replace it with `integerp' call.
>
> Yes, but the `condition-case' should handle any problems with it.

Don't we want to execute the code following (when resize ...) either way?

> In any case, we have to cater for the case where people (I'm not sure
> whether I got Andreas right) want to disable adjusting the window in the
> first place.  So I really think we should simply special-case this by
> providing some option like `vc-fit-diff-window-to-buffer' and adjust the
> window only if that variable has a non-nil value.  If it's non-nil, we
> can either

I'm fine with either behavior (not resizing or properly restoring), but 
`vc-diff' is not the only culprit. `vc-log-print' also does the 
shrinking, although it's harder to observe.
If there will be a variable, I don't see why it should be local to VC. 
Are there users who would want windows shrunk by VC, but not other 
packages, or vice versa?

> (1) set `temp-buffer-resize-mode' locally in such buffers, or
>
> (2) provide yet another variable which, when set, causes `quit-window'
>      to re-resize the window (we could misuse `even-window-heights' for
>      this purpose), or

That won't do if the original windows sizes were not even, like in the 
SO question linked to previously.

> (3) re-resize the window as in your patch.

-- Dmitry

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > Note that if the widow does get split, `vc-diff' behaves okay, because
 > `q' deletes the used window, and the configuration becomes as it was (at
 > least with `window-combination-resize' nil).

Not necessarily: When you have two windows one above the other and
`display-buffer' splits the upper one (thresholds permitting) and then
`vc-diff' temporarily resizes the middle one, it steals space from the
lower window.  When quitting the middle window, that space is returned
to the upper one.

 >> (1) When `temp-buffer-resize-mode' is non-nil and the window size has
 >>      changed, chances are that the change was caused by
 >>      `temp-buffer-resize-mode' so it seems pretty safe to rescind that
 >>      change.
 >
 > I don't think so. Like I said, the value saved in 'quit-restore
 > parameter is the same in this case, whether `temp-buffer-resize-mode' is
 > on or not.
 > So even if `temp-buffer-resize-mode' is on, we can't confidently decide
 > that the value was set by it.

Sure, but ...

 >> (2) When `temp-buffer-resize-mode' is nil and the window size has
 >>      changed, the change was probably due to some manual intervention
 >>      probably needed to see more of a buffer originally present and it
 >>      seems better to leave that change alone.

... in this case we can be sure that the user changed the size so we
probably should leave it alone.

 > Would it be harder to *not set* the 'quit-restore parameter in cases
 > when we don't want the configuration restored, instead?

The whole idea of quitting is to get as much as possible of the state
that existed before some temporal change without, however, rescinding
changes introduced by user.

 > But as it is, I'm not sure what's the problem with always using its
 > value when present. I've been running Emacs with the tiny patch I posted
 > for a couple of days, and it seems fine.
 >
 > Could you describe the scenario with "seeing more of a buffer originally
 > present"?

When watching diffs I usually very soon want to concentrate on the
modified file and its changes.  For that reason, I sometimes want to
enlarge its window and not have `quit-window' shrink it to what it was
before invoking vc-diff.  But this use case might be sufficiently exotic
to dismiss it.

 >>  > If I enable temp-buffer-resize-mode before doing vc-diff, after I quit
 >>  > the window, its size does get restored.
 >>
 >> It should be sufficient to set the variable `temp-buffer-resize-mode'
 >> locally in that buffer.
 >
 > Yes, but that's not what I meant.

It would be a hack anyway.

 >>  > Also, (nth 3 quad) is integer at that point even
 >> temp-buffer-resize-mode
 >>  > is disabled, so the mode check doesn't make sense.
 >>  > To be safe, though, we could replace it with `integerp' call.
 >>
 >> Yes, but the `condition-case' should handle any problems with it.
 >
 > Don't we want to execute the code following (when resize ...) either way?

Don't we?  How would the (when resize ...) check affect the rest?  And
keep in mind that any resize operation has to take into account that the
frame configuration or size has changed in the meantime.

 > I'm fine with either behavior (not resizing or properly restoring), but
 > `vc-diff' is not the only culprit. `vc-log-print' also does the
 > shrinking, although it's harder to observe.

What and where is `vc-log-print'?

 > If there will be a variable, I don't see why it should be local to VC.
 > Are there users who would want windows shrunk by VC, but not other
 > packages, or vice versa?

I don't know.  I think a vc-diff buffer should be considerded a
temporary buffer, subject to `temp-buffer-resize-mode'.  Ideally,
windows of non-temporary buffers are never shrunk automatically.

 >> (2) provide yet another variable which, when set, causes `quit-window'
 >>      to re-resize the window (we could misuse `even-window-heights' for
 >>      this purpose), or
 >
 > That won't do if the original windows sizes were not even, like in the
 > SO question linked to previously.

I miss you here.  The information is there (otherwise your patch
couldn't use it) and I'd just use it in the additional case where
`even-window-heights' is set.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 02.07.2012 20:32, martin rudalics wrote:
>  > Note that if the widow does get split, `vc-diff' behaves okay, because
>  > `q' deletes the used window, and the configuration becomes as it was (at
>  > least with `window-combination-resize' nil).
>
> Not necessarily: When you have two windows one above the other and
> `display-buffer' splits the upper one (thresholds permitting) and then
> `vc-diff' temporarily resizes the middle one, it steals space from the
> lower window.  When quitting the middle window, that space is returned
> to the upper one.

I guess that's true, my height threshold is just too high for that.
A bit inconsistent, though, isn't it? Stealing space from the lower, 
returning to the upper.

>  >> (1) When `temp-buffer-resize-mode' is non-nil and the window size has
>  >>      changed, chances are that the change was caused by
>  >>      `temp-buffer-resize-mode' so it seems pretty safe to rescind that
>  >>      change.
>  >
>  > I don't think so. Like I said, the value saved in 'quit-restore
>  > parameter is the same in this case, whether `temp-buffer-resize-mode' is
>  > on or not.
>  > So even if `temp-buffer-resize-mode' is on, we can't confidently decide
>  > that the value was set by it.
>
> Sure, but ...
>
>  >> (2) When `temp-buffer-resize-mode' is nil and the window size has
>  >>      changed, the change was probably due to some manual intervention
>  >>      probably needed to see more of a buffer originally present and it
>  >>      seems better to leave that change alone.
>
> ... in this case we can be sure that the user changed the size so we
> probably should leave it alone.

That depends on the definition of "the user". In our case, *I* didn't 
explicitly resize the window, `vc-diff' did.

>  > Would it be harder to *not set* the 'quit-restore parameter in cases
>  > when we don't want the configuration restored, instead?
>
> The whole idea of quitting is to get as much as possible of the state
> that existed before some temporal change without, however, rescinding
> changes introduced by user.

How about clearing (or changing) the 'quit-window parameter in each 
command when we're sure that the user won't want to have the size 
restored afterwards?
There would be a set of "user-facing" functions that would do that.

>  > But as it is, I'm not sure what's the problem with always using its
>  > value when present. I've been running Emacs with the tiny patch I posted
>  > for a couple of days, and it seems fine.
>  >
>  > Could you describe the scenario with "seeing more of a buffer originally
>  > present"?
>
> When watching diffs I usually very soon want to concentrate on the
> modified file and its changes.  For that reason, I sometimes want to
> enlarge its window and not have `quit-window' shrink it to what it was
> before invoking vc-diff.  But this use case might be sufficiently exotic
> to dismiss it.

How will the temp-buffer-resize-mode null check help in this case?
It's a global minor mode, so it's either t in all buffers, or nil in all 
of them.

>  >>  > Also, (nth 3 quad) is integer at that point even
>  >> temp-buffer-resize-mode
>  >>  > is disabled, so the mode check doesn't make sense.
>  >>  > To be safe, though, we could replace it with `integerp' call.
>  >>
>  >> Yes, but the `condition-case' should handle any problems with it.
>  >
>  > Don't we want to execute the code following (when resize ...) either
> way?
>
> Don't we?  How would the (when resize ...) check affect the rest?  And
> keep in mind that any resize operation has to take into account that the
> frame configuration or size has changed in the meantime.

Eh, I meant the comparison will blow up, it's above the condition-case:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
   /=(nil 42)
   eval((/= nil 42) nil)

>  > I'm fine with either behavior (not resizing or properly restoring), but
>  > `vc-diff' is not the only culprit. `vc-log-print' also does the
>  > shrinking, although it's harder to observe.
>
> What and where is `vc-log-print'?

Sorry, `vc-print-log'. C-x v l.

>  > If there will be a variable, I don't see why it should be local to VC.
>  > Are there users who would want windows shrunk by VC, but not other
>  > packages, or vice versa?
>
> I don't know.  I think a vc-diff buffer should be considerded a
> temporary buffer, subject to `temp-buffer-resize-mode'.  Ideally,
> windows of non-temporary buffers are never shrunk automatically.

Would a window that displayed a normal buffer previously but which now 
is displaying a temporary buffer be considered a "window of 
non-temporary buffer"?

>  >> (2) provide yet another variable which, when set, causes `quit-window'
>  >>      to re-resize the window (we could misuse `even-window-heights' for
>  >>      this purpose), or
>  >
>  > That won't do if the original windows sizes were not even, like in the
>  > SO question linked to previously.
>
> I miss you here.  The information is there (otherwise your patch
> couldn't use it) and I'd just use it in the additional case where
> `even-window-heights' is set.

I misunderstood. Re-resizing to the original height would work, of course.

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > A bit inconsistent, though, isn't it? Stealing space from the lower,
 > returning to the upper.

When you resize the selected window you try to steal from/give to the
lower window in order to not clobber the window-start position of the
selected window.  When you delete a window you return space to the upper
one because that's where the space usually came from.  That's just my
interpretation of the behavior, though.

 >>  >> (2) When `temp-buffer-resize-mode' is nil and the window size has
 >>  >>      changed, the change was probably due to some manual intervention
 >>  >>      probably needed to see more of a buffer originally present
 >> and it
 >>  >>      seems better to leave that change alone.
 >>
 >> ... in this case we can be sure that the user changed the size so we
 >> probably should leave it alone.
 >
 > That depends on the definition of "the user". In our case, *I* didn't
 > explicitly resize the window, `vc-diff' did.

Because of `even-window-heights'?  That's something I obviously forgot
to handle when quitting a window.  Maybe the best solution is to set the
3rd element of the quit-restore parameter iff either

(1) `temp-buffer-resize-mode' is non-nil, or

(2) `window--even-window-heights' actually did resize the window

and don't do the `temp-buffer-resize-mode' check in `quit-window'.

 > How about clearing (or changing) the 'quit-window parameter in each
 > command when we're sure that the user won't want to have the size
 > restored afterwards?
 > There would be a set of "user-facing" functions that would do that.

`adjust-window-trailing-edge' would be an obvious candidate.  But which
window's parameter would you clear?  Both?

 > How will the temp-buffer-resize-mode null check help in this case?
 > It's a global minor mode, so it's either t in all buffers, or nil in all
 > of them.

That's why I would have set `temp-buffer-resize-mode' buffer-locally to
t.  But it's ugly.

 >> Don't we?  How would the (when resize ...) check affect the rest?  And
 >> keep in mind that any resize operation has to take into account that the
 >> frame configuration or size has changed in the meantime.
 >
 > Eh, I meant the comparison will blow up, it's above the condition-case:
 >
 > Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
 >   /=(nil 42)
 >   eval((/= nil 42) nil)

I see.  This would have to be fixed anyway if the third element of the
quit-restore parameter is not set in `display-buffer-record-window'.

 >>  > I'm fine with either behavior (not resizing or properly restoring),
 >> but
 >>  > `vc-diff' is not the only culprit. `vc-log-print' also does the
 >>  > shrinking, although it's harder to observe.
 >>
 >> What and where is `vc-log-print'?
 >
 > Sorry, `vc-print-log'. C-x v l.

It's in `vc-log-internal-common', I see.

 >> I don't know.  I think a vc-diff buffer should be considerded a
 >> temporary buffer, subject to `temp-buffer-resize-mode'.  Ideally,
 >> windows of non-temporary buffers are never shrunk automatically.
 >
 > Would a window that displayed a normal buffer previously but which now
 > is displaying a temporary buffer be considered a "window of
 > non-temporary buffer"?

Only after it displays the normal buffer again.  Why did you ask?

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 03.07.2012 11:14, martin rudalics wrote:
>  >>  >> (2) When `temp-buffer-resize-mode' is nil and the window size has
>  >>  >>      changed, the change was probably due to some manual
> intervention
>  >>  >>      probably needed to see more of a buffer originally present
>  >> and it
>  >>  >>      seems better to leave that change alone.
>  >>
>  >> ... in this case we can be sure that the user changed the size so we
>  >> probably should leave it alone.
>  >
>  > That depends on the definition of "the user". In our case, *I* didn't
>  > explicitly resize the window, `vc-diff' did.
>
> Because of `even-window-heights'?  That's something I obviously forgot

Because of `shrink-window-if-larger-than-buffer'.
`even-window-heights' could have contributed in another case, if my 
windows were not of equal height already.

> to handle when quitting a window.  Maybe the best solution is to set the
> 3rd element of the quit-restore parameter iff either
>
> (1) `temp-buffer-resize-mode' is non-nil, or

Maybe just when `fit-window-to-buffer' is called? That would account for 
`shrink-window-if-larger-than-buffer', too.

> (2) `window--even-window-heights' actually did resize the window
 >
> and don't do the `temp-buffer-resize-mode' check in `quit-window'.
>
>  > How about clearing (or changing) the 'quit-window parameter in each
>  > command when we're sure that the user won't want to have the size
>  > restored afterwards?
>  > There would be a set of "user-facing" functions that would do that.
>
> `adjust-window-trailing-edge' would be an obvious candidate.  But which
> window's parameter would you clear?  Both?

What's the second one? Please keep in mind that I don't know the 
window.el codebase, I'm just reading the code along the discussion.

>  > How will the temp-buffer-resize-mode null check help in this case?
>  > It's a global minor mode, so it's either t in all buffers, or nil in all
>  > of them.
>
> That's why I would have set `temp-buffer-resize-mode' buffer-locally to
> t.  But it's ugly.

If `temp-buffer-resize-mode' were on, that wouldn't have made a 
difference, would it?
You'd have to locally set `temp-buffer-resize-mode' to nil in all cases 
when you don't want the window size to be restored afterwards, which is 
the same amount of work as clearing 'quit-window parameter.

>  >> I don't know.  I think a vc-diff buffer should be considerded a
>  >> temporary buffer, subject to `temp-buffer-resize-mode'.  Ideally,
>  >> windows of non-temporary buffers are never shrunk automatically.
>  >
>  > Would a window that displayed a normal buffer previously but which now
>  > is displaying a temporary buffer be considered a "window of
>  > non-temporary buffer"?
>
> Only after it displays the normal buffer again.  Why did you ask?

Because that's what at issue here. I think the main two cases of 
automatic shrinking are:
1) "temp buffer" in a window that was created for it,
2) "temp buffer" in a reused window,
and 2) is the reason I filed this bug.

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 >>  > That depends on the definition of "the user". In our case, *I* didn't
 >>  > explicitly resize the window, `vc-diff' did.
 >>
 >> Because of `even-window-heights'?  That's something I obviously forgot
 >
 > Because of `shrink-window-if-larger-than-buffer'.

Sure.  But as I proposed earlier we could have handled this by setting
`temp-buffer-resize-mode' to t in the diff-buffer.

 > `even-window-heights' could have contributed in another case, if my
 > windows were not of equal height already.

`even-window-heights' is customizable so it's not a primary issue.  But
we could undo the evening in `quit-window'.

 >> to handle when quitting a window.  Maybe the best solution is to set the
 >> 3rd element of the quit-restore parameter iff either
 >>
 >> (1) `temp-buffer-resize-mode' is non-nil, or
 >
 > Maybe just when `fit-window-to-buffer' is called? That would account for
 > `shrink-window-if-larger-than-buffer', too.

Unless it's an interactive call of
`shrink-window-if-larger-than-buffer' probably.

 >> `adjust-window-trailing-edge' would be an obvious candidate.  But which
 >> window's parameter would you clear?  Both?
 >
 > What's the second one? Please keep in mind that I don't know the
 > window.el codebase, I'm just reading the code along the discussion.

`adjust-window-trailing-edge' drags an edge between two windows and
usually resizes the two windows adjacent to the edge, for example, when
mouse-dragging the mode-line.  Hence we have two candidates for clearing
a quit-restore parameter's size element.

 > If `temp-buffer-resize-mode' were on, that wouldn't have made a
 > difference, would it?
 > You'd have to locally set `temp-buffer-resize-mode' to nil in all cases
 > when you don't want the window size to be restored afterwards, which is
 > the same amount of work as clearing 'quit-window parameter.

Why?  I set the buffer-local value only when `temp-buffer-resize-mode'
is off.  When it's on I don't assign a buffer-local value.  The
re-resize code in `quit-window' would trigger when either the local or
the global value is t.

Anyway, this would only handle the re-resizing when quitting the vc-diff
buffer.  It would not handle the case where people don't want any
resizing.  Maybe vc-diff should shrink iff `temp-buffer-resize-mode' is
on.

 >>  > Would a window that displayed a normal buffer previously but which now
 >>  > is displaying a temporary buffer be considered a "window of
 >>  > non-temporary buffer"?
 >>
 >> Only after it displays the normal buffer again.  Why did you ask?
 >
 > Because that's what at issue here. I think the main two cases of
 > automatic shrinking are:
 > 1) "temp buffer" in a window that was created for it,
 > 2) "temp buffer" in a reused window,
 > and 2) is the reason I filed this bug.

Remember that the vc-diff buffer is not a temporary buffer.  Currently,
in order to trigger automatic resizing of temporary buffer windows you
have to use `with-output-to-temp-buffer'.  If vc-diff had used that
macro, the entire resizing issue would have been handled already.
Another way is to explicitly call `resize-temp-buffer-window' and set
`temp-buffer-resize-mode' t in that buffer.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 03.07.2012 20:40, martin rudalics wrote:
 > > Because of `shrink-window-if-larger-than-buffer'.
 >
 > Sure.  But as I proposed earlier we could have handled this by setting
 > `temp-buffer-resize-mode' to t in the diff-buffer.

This will re-introduce the issue, the one you said 
`temp-buffer-resize-mode' check was guarding from.
Namely, if I run `vc-diff', it reuses some window that has a neighbor 
vertically, I drag its window border to resize, then go back to the 
window and press 'q', it will restore the original height, like you said 
it shouldn't.

>  > `even-window-heights' could have contributed in another case, if my
>  > windows were not of equal height already.
>
> `even-window-heights' is customizable so it's not a primary issue.  But
> we could undo the evening in `quit-window'.

I agree.

>  >> to handle when quitting a window.  Maybe the best solution is to set
> the
>  >> 3rd element of the quit-restore parameter iff either
>  >>
>  >> (1) `temp-buffer-resize-mode' is non-nil, or
>  >
>  > Maybe just when `fit-window-to-buffer' is called? That would account for
>  > `shrink-window-if-larger-than-buffer', too.
>
> Unless it's an interactive call of
> `shrink-window-if-larger-than-buffer' probably.

Well, yes. I think that's the hard part - deciding on the set of 
functions and if we need to do the check with `called-interactively-p' 
in some of them.

>  >> `adjust-window-trailing-edge' would be an obvious candidate.  But which
>  >> window's parameter would you clear?  Both?
>  >
>  > What's the second one? Please keep in mind that I don't know the
>  > window.el codebase, I'm just reading the code along the discussion.
>
> `adjust-window-trailing-edge' drags an edge between two windows and
> usually resizes the two windows adjacent to the edge, for example, when
> mouse-dragging the mode-line.  Hence we have two candidates for clearing
> a quit-restore parameter's size element.

Then yes, both of them, I guess.
`enlarge-window' and `enlarge-window-horizontally' could be another 
candidates. Not sure about `delete-window' (when we're deleting one 
window in a configuration), could be either way.

>  > If `temp-buffer-resize-mode' were on, that wouldn't have made a
>  > difference, would it?
>  > You'd have to locally set `temp-buffer-resize-mode' to nil in all cases
>  > when you don't want the window size to be restored afterwards, which is
>  > the same amount of work as clearing 'quit-window parameter.
>
> Why?  I set the buffer-local value only when `temp-buffer-resize-mode'
> is off.  When it's on I don't assign a buffer-local value.  The
> re-resize code in `quit-window' would trigger when either the local or
> the global value is t.

There are cases when we don't want it to be triggered, right? (See 
example at the top of this email). And when `temp-buffer-resize-mode' is 
t locally or globally, re-resizing code will always be triggered.

Hence, the `temp-buffer-resize-mode' check in `quit-window' does not 
really serve the purpose you ascribe to it. Or doesn't serve it well enough.

So I think the first thing to do is replace that check with (integerp 
...), like I suggested, and consider the question of when not to restore 
window height a separate issue (maybe file a separate bug, maybe not), 
because the issue is already there when `temp-buffer-resize-mode' is on.

> Anyway, this would only handle the re-resizing when quitting the vc-diff
> buffer.  It would not handle the case where people don't want any
> resizing.  Maybe vc-diff should shrink iff `temp-buffer-resize-mode' is
> on.

Maybe so. I think this is also a separate issue, but it's closely 
related to identifying the set of functions after which we don't restore 
window sizes, because just as we might want not to restore the height 
after `adjust-window-trailing-edge' was called, or 
`shrink-buffer-if-...' was called interactively, we similarly might want 
to resize the windows *only* when one of those functions is called (and 
only when interactively, in `shrink-buffer-if...' case).

>  >>  > Would a window that displayed a normal buffer previously but
> which now
>  >>  > is displaying a temporary buffer be considered a "window of
>  >>  > non-temporary buffer"?
>  >>
>  >> Only after it displays the normal buffer again.  Why did you ask?
>  >
>  > Because that's what at issue here. I think the main two cases of
>  > automatic shrinking are:
>  > 1) "temp buffer" in a window that was created for it,
>  > 2) "temp buffer" in a reused window,
>  > and 2) is the reason I filed this bug.
>
> Remember that the vc-diff buffer is not a temporary buffer.  Currently,

Yes, hence the quotes around "temp buffer".

> in order to trigger automatic resizing of temporary buffer windows you
> have to use `with-output-to-temp-buffer'.  If vc-diff had used that
> macro, the entire resizing issue would have been handled already.

It wouldn't, because `temp-buffer-resize-mode' is off by default. Or 
even if it were on by default, just because it could be switched on and 
off, the logic there can't depend on it.

> Another way is to explicitly call `resize-temp-buffer-window' and set
> `temp-buffer-resize-mode' t in that buffer.

I think that's the only way that restoring `vc-diff' window height could 
have worked with current `quit-window' code.
But like I said above (see the top of this email), it fixes one problem 
by introducing another.

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 >  > Sure.  But as I proposed earlier we could have handled this by setting
 >  > `temp-buffer-resize-mode' to t in the diff-buffer.
 >
 > This will re-introduce the issue, the one you said
 > `temp-buffer-resize-mode' check was guarding from.
 > Namely, if I run `vc-diff', it reuses some window that has a neighbor
 > vertically, I drag its window border to resize, then go back to the
 > window and press 'q', it will restore the original height,

... but only for vc-diff buffers ...

 > like you said
 > it shouldn't.

... do that for arbitrary buffers.  If we remove the corresponding check
in `quit-window', all windows that can be quit are affected.

 >> `even-window-heights' is customizable so it's not a primary issue.  But
 >> we could undo the evening in `quit-window'.
 >
 > I agree.

We'll do so when we apply your last patch ;-)

 >> Unless it's an interactive call of
 >> `shrink-window-if-larger-than-buffer' probably.
 >
 > Well, yes. I think that's the hard part - deciding on the set of
 > functions and if we need to do the check with `called-interactively-p'
 > in some of them.

It's tedious and I wouldn't like to do it.

 > Then yes, both of them, I guess.
 > `enlarge-window' and `enlarge-window-horizontally' could be another
 > candidates.

Yes.

 > Not sure about `delete-window' (when we're deleting one
 > window in a configuration), could be either way.

No (we'd have to care about its clients like `quit-window' too then).

 > There are cases when we don't want it to be triggered, right? (See
 > example at the top of this email). And when `temp-buffer-resize-mode' is
 > t locally or globally, re-resizing code will always be triggered.

My implicit assumption was that people using `temp-buffer-resize-mode'
want automatic changes like that in vc-diff rescinded when done with the
buffer.

 > Hence, the `temp-buffer-resize-mode' check in `quit-window' does not
 > really serve the purpose you ascribe to it. Or doesn't serve it well
 > enough.

Because vc-diff does something special here.

 > So I think the first thing to do is replace that check with (integerp
 > ...), like I suggested, and consider the question of when not to restore
 > window height a separate issue (maybe file a separate bug, maybe not),
 > because the issue is already there when `temp-buffer-resize-mode' is
 > on.

OK, let's do that.  People had enough time to express their concerns
about it.  If problems come up, we'll hear about them soon enough.

 >> Anyway, this would only handle the re-resizing when quitting the vc-diff
 >> buffer.  It would not handle the case where people don't want any
 >> resizing.  Maybe vc-diff should shrink iff `temp-buffer-resize-mode' is
 >> on.
 >
 > Maybe so. I think this is also a separate issue, but it's closely
 > related to identifying the set of functions after which we don't restore
 > window sizes, because just as we might want not to restore the height
 > after `adjust-window-trailing-edge' was called, or
 > `shrink-buffer-if-...' was called interactively, we similarly might want
 > to resize the windows *only* when one of those functions is called (and
 > only when interactively, in `shrink-buffer-if...' case).

`vc-diff' unconditionally resizes the window.  What if someone simply
doesn't want its `shrink-window-if-larger-than-buffer'?

 >> in order to trigger automatic resizing of temporary buffer windows you
 >> have to use `with-output-to-temp-buffer'.  If vc-diff had used that
 >> macro, the entire resizing issue would have been handled already.
 >
 > It wouldn't, because `temp-buffer-resize-mode' is off by default.

That's what I meant: In that case there would not have been any resizing
unless triggered by `even-window-heights'.

 > Or
 > even if it were on by default, just because it could be switched on and
 > off, the logic there can't depend on it.

We could make the value at the time the window was reused prevail via
the quit-restore parameter: That is, save the total size of the window
only if either `even-window-heights' or `temp-buffer-resize-mode' are
on.

 >> Another way is to explicitly call `resize-temp-buffer-window' and set
 >> `temp-buffer-resize-mode' t in that buffer.
 >
 > I think that's the only way that restoring `vc-diff' window height could
 > have worked with current `quit-window' code.
 > But like I said above (see the top of this email), it fixes one problem
 > by introducing another.

... in a limited way, though.

Let's close this thread as follows: Remove the `temp-buffer-resize-mode'
check in `quit-window' and add an integerp check for the third element.
If this has any bad consequences, we can still

(1) have `vc-diff' use something similar to `with-output-to-temp-buffer'
     so that `temp-buffer-resize-mode' is honored,

(2) resize the window only if `temp-buffer-resize-mode' is enabled, or

(3) resize the window unconditionally as now but locally set the
     variable `temp-buffer-resize-mode' to t in the diff buffer.

All these approaches would re-resize the window on quitting provided
`even-window-heights' is nil.

Independently from that we can provide an extra vc-prefixed variable
which controls whether the diff window shall be resiized in the first
place.  As you said, that would be a different bug.

More generally, we could provide an option specifying a function or
regexp for all buffers whose windows should be fit to them and have
`fit-window-to-buffer' (unless called interactively) honor that.  That
option would then override `temp-buffer-resize-mode' as well.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 04.07.2012 13:18, martin rudalics wrote:
>  >  > Sure.  But as I proposed earlier we could have handled this by
> setting
>  >  > `temp-buffer-resize-mode' to t in the diff-buffer.
>  >
>  > This will re-introduce the issue, the one you said
>  > `temp-buffer-resize-mode' check was guarding from.
>  > Namely, if I run `vc-diff', it reuses some window that has a neighbor
>  > vertically, I drag its window border to resize, then go back to the
>  > window and press 'q', it will restore the original height,
>
> ... but only for vc-diff buffers ...
>
>  > like you said
>  > it shouldn't.
>
> ... do that for arbitrary buffers.  If we remove the corresponding check
> in `quit-window', all windows that can be quit are affected.

I think it's good if all windows of this kind behave similarly.

>  >> `even-window-heights' is customizable so it's not a primary issue.  But
>  >> we could undo the evening in `quit-window'.
>  >
>  > I agree.
>
> We'll do so when we apply your last patch ;-)

I tried it with just my patch applied, and it didn't work, because in 
this case the stored height value was of already resized window:
`display-buffer-record-window' is called from `window--display-buffer', 
and `display-buffer-use-some-window' calls `window--even-window-heights' 
before `window--display-buffer'.

Maybe that's fine, I'll just set the variable to nil.

>  >> Unless it's an interactive call of
>  >> `shrink-window-if-larger-than-buffer' probably.
>  >
>  > Well, yes. I think that's the hard part - deciding on the set of
>  > functions and if we need to do the check with `called-interactively-p'
>  > in some of them.
>
> It's tedious and I wouldn't like to do it.

No problem, let's wait and see if/when someone complains.

Until then, the issue is somewhat alleviated by the fact that you can 
press 'z' or 'C-x k' instead of 'q', and both of these won't trigger 
height restoration.

>  >> Anyway, this would only handle the re-resizing when quitting the
> vc-diff
>  >> buffer.  It would not handle the case where people don't want any
>  >> resizing.  Maybe vc-diff should shrink iff `temp-buffer-resize-mode' is
>  >> on.
>  >
>  > Maybe so. I think this is also a separate issue, but it's closely
>  > related to identifying the set of functions after which we don't restore
>  > window sizes, because just as we might want not to restore the height
>  > after `adjust-window-trailing-edge' was called, or
>  > `shrink-buffer-if-...' was called interactively, we similarly might want
>  > to resize the windows *only* when one of those functions is called (and
>  > only when interactively, in `shrink-buffer-if...' case).
>
> `vc-diff' unconditionally resizes the window.  What if someone simply
> doesn't want its `shrink-window-if-larger-than-buffer'?

The idea was to disable resizing at a lower level. For example, let's 
call the new variable `dont-resize'. If it's set to t, then 
`window--resize-this-window' won't do anything. You'd rebind 
`dont-resize' to nil locally in select cases: in 
`adjust-window-trailing-edge', when `shrink-buffer-if-...' is called 
interactively, etc. That's the rough outline.
If someone wants `shrink-window-if-...' to have no effect only in 
`vc-diff', well, that's a different goal.

>  >> in order to trigger automatic resizing of temporary buffer windows you
>  >> have to use `with-output-to-temp-buffer'.  If vc-diff had used that
>  >> macro, the entire resizing issue would have been handled already.
>  >
>  > It wouldn't, because `temp-buffer-resize-mode' is off by default.
>
> That's what I meant: In that case there would not have been any resizing
> unless triggered by `even-window-heights'.

I see.

>  > Or
>  > even if it were on by default, just because it could be switched on and
>  > off, the logic there can't depend on it.
>
> We could make the value at the time the window was reused prevail via
> the quit-restore parameter: That is, save the total size of the window
> only if either `even-window-heights' or `temp-buffer-resize-mode' are
> on.

I think it's a little backwards: if a user doesn't enable 
`temp-buffer-resize-mode' or `even-window-heights', then they probably 
care about keeping their window configuration the same over time.
If some command like `vc-diff' still can resize a window automatically, 
we should strive to keep that action undo-able.

There's a positive side to that suggestion (if the user sets up the 
preferred configuration after the temp buffer is displayed), but that 
doesn't outweigh the drawback.

> Let's close this thread as follows: Remove the `temp-buffer-resize-mode'
> check in `quit-window' and add an integerp check for the third element.

Agreed.

> If this has any bad consequences, we can still
>
> (1) have `vc-diff' use something similar to `with-output-to-temp-buffer'
>      so that `temp-buffer-resize-mode' is honored,
>
> (2) resize the window only if `temp-buffer-resize-mode' is enabled, or

Both 1 and 2 would be fine, I think, although 1 would require some extra 
work to make it work asynchronously. IIUC, `with-output-to-temp-buffer' 
waits until the called process completes, and `vc-diff' supports both 
synchronous and asynchronous execution.

> (3) resize the window unconditionally as now but locally set the
>      variable `temp-buffer-resize-mode' to t in the diff buffer.
>
> All these approaches would re-resize the window on quitting provided
> `even-window-heights' is nil.
>
> Independently from that we can provide an extra vc-prefixed variable
> which controls whether the diff window shall be resiized in the first
> place.  As you said, that would be a different bug.
>
> More generally, we could provide an option specifying a function or
> regexp for all buffers whose windows should be fit to them and have
> `fit-window-to-buffer' (unless called interactively) honor that.  That
> option would then override `temp-buffer-resize-mode' as well.

-- Dmitry

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > I tried it with just my patch applied, and it didn't work, because in
 > this case the stored height value was of already resized window:
 > `display-buffer-record-window' is called from `window--display-buffer',
 > and `display-buffer-use-some-window' calls `window--even-window-heights'
 > before `window--display-buffer'.

I think we could do

     (when window
       (prog1
	  (window--display-buffer buffer window 'reuse)
       (window--even-window-heights window)))

in `display-buffer-use-some-window'.

 > Maybe that's fine, I'll just set the variable to nil.

OK

 > Until then, the issue is somewhat alleviated by the fact that you can
 > press 'z' or 'C-x k' instead of 'q', and both of these won't trigger
 > height restoration.

I'm more concerned with the fact that an application might reuse the
shrunk window via `display-buffer'.

 > If someone wants `shrink-window-if-...' to have no effect only in
 > `vc-diff', well, that's a different goal.

But that's probably what some people want.

 >> Let's close this thread as follows: Remove the `temp-buffer-resize-mode'
 >> check in `quit-window' and add an integerp check for the third element.
 >
 > Agreed.

Can you install it?  Else please post the patch and a ChangeLog entry.

 >> (1) have `vc-diff' use something similar to `with-output-to-temp-buffer'
 >>      so that `temp-buffer-resize-mode' is honored,
 >>
 >> (2) resize the window only if `temp-buffer-resize-mode' is enabled, or
 >
 > Both 1 and 2 would be fine, I think, although 1 would require some extra
 > work to make it work asynchronously. IIUC, `with-output-to-temp-buffer'
 > waits until the called process completes, and `vc-diff' supports both
 > synchronous and asynchronous execution.

We need a susbstitute for `with-output-to-temp-buffer' for
asynchronously filled buffers.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

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

On 05.07.2012 13:53, martin rudalics wrote:
>  > I tried it with just my patch applied, and it didn't work, because in
>  > this case the stored height value was of already resized window:
>  > `display-buffer-record-window' is called from `window--display-buffer',
>  > and `display-buffer-use-some-window' calls `window--even-window-heights'
>  > before `window--display-buffer'.
>
> I think we could do
>
>      (when window
>        (prog1
>        (window--display-buffer buffer window 'reuse)
>        (window--even-window-heights window)))
>
> in `display-buffer-use-some-window'.

That should work, but IMO `even-window-heights' behavior is just too 
surprising (with its conditions on window positions), so, if you don't 
mind, I'll open another bug with proposal to change its default value, 
presenting the above as alternative.

>  > Until then, the issue is somewhat alleviated by the fact that you can
>  > press 'z' or 'C-x k' instead of 'q', and both of these won't trigger
>  > height restoration.
>
> I'm more concerned with the fact that an application might reuse the
> shrunk window via `display-buffer'.

In this case, unless the new buffer is the same as the one already 
displayed in the shrunk window, `display-buffer-record-window' will
overwrite the 'quit-restore parameter, so I don't see what the problem 
scenario would be. Hadn't managed to reproduce one either.

>  > If someone wants `shrink-window-if-...' to have no effect only in
>  > `vc-diff', well, that's a different goal.
>
> But that's probably what some people want.

I don't think we've seen a request exactly like that yet, but that would 
require a vc-prefixed variable.
Asynchronous `with-output-to-temp-buffer' proposal is better in that regard.

>  >> Let's close this thread as follows: Remove the
> `temp-buffer-resize-mode'
>  >> check in `quit-window' and add an integerp check for the third element.
>  >
>  > Agreed.
>
> Can you install it?  Else please post the patch and a ChangeLog entry.

Nope. Is unified diff okay?

window.el (quit-window): Always restore window height when it's saved in 
quit-restore parameter.

-- Dmitry

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

diff --git a/lisp/window.el b/lisp/window.el
index b362f40..f9adf84 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -3069,9 +3069,8 @@ one.  If non-nil, reset `quit-restore' parameter to nil."
 	   (buffer-live-p (car quad))
 	   (eq (nth 3 quit-restore) buffer))
       ;; Show another buffer stored in quit-restore parameter.
-      (setq resize (with-current-buffer buffer
-		     (and temp-buffer-resize-mode
-			  (/= (nth 3 quad) (window-total-size window)))))
+      (setq resize (and (integerp (nth 3 quad))
+                        (/= (nth 3 quad) (window-total-size window))))
       (set-window-dedicated-p window nil)
       (when resize
 	;; Try to resize WINDOW to its old height but don't signal an

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[martin rudalics]

 > That should work, but IMO `even-window-heights' behavior is just too
 > surprising (with its conditions on window positions), so, if you don't
 > mind, I'll open another bug with proposal to change its default value,
 > presenting the above as alternative.

OK

 >> I'm more concerned with the fact that an application might reuse the
 >> shrunk window via `display-buffer'.
 >
 > In this case, unless the new buffer is the same as the one already
 > displayed in the shrunk window, `display-buffer-record-window' will
 > overwrite the 'quit-restore parameter, so I don't see what the problem
 > scenario would be. Hadn't managed to reproduce one either.

Consider users with always <= 2 windows per frame: If `display-buffer'
displays some temporary buffer in the other, reused window, shrinking it
to some few lines, calling `switch-to-buffer-other-window' in that
situation won't be of much fun.

 >>  > If someone wants `shrink-window-if-...' to have no effect only in
 >>  > `vc-diff', well, that's a different goal.
 >>
 >> But that's probably what some people want.
 >
 > I don't think we've seen a request exactly like that yet, but that would
 > require a vc-prefixed variable.

When we get such a request you'll take care of it ;-)

 > window.el (quit-window): Always restore window height when it's saved in
 > quit-restore parameter.

Installed as revision 108897.

martin

bug#11810: 24.1.50; `vc-diff' shrinks pre-existing window

[Dmitry Gutov]

On 06.07.2012 10:36, martin rudalics wrote:
>  >> I'm more concerned with the fact that an application might reuse the
>  >> shrunk window via `display-buffer'.
>  >
>  > In this case, unless the new buffer is the same as the one already
>  > displayed in the shrunk window, `display-buffer-record-window' will
>  > overwrite the 'quit-restore parameter, so I don't see what the problem
>  > scenario would be. Hadn't managed to reproduce one either.
>
> Consider users with always <= 2 windows per frame: If `display-buffer'
> displays some temporary buffer in the other, reused window, shrinking it
> to some few lines, calling `switch-to-buffer-other-window' in that
> situation won't be of much fun.

But that's an old issue. We could either prohibit shrinking, which 
apparently isn't an option, or restore saved height before displaying 
the new buffer (in `display-buffer-use-some-window', for example).
Would that be the expected behavior?

>  >>  > If someone wants `shrink-window-if-...' to have no effect only in
>  >>  > `vc-diff', well, that's a different goal.
>  >>
>  >> But that's probably what some people want.
>  >
>  > I don't think we've seen a request exactly like that yet, but that would
>  > require a vc-prefixed variable.
>
> When we get such a request you'll take care of it ;-)

Maybe. :) You'd have to notify me if I miss the bug.

>  > window.el (quit-window): Always restore window height when it's saved in
>  > quit-restore parameter.
>
> Installed as revision 108897.

Works as expected. I believe this can be tagged as fixed now.

--Dmitry