simplifying windmove-frame-edges

simplifying windmove-frame-edges

[Juanma Barranquero]

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

A few years ago (well, fifteen, really), `windmove-frame-edges' was changed
with this commit

commit 82ae2f3f78fca3b1932e60fbb4bb1fa050cea6db
 Author: Eli Zaretskii <eliz@gnu.org>
 Date:   2004-09-13 20:08:44 +0000

     (windmove-frame-edges): Report coordinates of
     outside edges of frame, not inside edges.
     (windmove-coordinates-of-position): Convert into wrapper to new
     function `windmove-coordinates-of-window-position';
     `compute-motion' always applies to selected window.
     (windmove-coordinates-of-position): Update documentation to refer
     to Emacs 21 Lisp Reference Manual.
     (windmove-find-other-window): Fix off-by-one errors for max x,y.

The relevant change to `windmove-frame-edges' is this:

 --- a/lisp/windmove.el
 +++ b/lisp/windmove.el
 @@ -324,11 +324,11 @@ windmove-frame-edges
    (let* ((frame (if window
                     (window-frame window)
                   (selected-frame)))
 -        (top-left (window-inside-edges (frame-first-window frame)))
 +        (top-left (window-edges (frame-first-window frame)))
          (x-min (nth 0 top-left))
          (y-min (nth 1 top-left))
 -        (x-max (+ x-min (frame-width frame) -1)) ; 1- for last row & col
 -        (y-max (+ x-max (frame-height frame) -1)))
 +        (x-max (1- (frame-width frame))) ; 1- for last row & col
 +        (y-max (1- (frame-height frame))))
      (list x-min y-min x-max y-max)))

  ;; it turns out that constraining is always a good thing, even when


But, as it is now,  I don't see the difference between

