[PATCH] Fix `window-at' and `coordinates-in-window-p'

[PATCH] Fix `window-at' and `coordinates-in-window-p'

[Lőrentey Károly]

[-- Attachment #1.1.1: Type: text/plain, Size: 982 bytes --]

Try this under X:

    emacs -q --no-site-file --internal-border 1
                            ^^^^^^^^^^^^^^^^^^^
	M-: (window-at 0 0) <RET>
        M-: (coordinates-in-window-p '(0 . 0) (selected-window)) <RET>

Under Emacs 21.3.1, both `window-at' and `coordinates-in-window-p'
ignore the frame's internal border, and (IMHO) correctly return
non-nil values; under the current CVS version, they both return nil.

In CVS, the following functions always return nil if the
internal-border-width frame parameter is non-zero.  I think that is wrong.

	(defun test-coord-1 (window)
          (let ((edges (window-edges window)))
            (eq window (window-at (nth 0 edges) (nth 1 edges)))))

	(defun test-coord-2 (window)
          (let ((edges (window-edges window)))
	    (coordinates-in-window-p (cons (nth 0 edges)
					   (nth 1 edges))
				     window)))

This bug confuses the windmove.el package and renders it almost
unusable for me.  I fixed it with the following patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: Type: text/x-patch, Size: 4016 bytes --]

*** orig/src/frame.h
--- mod/src/frame.h
***************
*** 880,919 ****
     canonical char width is to be used.  X must be a Lisp integer or
     float.  Value is a C integer.  */
  
! #define FRAME_PIXEL_X_FROM_CANON_X(F, X)		\
!      (INTEGERP (X)					\
!       ? XINT (X) * FRAME_COLUMN_WIDTH (F)		\
!       : (int) (XFLOAT_DATA (X) * FRAME_COLUMN_WIDTH (F)))
  
  /* Convert canonical value Y to pixels.  F is the frame whose
     canonical character height is to be used.  X must be a Lisp integer
     or float.  Value is a C integer.  */
  
  #define FRAME_PIXEL_Y_FROM_CANON_Y(F, Y)		\
!      (INTEGERP (Y)					\
!       ? XINT (Y) * FRAME_LINE_HEIGHT (F)		\
!       : (int) (XFLOAT_DATA (Y) * FRAME_LINE_HEIGHT (F)))
  
  /* Convert pixel-value X to canonical units.  F is the frame whose
     canonical character width is to be used.  X is a C integer.  Result
     is a Lisp float if X is not a multiple of the canon width,
     otherwise it's a Lisp integer.  */
  
! #define FRAME_CANON_X_FROM_PIXEL_X(F, X)			\
!      ((X) % FRAME_COLUMN_WIDTH (F) != 0				\
!       ? make_float ((double) (X) / FRAME_COLUMN_WIDTH (F))	\
!       : make_number ((X) / FRAME_COLUMN_WIDTH (F)))
  
  /* Convert pixel-value Y to canonical units.  F is the frame whose
     canonical character height is to be used.  Y is a C integer.
     Result is a Lisp float if Y is not a multiple of the canon width,
     otherwise it's a Lisp integer.  */
  
! #define FRAME_CANON_Y_FROM_PIXEL_Y(F, Y)			\
!      ((Y) % FRAME_LINE_HEIGHT (F) 				\
!       ? make_float ((double) (Y) / FRAME_LINE_HEIGHT (F))	\
!       : make_number ((Y) / FRAME_LINE_HEIGHT (F)))
! 
  
  \f
  /* Manipulating pixel sizes and character sizes.
--- 880,924 ----
     canonical char width is to be used.  X must be a Lisp integer or
     float.  Value is a C integer.  */
  
! #define FRAME_PIXEL_X_FROM_CANON_X(F, X)                 \
!   (FRAME_INTERNAL_BORDER_WIDTH (F) +                     \
!    (INTEGERP (X)                                         \
!     ? XINT (X) * FRAME_COLUMN_WIDTH (F)                  \
!     : (int) (XFLOAT_DATA (X) * FRAME_COLUMN_WIDTH (F))))
  
  /* Convert canonical value Y to pixels.  F is the frame whose
     canonical character height is to be used.  X must be a Lisp integer
     or float.  Value is a C integer.  */
  
  #define FRAME_PIXEL_Y_FROM_CANON_Y(F, Y)		\
!   (FRAME_INTERNAL_BORDER_WIDTH (F) +                    \
!    (INTEGERP (Y)                                        \
!     ? XINT (Y) * FRAME_LINE_HEIGHT (F)                  \
!     : (int) (XFLOAT_DATA (Y) * FRAME_LINE_HEIGHT (F))))
  
  /* Convert pixel-value X to canonical units.  F is the frame whose
     canonical character width is to be used.  X is a C integer.  Result
     is a Lisp float if X is not a multiple of the canon width,
     otherwise it's a Lisp integer.  */
  
! #define FRAME_CANON_X_FROM_PIXEL_X(F, X)                                \
!   ((X - FRAME_INTERNAL_BORDER_WIDTH (F)) % FRAME_COLUMN_WIDTH (F) != 0  \
!    ? make_float ((double) (X - FRAME_INTERNAL_BORDER_WIDTH (F)) /       \
!                  FRAME_COLUMN_WIDTH (F))                                \
!    : make_number ((X - FRAME_INTERNAL_BORDER_WIDTH (F)) /               \
!                   FRAME_COLUMN_WIDTH (F)))
  
  /* Convert pixel-value Y to canonical units.  F is the frame whose
     canonical character height is to be used.  Y is a C integer.
     Result is a Lisp float if Y is not a multiple of the canon width,
     otherwise it's a Lisp integer.  */
  
! #define FRAME_CANON_Y_FROM_PIXEL_Y(F, Y)                               \
!   ((Y - FRAME_INTERNAL_BORDER_WIDTH (F)) % FRAME_LINE_HEIGHT (F) != 0  \
!    ? make_float ((double) (Y - FRAME_INTERNAL_BORDER_WIDTH (F)) /      \
!                  FRAME_LINE_HEIGHT (F))                                \
!    : make_number ((Y - FRAME_INTERNAL_BORDER_WIDTH (F)) /              \
!                   FRAME_LINE_HEIGHT (F)))
  
  \f
  /* Manipulating pixel sizes and character sizes.

[-- Attachment #1.1.3: Type: text/plain, Size: 313 bytes --]


2004-07-23  Károly Lőrentey  <lorentey@elte.hu>

	* frame.h (FRAME_PIXEL_X_FROM_CANON_X, FRAME_PIXEL_Y_FROM_CANON_Y)
	(FRAME_CANON_X_FROM_PIXEL_X, FRAME_CANON_Y_FROM_PIXEL_Y):  Take
	the frame's internal border width into account.

Arch changeset (from lorentey@elte.hu--2004/emacs--lorentey--patch-67):

[-- Attachment #1.1.4: fix-coords.cset.tar.gz --]
[-- Type: application/octet-stream, Size: 1802 bytes --]

[-- Attachment #1.1.5: Type: text/plain, Size: 15 bytes --]


-- 
Károly

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Luc Teirlinck]

Your patch certainly makes the behavior of `window-at' and
`coordinates-in-window-p' consistent with their docstrings and their
documentation in `(elisp)Coordinates and Windows'.  In my opinion, the
current behavior is not.  I like your patch.  We will have to wait to
see what Richard's opinion is, however.

Sincerely,

Luc.

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Eli Zaretskii]

> From: lorentey@elte.hu (=?iso-8859-2?Q?L=F5rentey_K=E1roly?=)
> Date: Sat, 24 Jul 2004 00:29:47 +0200
> 
> This bug confuses the windmove.el package and renders it almost
> unusable for me.  I fixed it with the following patch:

Thanks.

Did you try this on a character terminal?  Please test both by running
Emacs with "emacs -nw" and by configuring and building Emacs with the
"--without-x" configure option.

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Lőrentey Károly]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> This bug confuses the windmove.el package and renders it almost
>> unusable for me.  I fixed it with the following patch:
>
> Did you try this on a character terminal?  Please test both by running
> Emacs with "emacs -nw" and by configuring and building Emacs with the
> "--without-x" configure option.

Both of my patches work fine without X; struct frame's
internal_border_width is always defined, and defaults to zero.

-- 
Károly

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Luc Teirlinck]

After taking a closer look at your patch, I guess one would have to be
careful that there are no functions calling the functions you changed
that _already_ adjust for FRAME_INTERNAL_BORDER_WIDTH, in which case
they will double-adjust after the patch.  I did not find any, but I
have not looked that carefully.

I guess an alternative would be to make the adjustment inside
`window-at' and `coordinates-in-window-p'.

A patch that Richard sent me and that does this for `window-at'
strangely enough seems to fail by making `window-at' return nil
everywhere:

*** window.c    20 Jul 2004 16:52:30 -0400      1.471
--- window.c    23 Jul 2004 20:05:29 -0400
***************
*** 937,944 ****
    CHECK_NUMBER_OR_FLOAT (y);

    return window_from_coordinates (f,
!                                 FRAME_PIXEL_X_FROM_CANON_X (f, x),
!                                 FRAME_PIXEL_Y_FROM_CANON_Y (f, y),
                                  0, 0, 0, 0);
  }

--- 937,946 ----
    CHECK_NUMBER_OR_FLOAT (y);

    return window_from_coordinates (f,
!                                 (FRAME_PIXEL_X_FROM_CANON_X (f, x)
!                                  + FRAME_INTERNAL_BORDER_WIDTH (f)),
!                                 (FRAME_PIXEL_Y_FROM_CANON_Y (f, y)
!                                  + FRAME_INTERNAL_BORDER_WIDTH (f)),
                                  0, 0, 0, 0);
  }

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Lőrentey Károly]

[-- Attachment #1.1.1: Type: text/plain, Size: 1367 bytes --]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:
> After taking a closer look at your patch, I guess one would have to be
> careful that there are no functions calling the functions you changed
> that _already_ adjust for FRAME_INTERNAL_BORDER_WIDTH, in which case
> they will double-adjust after the patch.  I did not find any, but I
> have not looked that carefully.

I did a grep in the source tree, and it seems these macros are not
used anywhere else in Emacs, except for one small reference in
Fwindow_vscroll:

FRAME_PIXEL_X_FROM_CANON_X:
	src/window.c:788: Fcoordinates_in_window_p
	src/window.c:943: Fwindow_at

FRAME_PIXEL_Y_FROM_CANON_Y:
        src/window.c:789: Fcoordinates_in_window_p
        src/window.c:944: Fwindow_at

FRAME_CANON_X_FROM_PIXEL_X:
	src/window.c:799: Fcoordinates_in_window_p

FRAME_CANON_Y_FROM_PIXEL_Y:
	src/window.c:800: Fcoordinates_in_window_p
	src/window.c:6167: Fwindow_vscroll

Unfortunately, `window-vscroll' doesn't like my patch:

	(set-frame-parameter nil 'internal-border-width 20)
	(set-window-vscroll nil 0.2) ==> -1.13, should be around 0.2
	(window-vscroll) ==> -1.13, should be around 0.2

> I guess an alternative would be to make the adjustment inside
> `window-at' and `coordinates-in-window-p'.

I think it's easier to adjust the pixel coordinates right inside
coordinates_in_window_p, which both functions use:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: Type: text/x-patch, Size: 383 bytes --]

*** orig/src/window.c
--- mod/src/window.c
***************
*** 607,612 ****
--- 607,616 ----
    int grabbable_width = ux;
    int lmargin_width, rmargin_width, text_left, text_right;
  
+   /* Adjust for the frame's internal border. */
+   *x += FRAME_INTERNAL_BORDER_WIDTH (f);
+   *y += FRAME_INTERNAL_BORDER_WIDTH (f);
+ 
    if (*x < x0 || *x >= x1)
      return ON_NOTHING;
  

[-- Attachment #1.1.3: Type: text/plain, Size: 223 bytes --]


2004-07-24  Károly Lőrentey  <lorentey@elte.hu>

	* window.c (coordinates_in_window): Adjust pixel coordinates to
	skip the frame's internal border.

Changeset: lorentey@elte.hu--2004/emacs--lorentey--0--patch-70


[-- Attachment #1.1.4: fix-coords-2.cset.tar.gz --]
[-- Type: application/octet-stream, Size: 1306 bytes --]

[-- Attachment #1.1.5: Type: text/plain, Size: 1547 bytes --]


This three-line patch fixes all problems with `window-at' and
`coordinates-in-window-p', and as far as I can see it does not affect
any other part of Emacs.

Yet another way to fix the problem would be to change the window-edges
family of functions to return ugly floats instead of nice round
integers, but on the Lisp level I think it's better to have the
frame's origin inside the frame's border.  I like the above patch the
most.

> A patch that Richard sent me and that does this for `window-at'
> strangely enough seems to fail by making `window-at' return nil
> everywhere:

This is strange, as the two patches look to be equivalent.

> *** window.c    20 Jul 2004 16:52:30 -0400      1.471
> --- window.c    23 Jul 2004 20:05:29 -0400
> ***************
> *** 937,944 ****
>     CHECK_NUMBER_OR_FLOAT (y);
>
>     return window_from_coordinates (f,
> !                                 FRAME_PIXEL_X_FROM_CANON_X (f, x),
> !                                 FRAME_PIXEL_Y_FROM_CANON_Y (f, y),
>                                   0, 0, 0, 0);
>   }
>
> --- 937,946 ----
>     CHECK_NUMBER_OR_FLOAT (y);
>
>     return window_from_coordinates (f,
> !                                 (FRAME_PIXEL_X_FROM_CANON_X (f, x)
> !                                  + FRAME_INTERNAL_BORDER_WIDTH (f)),
> !                                 (FRAME_PIXEL_Y_FROM_CANON_Y (f, y)
> !                                  + FRAME_INTERNAL_BORDER_WIDTH (f)),
>                                   0, 0, 0, 0);
>   }

-- 
Károly

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Luc Teirlinck]

   > A patch that Richard sent me and that does this for `window-at'
   > strangely enough seems to fail by making `window-at' return nil
   > everywhere:

   This is strange, as the two patches look to be equivalent.

I must have done _something_ wrong.  After rechecking, `window-at'
works fine with Richard's patch.  (But not `coordinates-in-window-p').
Note that Richard already installed his patch in CVS.

Sincerely,

Luc.

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Lőrentey Károly]

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

