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

[Dmitry Gutov <dgutov@yandex.ru>]

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