unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: master b3dd0ce: Provide new option `delete-window-set-selected'
@ 2021-06-10  8:37 Eli Zaretskii
  2021-06-10 15:01 ` martin rudalics
  0 siblings, 1 reply; 5+ messages in thread
From: Eli Zaretskii @ 2021-06-10  8:37 UTC (permalink / raw)
  To: Martin Rudalics; +Cc: emacs-devel

A few comments about this changeset:

Why a variable that is a user option is documented only in the ELisp
manual?  I think it should also be mentioned in the "Change Window"
node of the user manual, where we describe "C-x 0" and "C-x 1".

> +Possible choices are @code{mru} (the default) to select the most
> +recently used window on that frame and @code{pos} to choose the window
                                     ^
There should be a comma before "and" there.

> +at the position of point of the previously selected window.

This part:

  [...] 'pos' to choose the window at the position of point of the
  previously selected window [...]

is confusing: when you say "position of point", the most likely
interpretation is the buffer position of point, which makes the
meaning of the value nonsensical.  I understand that you meant the
frame-relative coordinates of point instead, so the text (here and in
the doc string) should IMO be amended to say that.

> +                                                             If this
> +option is @code{nil}, it means to choose the frame's first window
> +instead.

This text doesn't explain what does "the first window" mean in this
context.

> +           Note that a window with a non-@code{nil}
> +@code{no-other-window} parameter is never chosen.

Doesn't this depend on some argument to the relevant functions?

>  @cindex largest window
> -@defun get-largest-window &optional all-frames dedicated not-selected
> +@defun get-largest-window &optional all-frames dedicated not-selected no-other
>  This function returns the window with the largest area (height times
> -width).  The optional argument @var{all-frames} specifies the windows to
> -search, and has the same meaning as in @code{next-window}.
> -
> -A minibuffer window is never a candidate.  A dedicated window
> -(@pxref{Dedicated Windows}) is never a candidate unless the optional
> -argument @var{dedicated} is non-@code{nil}.  The selected window is not
> -a candidate if the optional argument @var{not-selected} is
> -non-@code{nil}.  If the optional argument @var{not-selected} is
> -non-@code{nil} and the selected window is the only candidate, this
> -function returns @code{nil}.
> -
> -If there are two candidate windows of the same size, this function
> -prefers the one that comes first in the cyclic ordering of windows,
> -starting from the selected window.
> +width).  If there are two candidate windows of the same size, it prefers
> +the one that comes first in the cyclic ordering of windows, starting
> +from the selected window.  The meaning of the arguments is the same as
> +for @code{get-lru-window}.
>  @end defun

This hunk loses the information about minibuffer windows and dedicated
windows, at least.  it also seems to lose the information about when
the selected window isn't a candidate.  Why?

