Re: Add function to rotate/transpose all windows

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 27 Sep 2024 19:29:21 +0200
> Cc: pranshusharma366@gmail.com, emacs-devel@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > This doc string needs to be more detailed in what it means to "put the
>  > window object of DEAD in the place of LIVE".  For example, it
>  > currently keeps silent about what happens to LIVE after the call.
> 
> The fact that LIVE existed at all was a misfeature.  I now put the dead
> window right into the new window created by 'split-window' with the help
> of a new argument.  Patch attached.

Thanks, but does it really make a lot of sense to make this a
side-effect of splitting a window?  (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.)

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

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.

> +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?  More generally, I don't think I understand what
you wanted to say by "or an arbitrary live 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:

> +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(??).

> +	  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?

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

> +	  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?

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.

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

Also, "new" should be up-cased.

> +	 resize the parent, reset new's buffer to nil and do the real
> +	 set_window_buffer.  */

Likewise.

> +      /* 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.

>  	  unchain_marker (XMARKER (w->start));
> +	  wset_old_buffer (w, w->contents);

What is this about?

> @@ -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?