Re: Add function to rotate/transpose all windows

[martin rudalics <rudalics@gmx.at>]

 > Thanks, but does it really make a lot of sense to make this a
 > side-effect of splitting a window?

It makes sense because (1) we do not have to make a new window that gets
deleted right away and (2) the geometry of the new window will be
handled by 'split-window' directly and not by some obscure copying
routine (I only later noted that 'resurrect-window' got me warnings
about memcpy combined with offsetof writing beyond the bounds of window
structures).

 > (If it makes sense due to
 > technical reasons, such as commonality of code of the implementation,
 > we could have a common internal subroutine with 2 separate APIs
 > exposed to Lisp.)

We can write a separate 'split-window-reuse-existing' function and have
it call a 'split-window-reuse-existing-internal' routine so we don't
compromise the present 'split-window' at all.  It's up to you what you
like more.

 > Or maybe it _will_ make a lot of sense if you reword the doc string so
 > that it explains why what we do with REFER is a variant of splitting a
 > window.  Something like
 >
 >    Instead of making a new window, this function can reuse an existing
 >    dead window...

OK.

 > Btw, "dead window" is mentioned only once in the ELisp manual, and
 > even that in passing, so it is not a very clear terminology, and might
 > need clarifications in this case.

The problem is that in Emacs a live window is a window that shows a
buffer.  Which implies that internal windows are dead.  I once tried to
convince Chong that this terminology is misleading but he didn't allow
me to change it.

 >> +If the optional fifth argument REFER is non-nil, it has to denote a
 >> +dead, former live window on the same frame as OLD or an arbitrary live
 >> +window.                                       ^^^
 >
 > What is "OLD" here?

It should be WINDOW.  OLD is the term used by 'split-window-internal'.

 > More generally, I don't think I understand what
 > you wanted to say by "or an arbitrary live window".

That's a separate functionality to give users more control of the window
the new window inherits properties from.  It's useful when splitting an
internal window and the _new_ window should get its initial buffer and
other properties from any but the frame's selected window.

 >> In the first case, REFER will become the new window with
 >> +properties like buffer, start and point, decorations and parameters as
 >> +to the last time when it was live.  In the latter case the new window
 >> +will inherit properties like buffer, start and point, decorations and
 >> +parameters from REFER.
 >
 > I suggest to describe each case separately, i.e. split the previous
 > sentence ("it has to denote ... or ...") into two, and explain
 > separately what happens in each case, instead of complicating with the
 > former and the latter.  Especially as there are additional cases (nil
 > or omitted), which add to the confusion:

I'll try to do that.

 >> +If REFER is nil or omitted, then if WINDOW is live, any such properties
 >> +are inherited from WINDOW.  If, however, WINDOW is an internal window,
 >> +the new window will inherit these properties from the window selected on
 >> +WINDOW's frame.
 >
 >> +	  if (!BUFFERP (r->old_buffer))
 >> +	    error ("Dead reference window was not a live window");
 >                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 > This seems to imply that the function expects a dead window to be a
 > live window(??).

In our misleading terminology a dead window can be either (i) a former
live window or (ii) an internal or former internal window.  I have to
exclude (ii) because an internal window should never become a leaf
window and vice-versa.

 >> +	  else if (!BUFFER_LIVE_P (XBUFFER (r->old_buffer)))
 >> +	    error ("Dead reference window's old buffer is dead");
 >
 > Will users understand what is "old buffer" of a window?

It's the buffer returned by 'window-old-buffer'.  I will have to explain
this in more detail.

 >> +	  else if (!EQ (r->frame, frame))
 >> +	    error ("Dead referenec window was on other frame");
 >                           ^^^^^^^^^
 > Typo.  Also, I'd say "was on a frame other that that of window being
 > split" or something like that.

OK.

 >> +	  dead = true;
 >> +	}
 >> +      else if (!WINDOW_LIVE_P (refer))
 >> +	error ("Reference window must not be internal");
 >
 > Are we sure "internal" here will be understood?  How about using "leaf
 > window" instead?

But it's the opposite - a "non-leaf window".  If you (or anyone else)
have any proposals on how to improve the nomenclature in this area, I'll
be all ears.

 > More generally, the text of these error messages is not easily
 > correlated to the problematic argument, because it neither mentions
 > the argument by its exact name, nor mentions the problematic window by
 > any other specific reference.

So the error messages should explain in more detail what when wrong.

 >> +      /* Provisorially set new's buffer to that of the reference window,
 >              ^^^^^^^^^^^^^
 > Did you mean "provisionally"? or maybe "temporarily"?

Both would fit the bill.  But this is code that I didn't intend to
submit is part of a completely different changeset where the sizes
of an individual window would be needed to set up the size hints of
frames correctly in set_window_buffer.

 >> +      /* Set buffer of NEW to buffer of reference window.  We have to do it
 >> +	 here so the sizes of NEW are in place.  But be sure to do it before
 >> +	 adjusting the frame glyphs - otherwise Emacs may inexplicably loop
 >> +	 forever.  */
 >
 > This should be more explicit what code below adjusts the frame's
 > glyphs, because without that this very good comment is less useful
 > than it could be.

Again part of that other changeset.

 >>   	  unchain_marker (XMARKER (w->start));
 >> +	  wset_old_buffer (w, w->contents);
 >
 > What is this about?

When we make a new window and delete it before redisplay runs
window_change_functions for the first time, the old buffer of the window
is nil.  But that would have caused the "Dead reference window's old
buffer is dead" error with the earlier proposed 'resurrect-window' while
in fact the window (made by 'split-window' right before) did have an old
buffer.  It has no significance for the new 'split-window'.

 >> @@ -7720,6 +7795,7 @@ delete_all_child_windows (Lisp_Object window)
 >>   	 only, we use this slot to save the buffer for the sake of
 >>   	 possible resurrection in Fset_window_configuration.  */
 >>         wset_combination_limit (w, w->contents);
 >> +      wset_old_buffer (w, w->contents);
 >
 > And this?

Same explanation.  The code for Fdelete_window_internal and
delete_all_child_windows should be refactored so that they would call a
common delete_leaf_window.

Thanks for the careful reading, martin