> +(defun window-at-pos (x y &optional frame no-other)
> +  "Return live window at coordinates X, Y on specified FRAME.

A better name for this function is window-at-x-y, IMO.  "Pos" can have
ambiguous interpretations, see above.

> +X and Y are counted in pixels from an origin at 0, 0 of FRAME's
> +native frame.  A coordinate on an edge shared by two windows is
> +attributed to the window on the right (or below).  Return nil if

If you say "FRAME-relative coordinates", doesn't that tell the same
story, just much more succinctly and clearly?

> +(defcustom delete-window-set-selected 'mru

Wouldn't the name delete-window-choose-selected be better?  The doc
string says that much:

> +  "How to choose a frame's selected window after window deletion.

Or maybe delete-selected-window-choose-replacement?

> +               ;; If `delete-window-internal' selected a window with a
> +               ;; non-nil 'no-other-window' parameter as its frame's
> +               ;; selected window, try to choose another one.
> +               (catch 'found
> +                 (walk-window-tree
> +                  (lambda (other)
> +                    (unless (window-parameter other 'no-other-window)
> +                      (set-frame-selected-window frame other)
> +                      (throw 'found t)))
> +                  frame))))

That "try" means that we could sometimes fail, and return a window
with a non-nil 'no-other-window' parameter, right?  If so, the doc
strings, which say such a window will _never_ be returned, are
inaccurate, right?

Thanks.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: master b3dd0ce: Provide new option `delete-window-set-selected'
  2021-06-10  8:37 master b3dd0ce: Provide new option `delete-window-set-selected' Eli Zaretskii
@ 2021-06-10 15:01 ` martin rudalics
  2021-06-10 15:29   ` Eli Zaretskii
  0 siblings, 1 reply; 5+ messages in thread
From: martin rudalics @ 2021-06-10 15:01 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

 > A few comments about this changeset:

Appreciated!

 > Why a variable that is a user option is documented only in the ELisp
 > manual?  I think it should also be mentioned in the "Change Window"
 > node of the user manual, where we describe "C-x 0" and "C-x 1".

In a restricted sense I suppose because we nowhere describe terms like
"frame selected window" or "most recently used window" there.  So in the
C-x 0 paragraph something like "The option @code{delete-window..} allows
to choose which window becomes the new selected window instead." should
do what you have in mind here.  OK?

 >> +           Note that a window with a non-@code{nil}
 >> +@code{no-other-window} parameter is never chosen.
 >
 > Doesn't this depend on some argument to the relevant functions?

Yes.  I have added such an argument to `get-mru-window' and friends.
But see my last remark below.

 >>   @cindex largest window
 >> -@defun get-largest-window &optional all-frames dedicated not-selected
 >> +@defun get-largest-window &optional all-frames dedicated not-selected no-other
 >>   This function returns the window with the largest area (height times
 >> -width).  The optional argument @var{all-frames} specifies the windows to
 >> -search, and has the same meaning as in @code{next-window}.
 >> -
 >> -A minibuffer window is never a candidate.  A dedicated window
 >> -(@pxref{Dedicated Windows}) is never a candidate unless the optional
 >> -argument @var{dedicated} is non-@code{nil}.  The selected window is not
 >> -a candidate if the optional argument @var{not-selected} is
 >> -non-@code{nil}.  If the optional argument @var{not-selected} is
 >> -non-@code{nil} and the selected window is the only candidate, this
 >> -function returns @code{nil}.
 >> -
 >> -If there are two candidate windows of the same size, this function
 >> -prefers the one that comes first in the cyclic ordering of windows,
 >> -starting from the selected window.
 >> +width).  If there are two candidate windows of the same size, it prefers
 >> +the one that comes first in the cyclic ordering of windows, starting
 >> +from the selected window.  The meaning of the arguments is the same as
 >> +for @code{get-lru-window}.
 >>   @end defun
 >
 > This hunk loses the information about minibuffer windows and dedicated
 > windows, at least.  it also seems to lose the information about when
 > the selected window isn't a candidate.  Why?

This information is now provided by the sentence "The meaning of the
arguments is the same as for ‘get-lru-window’."  just as with the
preceding `get-mru-window'.

 >> +(defun window-at-pos (x y &optional frame no-other)
 >> +  "Return live window at coordinates X, Y on specified FRAME.
 >
 > A better name for this function is window-at-x-y, IMO.  "Pos" can have
 > ambiguous interpretations, see above.

So `window-at-x-y' be it.

 >> +X and Y are counted in pixels from an origin at 0, 0 of FRAME's
 >> +native frame.  A coordinate on an edge shared by two windows is
 >> +attributed to the window on the right (or below).  Return nil if
 >
 > If you say "FRAME-relative coordinates", doesn't that tell the same
 > story, just much more succinctly and clearly?

It does.

 >> +(defcustom delete-window-set-selected 'mru
 >
 > Wouldn't the name delete-window-choose-selected be better?

I'll go for that unless Juri has a better idea.

 > The doc
 > string says that much:
 >
 >> +  "How to choose a frame's selected window after window deletion.
 >
 > Or maybe delete-selected-window-choose-replacement?
 >
 >> +               ;; If `delete-window-internal' selected a window with a
 >> +               ;; non-nil 'no-other-window' parameter as its frame's
 >> +               ;; selected window, try to choose another one.
 >> +               (catch 'found
 >> +                 (walk-window-tree
 >> +                  (lambda (other)
 >> +                    (unless (window-parameter other 'no-other-window)
 >> +                      (set-frame-selected-window frame other)
 >> +                      (throw 'found t)))
 >> +                  frame))))
 >
 > That "try" means that we could sometimes fail, and return a window
 > with a non-nil 'no-other-window' parameter, right?  If so, the doc
 > strings, which say such a window will _never_ be returned, are
 > inaccurate, right?

Functions like `get-mru-window' never return such a window provided
their NO-OTHER argument has been set.  The snippet you cite is from
`delete-window' which, in that case, leaves the window selected by
`delete-window-internal' alone and these doc strings are right.  But
I'll amend the documentation of the new option appropriately.

Thanks, martin




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: master b3dd0ce: Provide new option `delete-window-set-selected'
  2021-06-10 15:01 ` martin rudalics
@ 2021-06-10 15:29   ` Eli Zaretskii
  2021-06-11  8:07     ` martin rudalics
  0 siblings, 1 reply; 5+ messages in thread
From: Eli Zaretskii @ 2021-06-10 15:29 UTC (permalink / raw)
  To: martin rudalics; +Cc: emacs-devel

> Cc: emacs-devel@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 10 Jun 2021 17:01:59 +0200
> 
> In a restricted sense I suppose because we nowhere describe terms like
> "frame selected window" or "most recently used window" there.  So in the
> C-x 0 paragraph something like "The option @code{delete-window..} allows
> to choose which window becomes the new selected window instead." should
> do what you have in mind here.  OK?

Yes.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: master b3dd0ce: Provide new option `delete-window-set-selected'
  2021-06-10 15:29   ` Eli Zaretskii
@ 2021-06-11  8:07     ` martin rudalics
  2021-06-11 11:56       ` Eli Zaretskii
  0 siblings, 1 reply; 5+ messages in thread
From: martin rudalics @ 2021-06-11  8:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

I think I fixed most of the problems you found.  Please have a look when
you have time.

Thanks, martin



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: master b3dd0ce: Provide new option `delete-window-set-selected'
  2021-06-11  8:07     ` martin rudalics
@ 2021-06-11 11:56       ` Eli Zaretskii
  0 siblings, 0 replies; 5+ messages in thread
From: Eli Zaretskii @ 2021-06-11 11:56 UTC (permalink / raw)
  To: martin rudalics; +Cc: emacs-devel

> Cc: emacs-devel@gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 11 Jun 2021 10:07:11 +0200
> 
> I think I fixed most of the problems you found.  Please have a look when
> you have time.

Thanks, I'm happy now.



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-11 11:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10  8:37 master b3dd0ce: Provide new option `delete-window-set-selected' Eli Zaretskii
2021-06-10 15:01 ` martin rudalics
2021-06-10 15:29   ` Eli Zaretskii
2021-06-11  8:07     ` martin rudalics
2021-06-11 11:56       ` Eli Zaretskii

unofficial mirror of emacs-devel@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-devel/0 emacs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-devel emacs-devel/ https://yhetil.org/emacs-devel \
		emacs-devel@gnu.org
	public-inbox-index emacs-devel

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.devel
	nntp://news.gmane.io/gmane.emacs.devel


code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git