bug#16017: 24.3.50; windmove is broken

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

windmove-right and windmove-down don't do anything now.

In GNU Emacs 24.3.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.8.6)
 of 2013-12-01 on axl
Bzr revision: 115317 eliz@gnu.org-20131130191221-9pxtv2v0eilgek8u
Windowing system distributor `The X.Org Foundation', version 11.0.11403000
System Description:	Ubuntu 13.10

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

 > windmove-right and windmove-down don't do anything now.

As you mentioned elsewhere, this should be a consequence of the
pixelwise change.  Can you try to find out why it fails?

In `window-in-direction' I now use `window-pixel-edges' instead of
`window-edges'.  Would doing the same in windmove.el fix it?

martin

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 02.12.2013 09:38, martin rudalics wrote:
> In `window-in-direction' I now use `window-pixel-edges' instead of
> `window-edges'.  Would doing the same in windmove.el fix it?

It doesn't. If I replace both occurrences of `window-edges' with 
`window-pixel-edges', the only windmove command that works is 
`windmove-down'. I've also tried replacing `window-inside-edges' with 
`window-inside-pixel-edges' and `posn-col-row' with `posn-x-y'.

What does help, is changing `windmove-window-distance-delta' from 1 to 
2. So the problem is likely to do with off-by-one errors in `window-edges'.

bug#16017: 24.3.50; windmove is broken

[Stefan Monnier]

>> In `window-in-direction' I now use `window-pixel-edges' instead of
>> `window-edges'.  Would doing the same in windmove.el fix it?

BTW, shouldn't windmove use window-in-direction?


        Stefan

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

> BTW, shouldn't windmove use window-in-direction?

It could but IIRC there are some differences.  I'll look into it.

martin

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

 > What does help, is changing `windmove-window-distance-delta' from 1 to
 > 2. So the problem is likely to do with off-by-one errors in `window-edges'.

In a sense, yes.  When I assign columns and lines from pixel sizes I
have to round sometimes.  IIRC I tried to make the sums and the adjacent
edges fit but maybe I failed or there's another issue involved.  I'll
look into this as soon as I sorted out bug 16013 (unless you wanted to
have a look).

martin

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 02.12.2013 20:16, martin rudalics wrote:
> I'll into this as soon as I sorted out bug 16013 (unless you wanted to
> have a look).

Please do. I'm sure you understand the problem area better, and there 
are other issues I'd rather devote attention to.

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

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

 >> I'll into this as soon as I sorted out bug 16013 (unless you wanted to
 >> have a look).
 >
 > Please do. I'm sure you understand the problem area better, and there
 > are other issues I'd rather devote attention to.

IIUC the bug happens only in a window whose subwindow does not have an
integral number of lines like in a maximized frame.  Is that correct?

Here on a maximized frame with two windows above each other and the
upper one selected the following happens

(window-height) => 30
(window-pixel-height) => 484

so the selected window has 484 / 16 lines which is larger than 30.

Now `windmove-other-window-loc' with DIR 'down does

             (+ (1- (nth 3 edges))
                windmove-window-distance-delta))) ; (x, y1+d-1)

