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

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

[martin rudalics]

 > I don't understand what "implicitly" means here. Will
 > `even-window-heights' be nil by default?

My description was sloppy and inaccurate.

 >  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.

 >> 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.

(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.

 > 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.

 > 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.

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

(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

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

WDYT?

martin

Re: 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

Re: 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

Re: 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.