Luc Teirlinck <teirllm@dms.auburn.edu> writes:
> Note that Richard already installed his patch in CVS.

Ah; I didn't see it because it hasn't propagated to Arch yet.  I
looked it up in Savannah's viewCVS interface, and I am happy with
Richard's fix.  Thanks for the note!

-- 
Károly

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Richard Stallman]

    I think it's easier to adjust the pixel coordinates right inside
    coordinates_in_window_p, which both functions use:

This is not clean, because it gives the arguments of
coordinates_in_window_p a peculiar meaning (and doesn't document it,
by the way).  It is much better to keep their meanings consistent
with the rest of Emacs.

I will fix the other call.

Meanwhile, I fixed the other reported bug in coordinates-in-window-p.

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Lőrentey Károly]

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

Richard Stallman <rms@gnu.org> writes:
>     I think it's easier to adjust the pixel coordinates right inside
>     coordinates_in_window_p, which both functions use:
>
> This is not clean, because it gives the arguments of
> coordinates_in_window_p a peculiar meaning (and doesn't document it,
> by the way).  It is much better to keep their meanings consistent
> with the rest of Emacs.

OK.

> I will fix the other call.

If you mean the one in window-vscroll, I don't think there are
problems with that in CVS.  (It uses the macros for converting
relative lengths, as intended.  My patch would have broken that.)

> Meanwhile, I fixed the other reported bug in coordinates-in-window-p.

Great! :-)

-- 
Károly

[-- Attachment #1.2: Type: application/pgp-signature, Size: 188 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [PATCH] Fix `window-at' and `coordinates-in-window-p'

[Richard Stallman]

The comments define FRAME_CANON_X_FROM_PIXEL_X and friends
as converting units, which means that they should not offset
by the internal border.  That change might be wrong for other
uses of these macros.  That's why I fixed this case by
doing that in the code that uses the macros.

However, other uses of the same macros could have similar
bugs.  Could you look at the other uses of these macros?