bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Juri Linkov]

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

Tags: patch

etc/NEWS:

  *** New commands 'split-root-window-below' and 'split-root-window-right'.
  These commands split the root window in two, and are bound to 'C-x w 2'
  and 'C-x w 3', respectively.

0. emacs -Q
1. (setq split-window-keep-point nil)
2. C-x w 2
3. C-x w 2

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  split-window-below(nil #<window 14>)
  split-root-window-below(nil)
  funcall-interactively(split-root-window-below nil)
  command-execute(split-root-window-below)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: split-window-keep-point.patch --]
[-- Type: text/x-diff, Size: 1678 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 4099b707009..53d34a3bf6e 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5670,7 +5675,8 @@ split-window-keep-point
 
 (defun split-window-below (&optional size window-to-split)
   "Split WINDOW-TO-SPLIT into two windows, one above the other.
-WINDOW-TO-SPLIT is above.  The newly split-off window is
+WINDOW-TO-SPLIT defaults to the selected window and after
+splitting remains above.  The newly split-off window is
 below and displays the same buffer.  Return the new window.
 
 If optional argument SIZE is omitted or nil, both windows get the
@@ -5691,7 +5697,9 @@ split-window-below
       ;; `split-window' would not signal an error here.
       (error "Size of new window too small"))
     (setq new-window (split-window window-to-split size))
-    (unless split-window-keep-point
+    (unless (or split-window-keep-point
+                ;; `window-buffer' is nil for `frame-root-window'
+                (null (window-buffer window-to-split)))
       (with-current-buffer (window-buffer window-to-split)
 	;; Use `save-excursion' around vertical movements below
 	;; (Bug#10971).  Note: When WINDOW-TO-SPLIT's buffer has a
@@ -5732,7 +5740,8 @@ split-root-window-below
 
 (defun split-window-right (&optional size window-to-split)
   "Split WINDOW-TO-SPLIT into two side-by-side windows.
-WINDOW-TO-SPLIT is on the left.  The newly split-off window is on
+WINDOW-TO-SPLIT defaults to the selected window and after
+splitting remains on the left.  The newly split-off window is on
 the right and displays the same buffer.  Return the new window.
 
 If optional argument SIZE is omitted or nil, both windows get the

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 17 Jan 2023 19:10:48 +0200
> 
> etc/NEWS:
> 
>   *** New commands 'split-root-window-below' and 'split-root-window-right'.
>   These commands split the root window in two, and are bound to 'C-x w 2'
>   and 'C-x w 3', respectively.
> 
> 0. emacs -Q
> 1. (setq split-window-keep-point nil)
> 2. C-x w 2
> 3. C-x w 2
> 
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   split-window-below(nil #<window 14>)
>   split-root-window-below(nil)
>   funcall-interactively(split-root-window-below nil)
>   command-execute(split-root-window-below)
> 
> diff --git a/lisp/window.el b/lisp/window.el
> index 4099b707009..53d34a3bf6e 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -5670,7 +5675,8 @@ split-window-keep-point
>  
>  (defun split-window-below (&optional size window-to-split)
>    "Split WINDOW-TO-SPLIT into two windows, one above the other.
> -WINDOW-TO-SPLIT is above.  The newly split-off window is
> +WINDOW-TO-SPLIT defaults to the selected window and after
> +splitting remains above.  The newly split-off window is

"Remains" is not a good word here: we are splitting the window in two,
so none of them "remains".  I'd say "...and will be above the other
window after splitting".

>  (defun split-window-right (&optional size window-to-split)
>    "Split WINDOW-TO-SPLIT into two side-by-side windows.
> -WINDOW-TO-SPLIT is on the left.  The newly split-off window is on
> +WINDOW-TO-SPLIT defaults to the selected window and after
> +splitting remains on the left.  The newly split-off window is on
>  the right and displays the same buffer.  Return the new window.

Same here.

Martin, any comments?

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > Martin, any comments?

'split-window-below' in its present form doesn't make sense when
WINDOW-TO-SPLIT is life but is not the selected window.  It calls
'window-point', 'window-start', 'window-height', ... without an argument
- which means to implicitly use the selected window as argument instead
of WINDOW-TO-SPLIT.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > WINDOW-TO-SPLIT is life but is not the selected window.  It calls
                     _live_ was meant here

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Juri Linkov]

close 60886 29.0.60
thanks

> 'split-window-below' in its present form doesn't make sense when
> WINDOW-TO-SPLIT is life but is not the selected window.  It calls
> 'window-point', 'window-start', 'window-height', ... without an argument
> - which means to implicitly use the selected window as argument instead
> of WINDOW-TO-SPLIT.

Thanks for mentioning this problem.  Now fixed in the commit 758ac5eabbe.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > Thanks for mentioning this problem.  Now fixed in the commit 758ac5eabbe.

 > +the other window after splitting.  The newly split-off window is
 >  below and displays the same buffer.  Return the new window.

 > +left after splitting.  The newly split-off window is on the right and
 > +displays the same buffer.  Return the new window.

What is the "same buffer" when the root window is internal in
split-root-window-right/-below?

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Thu, 19 Jan 2023 00:03:12 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > Thanks for mentioning this problem.  Now fixed in the commit 758ac5eabbe.
> 
>  > +the other window after splitting.  The newly split-off window is
>  >  below and displays the same buffer.  Return the new window.
> 
>  > +left after splitting.  The newly split-off window is on the right and
>  > +displays the same buffer.  Return the new window.
> 
> What is the "same buffer" when the root window is internal in
> split-root-window-right/-below?

I guess "the same buffer as the window which was split"?

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 >> What is the "same buffer" when the root window is internal in
 >> split-root-window-right/-below?
 >
 > I guess "the same buffer as the window which was split"?

In the case sketched above the window which was split had no buffer.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Thu, 19 Jan 2023 09:49:06 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> What is the "same buffer" when the root window is internal in
>  >> split-root-window-right/-below?
>  >
>  > I guess "the same buffer as the window which was split"?
> 
> In the case sketched above the window which was split had no buffer.

I guess I don't understand the use case then? how can that happen?
And what would you suggest to say in the doc string instead?

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > I guess I don't understand the use case then? how can that happen?

With emacs -Q do

M-: (pop-to-buffer "*Messages*")
C-x w 3

 > And what would you suggest to say in the doc string instead?

Either nothing or what 'split-window' says in that case: That the new
window shows the buffer of the window selected on the frame containing
the window that was split.

Functions like 'split-window-below' were historically intended for
interactive use only.  The Elisp manual said about them

    For interactive use, Emacs provides two commands which always split
    the selected window.

which apparently is no longer true.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Thu, 19 Jan 2023 11:43:09 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > I guess I don't understand the use case then? how can that happen?
> 
> With emacs -Q do
> 
> M-: (pop-to-buffer "*Messages*")
> C-x w 3

This shows *Messages* in the new window, so I think something like

  ...the same buffer as the window which was selected when the
  function was invoked"

should be good.

>  > And what would you suggest to say in the doc string instead?
> 
> Either nothing or what 'split-window' says in that case: That the new
> window shows the buffer of the window selected on the frame containing
> the window that was split.

See above.

> Functions like 'split-window-below' were historically intended for
> interactive use only.  The Elisp manual said about them
> 
>     For interactive use, Emacs provides two commands which always split
>     the selected window.
> 
> which apparently is no longer true.

That doesn't say "only for interactive use", and it would make no
sense to say that for such a basic function, IMO.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 >> M-: (pop-to-buffer "*Messages*")
 >> C-x w 3
 >
 > This shows *Messages* in the new window, so I think something like
 >
 >    ...the same buffer as the window which was selected when the
 >    function was invoked"
 >
 > should be good.

This would miss the case where WINDOW-TO-SPLIT is live and not selected.
And also the case where the root window of a non-selected frame should
be split.

 >> The Elisp manual said about them
 >>
 >>      For interactive use, Emacs provides two commands which always split
 >>      the selected window.
 >>
 >> which apparently is no longer true.
 >
 > That doesn't say "only for interactive use", and it would make no
 > sense to say that for such a basic function, IMO.

I meant the "always split the selected window" part.  Just think of how
much easier it would have been, had these new functions been coded as

(defun split-root-window-below (&optional size)
   "Split root window of selected frame in two.
The current window configuration is retained in the top window,
the lower window takes up the whole width of the frame.  SIZE is
handled as in `split-window-below'.  The buffer of the new window is
taken from the selected window."
   (interactive "P")
   (when (and size (< size 0) (< (- size) window-min-height))
     ;; `split-window' would not signal an error here.
     (error "Size of new window too small"))
   (split-window (frame-root-window) size 'below))

(defun split-root-window-right (&optional size)
   "Split root window of selected frame into two side-by-side windows.
The current window configuration is retained within the left
window, and a new window is created on the right, taking up the
whole height of the frame.  SIZE is treated as by
`split-window-right'.  The buffer of the new window is taken from
the selected window."
   (interactive "P")
   (when (and size (< size 0) (< (- size) window-min-width))
     ;; `split-window' would not signal an error here.
     (error "Size of new window too small"))
   (split-window (frame-root-window) size 'right))

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Fri, 20 Jan 2023 10:17:39 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> M-: (pop-to-buffer "*Messages*")
>  >> C-x w 3
>  >
>  > This shows *Messages* in the new window, so I think something like
>  >
>  >    ...the same buffer as the window which was selected when the
>  >    function was invoked"
>  >
>  > should be good.
> 
> This would miss the case where WINDOW-TO-SPLIT is live and not selected.
> And also the case where the root window of a non-selected frame should
> be split.

OK, then how about

   ...the same buffer as shown in WINDOW-TO-SPLIT

?

>  >> The Elisp manual said about them
>  >>
>  >>      For interactive use, Emacs provides two commands which always split
>  >>      the selected window.
>  >>
>  >> which apparently is no longer true.
>  >
>  > That doesn't say "only for interactive use", and it would make no
>  > sense to say that for such a basic function, IMO.
> 
> I meant the "always split the selected window" part.

That is still correct in interactive use, isn't it?

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > OK, then how about
 >
 >     ...the same buffer as shown in WINDOW-TO-SPLIT

We are going in circles.  This fails when WINDOW-TO-SPLIT is internal.

 > I meant the "always split the selected window" part.
 >
 > That is still correct in interactive use, isn't it?

It's misleading as the current discussion shows.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Sat, 21 Jan 2023 11:08:32 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > OK, then how about
>  >
>  >     ...the same buffer as shown in WINDOW-TO-SPLIT
> 
> We are going in circles.

I'm trying my best, sorry.  Could use some help, I guess.

> This fails when WINDOW-TO-SPLIT is internal.

Which buffer will be shown in the new window in that case?

>  > I meant the "always split the selected window" part.
>  >
>  > That is still correct in interactive use, isn't it?
> 
> It's misleading as the current discussion shows.

It's a minor issue, so let's not lose the focus.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 >> This fails when WINDOW-TO-SPLIT is internal.
 >
 > Which buffer will be shown in the new window in that case?

If WINDOW-TO-SPLIT is live, the buffer shown in the new window is the
buffer shown in WINDOW-TO-SPLIT.

If WINDOW-TO-SPLIT is not live, the buffer shown in the new window is
the buffer shown in the selected window of WINDOW-TO-SPLIT's frame.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Sat, 21 Jan 2023 16:30:57 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> This fails when WINDOW-TO-SPLIT is internal.
>  >
>  > Which buffer will be shown in the new window in that case?
> 
> If WINDOW-TO-SPLIT is live, the buffer shown in the new window is the
> buffer shown in WINDOW-TO-SPLIT.
> 
> If WINDOW-TO-SPLIT is not live, the buffer shown in the new window is
> the buffer shown in the selected window of WINDOW-TO-SPLIT's frame.

Thanks.  Then I suggest

    Split WINDOW-TO-SPLIT into two windows, one above the other.
  WINDOW-TO-SPLIT defaults to the selected window if omitted or nil.
  The newly created window will be below WINDOW-TO-SPLIT and will show
  the same buffer as WINDOW-TO-SPLIT, if it is a live window, else the
  buffer shown in the WINDOW-TO-SPLIT's frame's selected window.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Juri Linkov]

> Just think of how much easier it would have been, had these new
> functions been coded as
>
> (defun split-root-window-below (&optional size)
>   "Split root window of selected frame in two.
> The current window configuration is retained in the top window,
> the lower window takes up the whole width of the frame.  SIZE is
> handled as in `split-window-below'.  The buffer of the new window is
> taken from the selected window."
>   (interactive "P")
>   (when (and size (< size 0) (< (- size) window-min-height))
>     ;; `split-window' would not signal an error here.
>     (error "Size of new window too small"))
>   (split-window (frame-root-window) size 'below))
>
> (defun split-root-window-right (&optional size)
>   "Split root window of selected frame into two side-by-side windows.
> The current window configuration is retained within the left
> window, and a new window is created on the right, taking up the
> whole height of the frame.  SIZE is treated as by
> `split-window-right'.  The buffer of the new window is taken from
> the selected window."
>   (interactive "P")
>   (when (and size (< size 0) (< (- size) window-min-width))
>     ;; `split-window' would not signal an error here.
>     (error "Size of new window too small"))
>   (split-window (frame-root-window) size 'right))

I proposed the same in bug#56767.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 >      Split WINDOW-TO-SPLIT into two windows, one above the other.
 >    WINDOW-TO-SPLIT defaults to the selected window if omitted or nil.
 >    The newly created window will be below WINDOW-TO-SPLIT and will show
 >    the same buffer as WINDOW-TO-SPLIT, if it is a live window, else the
 >    buffer shown in the WINDOW-TO-SPLIT's frame's selected window.

Sounds correct.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > I proposed the same in bug#56767.

That would have avoided nonsense like retrieving 'window-point' of an
uninvolved window or the 'quit-restore' parameter of an internal window.

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Juri Linkov]

>> I proposed the same in bug#56767.
>
> That would have avoided nonsense like retrieving 'window-point' of an
> uninvolved window or the 'quit-restore' parameter of an internal window.

Then I don't know what to do at this time before the release:
whether to rewrite them or not.

Also these commands are missing:

(defun split-root-window-above (&optional size)
  (interactive "P")
  (when (and size (< size 0) (< (- size) window-min-height))
    ;; `split-window' would not signal an error here.
    (error "Size of new window too small"))
  (split-window (frame-root-window) size 'above))

(defun split-root-window-left (&optional size)
  (interactive "P")
  (when (and size (< size 0) (< (- size) window-min-width))
    ;; `split-window' would not signal an error here.
    (error "Size of new window too small"))
  (split-window (frame-root-window) size 'left))

There are no corresponding split-window-above and split-window-left,
because they provide the same result as the existing
split-window-below and split-window-right.  But there is no way
to add a window on the left/above for the root window
by using split-root-window-below or split-root-window-right.
Therefore split-root-window-above and split-root-window-left
are needed.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Juri Linkov]

>> I proposed the same in bug#56767.
>
> That would have avoided nonsense like retrieving 'window-point' of an
> uninvolved window or the 'quit-restore' parameter of an internal window.

Maybe if it's an internal window it should fall back to the selected window
to get the 'quit-restore' parameter?

OTOH, the new argument WINDOW-TO-SPLIT makes sense for
split-window-below and split-window-right when it's not
an internal window, to be able to split any non-selected window.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Sun, 22 Jan 2023 10:53:20 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >      Split WINDOW-TO-SPLIT into two windows, one above the other.
>  >    WINDOW-TO-SPLIT defaults to the selected window if omitted or nil.
>  >    The newly created window will be below WINDOW-TO-SPLIT and will show
>  >    the same buffer as WINDOW-TO-SPLIT, if it is a live window, else the
>  >    buffer shown in the WINDOW-TO-SPLIT's frame's selected window.
> 
> Sounds correct.

Thanks, installed.

Btw, the doc string of split-window-right incorrectly said that the
new window will be to the _left_; I fixed that as well.

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[martin rudalics]

 > Btw, the doc string of split-window-right incorrectly said that the
 > new window will be to the _left_; I fixed that as well.

Are you sure?  Looking at this diff

https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-29&id=41f497c8bee6e36a621a6d9088636069159387a9

I see

-WINDOW-TO-SPLIT defaults to the selected window and and will be on the
-left after splitting.  The newly split-off window is on the right and
-displays the same buffer.  Return the new window.
+WINDOW-TO-SPLIT defaults to the selected window if omitted or nil.
+The newly created window will be to the right of WINDOW-TO-SPLIT and
+will show the same buffer as WINDOW-TO-SPLIT, if it is a live window,
+else the buffer shown in the WINDOW-TO-SPLIT's frame's selected window.
+Return the new window.

Or am I missing something?

martin

bug#60886: 29.0.60; split-root-window-below broken for split-window-keep-point

[Eli Zaretskii]

> Date: Thu, 26 Jan 2023 16:44:45 +0100
> Cc: juri@linkov.net, 60886@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > Btw, the doc string of split-window-right incorrectly said that the
>  > new window will be to the _left_; I fixed that as well.
> 
> Are you sure?  Looking at this diff
> 
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-29&id=41f497c8bee6e36a621a6d9088636069159387a9
> 
> I see
> 
> -WINDOW-TO-SPLIT defaults to the selected window and and will be on the
> -left after splitting.  The newly split-off window is on the right and
> -displays the same buffer.  Return the new window.
> +WINDOW-TO-SPLIT defaults to the selected window if omitted or nil.
> +The newly created window will be to the right of WINDOW-TO-SPLIT and
> +will show the same buffer as WINDOW-TO-SPLIT, if it is a live window,
> +else the buffer shown in the WINDOW-TO-SPLIT's frame's selected window.
> +Return the new window.
> 
> Or am I missing something?

Ignore me, the original text confused me.  The new one doesn't.