and returns 30.  `window-at' returns the selected window since that
window extends until after line 30 and windmove doesn't move because the
target window is the selected one.

In the long run I'd like to use `window-in-direction' here because it
doesn't suffer this problem. Meanwhile I can offer the attached patch
(although I'm not sure whether it is really better than customizing
`windmove-window-distance-delta').

martin


BTW: If you want to test `window-in-direction' directly you can try the
following instead:

(defun select-window-on-left ()
   "Select window on the left."
   (interactive)
   (select-window (window-in-direction 'left)))

(defun select-window-above ()
   "Select window above."
   (interactive)
   (select-window (window-in-direction 'above)))

(defun select-window-on-right ()
   "Select window on the right."
   (interactive)
   (select-window (window-in-direction 'right)))

(defun select-window-below ()
   "Select window below."
   (interactive)
   (select-window (window-in-direction 'below)))

(defun select-window-keybindings (&optional modifier)
   "Set up keybindings as in windmove."
   (interactive)
   (unless modifier (setq modifier 'shift))
   (global-set-key (vector (list modifier 'left))  'select-window-on-left)
   (global-set-key (vector (list modifier 'right)) 'select-window-on-right)
   (global-set-key (vector (list modifier 'up))    'select-window-above)
   (global-set-key (vector (list modifier 'down))  'select-window-below))

[-- Attachment #2: windmove.diff --]
[-- Type: text/plain, Size: 1642 bytes --]

=== modified file 'lisp/windmove.el'
--- lisp/windmove.el	2013-01-01 09:11:05 +0000
+++ lisp/windmove.el	2013-12-03 10:19:43 +0000
@@ -438,24 +438,28 @@
 to.  DIR is one of `left', `up', `right', or `down'; an optional ARG
 is handled as by `windmove-reference-loc'; WINDOW is the window that
 movement is relative to."
-  (let ((edges (window-edges window))   ; edges: (x0, y0, x1, y1)
+  (let ((edges (window-pixel-edges window))   ; edges: (x0, y0, x1, y1)
         (refpoint (windmove-reference-loc arg window))) ; (x . y)
     (cond
      ((eq dir 'left)
-      (cons (- (nth 0 edges)
+      (cons (- (ceiling (nth 0 edges)
+			(frame-char-width (window-frame window)))
                windmove-window-distance-delta)
             (cdr refpoint)))            ; (x0-d, y)
      ((eq dir 'up)
       (cons (car refpoint)
-            (- (nth 1 edges)
+            (- (ceiling (nth 1 edges)
+			(frame-char-height (window-frame window)))
                windmove-window-distance-delta))) ; (x, y0-d)
      ((eq dir 'right)
-      (cons (+ (1- (nth 2 edges))	; -1 to get actual max x
+      (cons (+ (1- (ceiling (nth 2 edges)
+			    (frame-char-width (window-frame window))))	; -1 to get actual max x
                windmove-window-distance-delta)
             (cdr refpoint)))            ; (x1+d-1, y)
      ((eq dir 'down)			; -1 to get actual max y
       (cons (car refpoint)
-            (+ (1- (nth 3 edges))
+            (+ (1- (ceiling (nth 3 edges)
+			    (frame-char-height (window-frame window))))
                windmove-window-distance-delta))) ; (x, y1+d-1)
      (t (error "Invalid direction of movement: %s" dir)))))

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 03.12.2013 13:44, martin rudalics wrote:
> IIUC the bug happens only in a window whose subwindow does not have an
> integral number of lines like in a maximized frame.  Is that correct?

Seems so. On my machine, the problem is there when the window is 
maximized (and the frame has non-integral number of lines), but 
disappears when Emacs is in fullscreen mode, without window borders and 
everything (then the frame height looks evenly divisible by line height).

> In the long run I'd like to use `window-in-direction' here because it
> doesn't suffer this problem. Meanwhile I can offer the attached patch
> (although I'm not sure whether it is really better than customizing
> `windmove-window-distance-delta').

The patch works, except it doesn't let me move to the minibuffer window 
when it's active, and doesn't say so when the minibuffer is inactive 
(and it should, according to `windmove-do-window-select'). Changing the 
value of `windmove-window-distance-delta' doesn't help with it either, 
so looks like the patch is indeed equivalent to that.

> BTW: If you want to test `window-in-direction' directly you can try the
> following instead:

Thank you, this is better. The minibuffer is selectable this way, and 
other cases of window navigation work, too.

The "no window there" feedback is non-existent (error "Wrong type 
argument: window-live-p, nil"), but that's to be expected, I guess.

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

 > The patch works, except it doesn't let me move to the minibuffer window
 > when it's active, and doesn't say so when the minibuffer is inactive
 > (and it should, according to `windmove-do-window-select'). Changing the
 > value of `windmove-window-distance-delta' doesn't help with it either,
 > so looks like the patch is indeed equivalent to that.

I nevertheless checked it in to allow others basic navigation.  I'll
look into the minibuffer case later.

 > The "no window there" feedback is non-existent (error "Wrong type
 > argument: window-live-p, nil"), but that's to be expected, I guess.

Certainly not (the function I use was too complex to post here): Try

(defun select-window-on-left ()
   "Select window on the left."
   (interactive)
   (let ((window (window-in-direction 'left)))
     (when window (select-window window))))

martin

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 04.12.2013 16:31, martin rudalics wrote:
> I nevertheless checked it in to allow others basic navigation.  I'll
> look into the minibuffer case later.

I thought you might go ahead and just port windmove to use 
`window-in-direction'.

> Certainly not (the function I use was too complex to post here): Try

Thanks, but I didn't mean I couldn't fix it myself. Rather that I didn't 
expect much error-handling from those short snippets.

They work well, by the way. winner doesn't do anything essential that 
those commands don't do, as far as I can see.

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

 > I thought you might go ahead and just port windmove to use
 > `window-in-direction'.

windmove has an extra ARG to distinguish the upper left corner of a
window and its position of point as reference points.  I can't easily
dismiss that.

 >> Certainly not (the function I use was too complex to post here): Try
 >
 > Thanks, but I didn't mean I couldn't fix it myself. Rather that I didn't
 > expect much error-handling from those short snippets.
 >
 > They work well, by the way. winner doesn't do anything essential that
 > those commands don't do, as far as I can see.

Do you mean winner or windmove?

martin

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 05.12.2013 08:59, martin rudalics wrote:
> windmove has an extra ARG to distinguish the upper left corner of a
> window and its position of point as reference points.  I can't easily
> dismiss that.

Ah, yes, it has. I was not aware.

> Do you mean winner or windmove?

Sorry, windmove, of course.

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

> ..., windmove, of course.

Should work again now including wrapping, prefix argument, and
original error messages.  Please have a look.

Thanks, martin

bug#16017: 24.3.50; windmove is broken

[Dmitry Gutov]

On 13.12.2013 19:23, martin rudalics wrote:
> Should work again now including wrapping, prefix argument, and
> original error messages.  Please have a look.

Thank you, everything seems to work.

The prefix argument handling is weird (`C-u 4' has effect, just `C-u' 
doesn't), but that's likely old behavior.

bug#16017: 24.3.50; windmove is broken

[martin rudalics]

 > The prefix argument handling is weird (`C-u 4' has effect, just `C-u'
 > doesn't), but that's likely old behavior.

IIUC it's explained in the doc-strings as "With no prefix argument, or
with prefix argument equal to zero, ..." so we probably shouldn't change
it.

martin