(defun windmove-frame-edges (window)
  "Return (X-MIN Y-MIN X-MAX Y-MAX) for the frame containing WINDOW.
If WINDOW is nil, return the edges for the selected frame.
\(X-MIN, Y-MIN) is the zero-based coordinate of the top-left corner
of the frame; (X-MAX, Y-MAX) is the zero-based coordinate of the
bottom-right corner of the frame.
For example, if a frame has 76 rows and 181 columns, the return value
from `windmove-frame-edges' will be the list (0 0 180 75)."
  (let* ((frame (if window
                    (window-frame window)
                  (selected-frame)))
         (top-left (window-edges (frame-first-window frame)))
         (x-min (nth 0 top-left))
         (y-min (nth 1 top-left))
         (x-max (1- (frame-width frame))) ; 1- for last row & col
         (y-max (1- (frame-height frame))))
    (list x-min y-min x-max y-max)))

and just defining it as

(defun windmove-frame-edges (window)
  "..."
  (window-edges (frame-root-window window)))

[-- Attachment #2: Type: text/html, Size: 3091 bytes --]

Re: simplifying windmove-frame-edges

[martin rudalics]

 > and just defining it as
 >
 > (defun windmove-frame-edges (window)
 >    "..."
 >    (window-edges (frame-root-window window)))

FWIW 'windmove-frame-edges' and all functions in windmove.el defined
before 'windmove-find-other-window' are no more used ever since the
latter calls 'window-in-direction'.

martin

Re: simplifying windmove-frame-edges

[Juanma Barranquero]

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

On Wed, Oct 23, 2019 at 6:05 PM martin rudalics <rudalics@gmx.at> wrote:

> FWIW 'windmove-frame-edges' and all functions in windmove.el defined
> before 'windmove-find-other-window' are no more used ever since the
> latter calls 'window-in-direction'.

I see.

Why not remove (or, at the very least, comment out) all these functions,
then? Even if they're part of the "published interface" of windmove.el,
they are quite clearly internal implementation functions. I don't think any
package out there is calling `windmove-reference-loc' because it just
happened to be *the* function it needed.

[-- Attachment #2: Type: text/html, Size: 793 bytes --]

Re: simplifying windmove-frame-edges

[martin rudalics]

 > Why not remove (or, at the very least, comment out) all these functions,
 > then? Even if they're part of the "published interface" of windmove.el,
 > they are quite clearly internal implementation functions. I don't think any
 > package out there is calling `windmove-reference-loc' because it just
 > happened to be *the* function it needed.

With 'window-in-direction' practically the entire code in windmove.el
had become obsolete, so we could have easily removed it back then.
Later on Juri added directional selection and deletion.  So go ahead
and remove whatever you see fit (unless Eli objects) ...

martin

Re: simplifying windmove-frame-edges

[Phil Sainty]

On 24/10/19 7:10 AM, Juanma Barranquero wrote:
> I don't think any package out there is calling
> `windmove-reference-loc' because it just happened to be *the*
> function it needed.

Just to save anyone else from checking framemove.el, that only
touches `windmove-do-window-select', which is not one of the
deprecated functions; so that's not a problem.

https://www.emacswiki.org/emacs/framemove.el

Re: simplifying windmove-frame-edges

[Juanma Barranquero]

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

On Thu, Oct 24, 2019 at 11:13 AM Phil Sainty <psainty@orcon.net.nz> wrote:

> Just to save anyone else from checking framemove.el, that only
> touches `windmove-do-window-select', which is not one of the
> deprecated functions; so that's not a problem.

Thanks.

It makes sense that some package uses windmove-do-window-select, which is
the workhorse of windmove.el. The functions above
windmove-find-other-window are just ancillary code, and code that *does not
work* currently, that being the reason Martin rewrote
windmove-find-other-window in the first place.

- These are small and do trivial work:
   windmove-coord-add
   windmove-constraint-to-range
   windmove-constraint-around-range

- This one could be useful, but, as noted, it's just equivalent to
(window-edges (frame-root-window arg)):
  windmove-frame-edges

- And these are very specific of windmove, and also, according to Martin's
comment, "after the pixelwise change the old approach didn't work any more":
  windmove-constrain-loc-for-movement
  windmove-wrap-loc-for-movement
  windmove-reference-loc
  windmove-other-window-loc

So at the very least they should be made obsolete, though I don't really
see the point of keeping that unused code in the trunk.

[-- Attachment #2: Type: text/html, Size: 1504 bytes --]

Re: simplifying windmove-frame-edges

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 24 Oct 2019 08:44:51 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
>  > Why not remove (or, at the very least, comment out) all these functions,
>  > then? Even if they're part of the "published interface" of windmove.el,
>  > they are quite clearly internal implementation functions. I don't think any
>  > package out there is calling `windmove-reference-loc' because it just
>  > happened to be *the* function it needed.
> 
> With 'window-in-direction' practically the entire code in windmove.el
> had become obsolete, so we could have easily removed it back then.
> Later on Juri added directional selection and deletion.  So go ahead
> and remove whatever you see fit (unless Eli objects) ...

We cannot just delete them, we need to obsolete them first.  They were
with us for far too long.

Re: simplifying windmove-frame-edges

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 399 bytes --]

On Thu, Oct 24, 2019 at 4:11 PM Eli Zaretskii <eliz@gnu.org> wrote:

> We cannot just delete them, we need to obsolete them first.  They were
> with us for far too long.

I find the argument less than compelling, given the fact that the functions
have been known not to work well in some cases for the past six years or so.

But you're the boss.

Here's the patch, split in three parts for clarity.

[-- Attachment #1.2: Type: text/html, Size: 562 bytes --]

[-- Attachment #2: 0001-windmove.el-Obsolete-code-not-used-since-2013.patch --]
[-- Type: application/x-patch, Size: 4031 bytes --]

[-- Attachment #3: 0002-windmove.el-Fix-docstrings-not-to-refer-to-obsolete-.patch --]
[-- Type: application/x-patch, Size: 1975 bytes --]

[-- Attachment #4: 0003-windmove.el-Remove-comments-about-the-old-implementa.patch --]
[-- Type: application/x-patch, Size: 11568 bytes --]

Re: simplifying windmove-frame-edges

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 25 Oct 2019 14:17:23 +0200
> Cc: martin rudalics <rudalics@gmx.at>, Emacs developers <emacs-devel@gnu.org>
> 
> > We cannot just delete them, we need to obsolete them first.  They were
> > with us for far too long.
> 
> I find the argument less than compelling, given the fact that the functions have been known not to work well in
> some cases for the past six years or so.

From bitter experience, we have no good ways of knowing which APIs are
used by unbundled packages and user code out there.  Emacs is a stable
package, so users rightfully expect it not to break their code by
backward-incompatible changes.

> --- a/lisp/windmove.el
> +++ b/lisp/windmove.el
> @@ -473,15 +473,19 @@ windmove-other-window-loc
>  ;; Rewritten on 2013-12-13 using `window-in-direction'.  After the
>  ;; pixelwise change the old approach didn't work any more.  martin
>  (defun windmove-find-other-window (dir &optional arg window)
> -  "Return the window object in direction DIR.
> -DIR, ARG, and WINDOW are handled as by `windmove-other-window-loc'."
> +  "Return the window object in direction DIR as seen from WINDOW.
> +DIR is one of `left', `up', `right', or `down'.
> +Optional ARG, if negative, means to use the right or bottom edge of
> +WINDOW as reference position, instead of `window-point'; if positive,
> +use the left or top edge of WINDOW as reference point.
> +WINDOW must be a live window and defaults to the selected one."
>    (window-in-direction dir window nil arg windmove-wrap-around t))

The last sentence should be the 2nd, because you mention WINDOW in the
first sentence of the doc string.

> From 1e7b2fe06a4fce4dc2ce52b037145190d86176ca Mon Sep 17 00:00:00 2001
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 25 Oct 2019 14:02:22 +0200
> Subject: [PATCH 3/3] windmove.el: Remove comments about the old implementation

Are these comments really no longer relevant?

Thanks.

Re: simplifying windmove-frame-edges

[Juanma Barranquero]

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

On Fri, Oct 25, 2019 at 2:51 PM Eli Zaretskii <eliz@gnu.org> wrote:

> The last sentence should be the 2nd, because you mention WINDOW in the
> first sentence of the doc string.

Ok.

> Are these comments really no longer relevant?

They are relevant for the obsolete code, but they are misleading because
they describe an implementation that's no longer used.

I don't think we want to spend time fixing these functions, or any bugs in
them, and we don't want to encourage *more* unbundled packages or user code
using these functions. It's better to strip the comment, put a notice, and
let it be until we feel it's safe to just remove it all.

[-- Attachment #2: Type: text/html, Size: 824 bytes --]

Re: simplifying windmove-frame-edges

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 25 Oct 2019 15:13:45 +0200
> Cc: martin rudalics <rudalics@gmx.at>, Emacs developers <emacs-devel@gnu.org>
> 
> > Are these comments really no longer relevant?
> 
> They are relevant for the obsolete code, but they are misleading because they describe an implementation
> that's no longer used.
> 
> I don't think we want to spend time fixing these functions, or any bugs in them, and we don't want to
> encourage *more* unbundled packages or user code using these functions. It's better to strip the comment,
> put a notice, and let it be until we feel it's safe to just remove it all.

So you are saying that nothing in those comments is relevant for the
implementation of non-obsolete functions?  IOW, no reasonable amount
of changes in the comments will ever yield anything useful for the
code we now consider the mainstream?  If so, then yes, removing those
comments is TRT.

Thanks.

Re: simplifying windmove-frame-edges

[Juanma Barranquero]

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

On Fri, Oct 25, 2019 at 3:43 PM Eli Zaretskii <eliz@gnu.org> wrote:

> So you are saying that nothing in those comments is relevant for the
> implementation of non-obsolete functions?  IOW, no reasonable amount
> of changes in the comments will ever yield anything useful for the
> code we now consider the mainstream?  If so, then yes, removing those
> comments is TRT.

Yes, I think so. The only relevant info in that big "implementation note"
fragment is a duplicate of the comment at the beginning of the file. The
rest are just details about how to find the window, or talk about window
and frame coordinates. Neither of those is relevant with the
`window-in-direction' implementation.

[-- Attachment #2: Type: text/html, Size: 850 bytes --]