Re: Bug in CVS Emacs frame positioning under X

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    In the latest CVS Emacs under X, there seems to be a frame positioning bug that
    is timing related.

Does it happen in older CVS versions too?  For instance,
did it happen a month ago?  6 months ago?

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

On 3/22/06, Richard Stallman wrote:
>     In the latest CVS Emacs under X, there seems to be a frame positioning bug that
>     is timing related.
>
> Does it happen in older CVS versions too?  For instance,
> did it happen a month ago?  6 months ago?

Unknown.  I'll check-out the older sources and rebuild.  I'll let you know.
--
Fran Litterio

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

On 3/22/06, I (Fran Litterio) wrote:
> On 3/22/06, Richard Stallman wrote:
> >     In the latest CVS Emacs under X, there seems to be a frame positioning bug that
> >     is timing related.
> >
> > Does it happen in older CVS versions too?  For instance,
> > did it happen a month ago?  6 months ago?
>
> Unknown.  I'll check-out the older sources and rebuild.  I'll let you know.

I have checked out the CVS Emacs source code from October 1, 2005, and
the bug existed at that time.  Additionally, I have found a simple way
to reproduce the problem:

1. Start Emacs under the X Window system using the command: emacs -q

2. Evaluate the following Elisp:

	(dolist (i '(1 2 3 4 5 6))
	  (let ((frame (make-frame '((top . 50) (left . 50)))))
	    (set-frame-position frame 200 200)
	    (set-frame-position frame 300 300)
	    (sit-for 0.5)))

You may have to evaluate the above Elisp more than once to see the
malfunction (although I see it every time).

The symptom is that not all of the frames end up positioned at x/y
coordinates 300/300.  Some frames are positioned at 300+A/300+B, where
A is the width of the left border drawn by the window manager and B is
the height of the top border drawn by the window manager.  Some frames
even end up at 400/400 !

I think I have a patch to src/xterm.c that may fix this.  I will try
it and let you know if the patch works.
--
Fran Litterio

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    The symptom is that not all of the frames end up positioned at x/y
    coordinates 300/300.  Some frames are positioned at 300+A/300+B, where
    A is the width of the left border drawn by the window manager and B is
    the height of the top border drawn by the window manager.  Some frames
    even end up at 400/400 !

    I think I have a patch to src/xterm.c that may fix this.  I will try
    it and let you know if the patch works.

This resembles a problem that I struggled with about 10 years ago.
It might be the same one.

I found that various cases of window positioning were a little bit
off.  So I would fix the case that failed, and then some other case
started to fail.  Eventually I gave up.

With better records, perhaps I could have determined the pattern of
what failed and what succeeded, and figured out an overall solution.
We should start keeping such records now, in case the patch
which fixes this case breaks another case.

Re: Bug in CVS Emacs frame positioning under X

[Jan Djärv]

Richard Stallman wrote:
>     The symptom is that not all of the frames end up positioned at x/y
>     coordinates 300/300.  Some frames are positioned at 300+A/300+B, where
>     A is the width of the left border drawn by the window manager and B is
>     the height of the top border drawn by the window manager.  Some frames
>     even end up at 400/400 !
> 
>     I think I have a patch to src/xterm.c that may fix this.  I will try
>     it and let you know if the patch works.

Please also tell which window managers you tested it on.

> 
> This resembles a problem that I struggled with about 10 years ago.
> It might be the same one.
> 
> I found that various cases of window positioning were a little bit
> off.  So I would fix the case that failed, and then some other case
> started to fail.  Eventually I gave up.

That is the basic problem.  Each window manager does window moves differently, 
and the fact that events may come at different times complicates things.  I 
now try to make it work on a lot of window managers, but I gave up on "all".

> 
> With better records, perhaps I could have determined the pattern of
> what failed and what succeeded, and figured out an overall solution.
> We should start keeping such records now, in case the patch
> which fixes this case breaks another case.


The patch must be tested on at least 10 or so of the most common window 
managers.  Usually it is difficult to make a patch that doesn't break some 
other window manager.

	Jan D.

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

On 3/25/06, Richard Stallman wrote:

>     The symptom is that not all of the frames end up positioned at x/y
>     coordinates 300/300.  Some frames are positioned at 300+A/300+B, where
>     A is the width of the left border drawn by the window manager and B is
>     the height of the top border drawn by the window manager.  Some frames
>     even end up at 400/400 !

> This resembles a problem that I struggled with about 10 years ago.
> It might be the same one.
>
> I found that various cases of window positioning were a little bit
> off.  So I would fix the case that failed, and then some other case
> started to fail.  Eventually I gave up.

My major concern is that the current malfunction has non-deterministic
user-visible effects.  Depending of the X event timing, programmatically
positioned frames can end up in one of three places:

	1. The correct position.

	2. Nearby the correct position (i.e., off by the width and height of the
           window manager decorations).

	3. Very far away from the correct position (i.e., off by hundreds of
           pixels in both the X and Y coordinates).

In cases #2 and #3, the difference between the incorrect position and the
correct position is remembered by the frame and affects all future positioning
of that frame.  In case #3, this can position a frame off-screen even if the
user specifies coordinates that are on-screen!

> With better records, perhaps I could have determined the pattern of
> what failed and what succeeded, and figured out an overall solution.
> We should start keeping such records now, in case the patch
> which fixes this case breaks another case.

The current algorithm in src/xterm.c is close to providing a universal solution,
but it makes one incorrect assumption: that the location of a frame after
calling XMoveWindow() is correctly specified in the first ConfigureNotify event
received after the call to XMoveWindow().  Function x_check_expected_move() uses
the location from the ConfigureNotify event to compensate for frames that are
mispositioned by "type A" window managers.

If there were a function that can be called to synchronously obtain the current
position of a window, then x_check_expected_move() could use that function
instead of relying on possibly incorrect data from the last ConfigureNotify
event.

I don't (yet) know enough about X Window system programming to know if such a
function exists.  If someone does know, please inform me.  If I find a solution,
I'll submit a patch.
--
Francis Litterio
flitterio@gmail.com
http://world.std.com/~franl/
GPG and PGP public keys available on keyservers.

Re: Bug in CVS Emacs frame positioning under X

[Jan Djärv]

Fran Litterio wrote:

> 
> If there were a function that can be called to synchronously obtain the current
> position of a window, then x_check_expected_move() could use that function
> instead of relying on possibly incorrect data from the last ConfigureNotify
> event.
> 
> I don't (yet) know enough about X Window system programming to know if such a
> function exists.  If someone does know, please inform me.  If I find a solution,
> I'll submit a patch.


XSync, XMoveWindow, and then XSync again should make the next ConfigureWindow 
contain the position after move.  XGetGeometry and XGetWindowAttributes should 
do the trick.

	Jan D.

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    My major concern is that the current malfunction has non-deterministic
    user-visible effects.  Depending of the X event timing, programmatically
    positioned frames can end up in one of three places:

It could be that we could solve this by completely reorganizing the
way that code works.  The person who wrote it was not the best
programmer, and I then tried to fix it up with just a partial
understanding of how to use Xt.  Someone who really knows the right
way to use Xt could perhaps do this right, and then it would work.

Let's continue this discussion just on emacs-devel.

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

On 3/29/06, Richard Stallman wrote:
> It could be that we could solve this by completely reorganizing the
> way that code works.  The person who wrote it was not the best
> programmer, and I then tried to fix it up with just a partial
> understanding of how to use Xt.

I can't claim to be an expert at X programming, but I've changed the
frame positioning algorithm in src/xterm.c so that it copes with the
non-deterministic nature of X event timing, and so that it fixes the
frame positioning bug I reported earlier in this thread.  Patches to
src/xterm.h and src/xterm.c are below.

Here's what I did:

First, I tried Jan Djärv's suggestion to call XSync() to force Xlib to
wait for the X server to handle the XMoveWindow() request, but that
failed to make XGetGeometry() see up-to-date position information for
the just-moved frame. Instead, I wrote a new function,
x_sync_with_move() that uses XSync() in a loop to make sure
XGetGeometry() sees up-to-date position information after a frame is
moved.

Frankly, this is not an elegant solution -- it is periliously close to
busy-waiting (except that XSync() blocks). But my testing has shown
that it guarantees that we know the up-to-date position of
recently-moved frames, which is a requirement for detecting Type A
window managers (i.e., WMs that misposition frames by the width and
height of the WM decorations). Also, the loop in x_sync_with_move() is
bounded so that it cannot excessively consume cycles.

Then I changed x_set_offset() to perform these calls in this order:

   XMoveWindow()
   x_sync_with_move()
   x_check_expected_move()

I rewrote x_check_expected_move() so that it calls XMoveWindow()
directly to compensate for Type A window managers and the top/left
adjustments are recorded for future use with that frame. The
adjustments are stored in struct x_output, so the behavior is correct
even if the user's window manager draws differently sized decorations
around different Emacs frames (e.g., FVWM can be configured to do
this).

I removed the call to x_check_expected_move() from the code that
handles ConfigureNotify events, because it is now called synchronously
immediately after XMoveWindow().

I am currently running with this patch installed, and I'm seeing
completely deterministic frame positioning that accurately
compenstates for my Type A window manager (FVWM). I will test it
against twm and mwm.  If others can test it against their favorite WM,
I would greatly appreciate it.

A good acid test is to evaluate the following Elisp:

        (dolist (i '(1 2 3 4 5 6 7 8 9 a b c d e))
         (let ((frame (make-frame '((top . 50) (left . 50)))))
           (set-frame-position frame 200 200)
           (set-frame-position frame 300 300)))

All of the frames should end up exactly at x/y position 300/300.
--
Francis Litterio
flitterio@gmail.com
http://world.std.com/~franl/
GPG and PGP public keys available on keyservers.


Index: xterm.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xterm.h,v
retrieving revision 1.183
diff -w -u -r1.183 xterm.h
--- xterm.h	18 Mar 2006 13:49:11 -0000	1.183
+++ xterm.h	1 Apr 2006 13:56:52 -0000
@@ -637,18 +637,14 @@
      FocusOut and LeaveNotify clears EXPLICIT/IMPLICIT. */
   int focus_state;

-  /* The latest move we made to FRAME_OUTER_WINDOW.  Saved so we can
-     compensate for type A WMs (see wm_type in dpyinfo above).  */
-  int expected_top;
-  int expected_left;
-
   /* The offset we need to add to compensate for type A WMs.  */
   int move_offset_top;
   int move_offset_left;

-  /* Nonzero if we have made a move and needs to check if the WM placed us
-     at the right position.  */
-  int check_expected_move;
+  /* The frame's left/top offsets before we call XMoveWindow().  See
+     x_check_expected_move(). */
+  int left_before_move;
+  int top_before_move;
 };

 #define No_Cursor (None)


Index: xterm.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xterm.c,v
retrieving revision 1.909
diff -w -u -r1.909 xterm.c
--- xterm.c	24 Mar 2006 15:24:20 -0000	1.909
+++ xterm.c	1 Apr 2006 13:58:21 -0000
@@ -366,7 +366,8 @@
 					    Lisp_Object *, Lisp_Object *,
 					    unsigned long *));
 static void x_check_fullscreen P_ ((struct frame *));
-static void x_check_expected_move P_ ((struct frame *));
+static void x_check_expected_move P_ ((struct frame *, int, int));
+static void x_sync_with_move(struct frame *, int, int, int);
 static int handle_one_xevent P_ ((struct x_display_info *, XEvent *,
 				  int *, struct input_event *));

@@ -6661,11 +6662,8 @@
               && GTK_WIDGET_MAPPED (FRAME_GTK_OUTER_WIDGET (f)))
 #endif
             {
-	      /* What we have now is the position of Emacs's own window.
-		 Convert that to the position of the window manager window.  */
 	      x_real_positions (f, &f->left_pos, &f->top_pos);

-	      x_check_expected_move (f);
 	      if (f->want_fullscreen & FULLSCREEN_WAIT)
 		f->want_fullscreen &= ~(FULLSCREEN_WAIT|FULLSCREEN_BOTH);
             }
@@ -8212,8 +8210,11 @@
 {
   int modified_top, modified_left;

-  if (change_gravity > 0)
+  if (change_gravity != 0)
     {
+      FRAME_X_OUTPUT (f)->left_before_move = f->left_pos;
+      FRAME_X_OUTPUT (f)->top_before_move = f->top_pos;
+
       f->top_pos = yoff;
       f->left_pos = xoff;
       f->size_hint_flags &= ~ (XNegative | YNegative);
@@ -8231,7 +8232,7 @@
   modified_left = f->left_pos;
   modified_top = f->top_pos;

-  if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
+  if (change_gravity != 0 && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
     {
       /* Some WMs (twm, wmaker at least) has an offset that is smaller
          than the WM decorations.  So we use the calculated offset instead
@@ -8243,12 +8244,26 @@
   XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
                modified_left, modified_top);

-  if (FRAME_VISIBLE_P (f)
-      && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
+  x_sync_with_move(f, f->left_pos, f->top_pos,
+		   FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN ? 1 : 0);
+
+  /* change_gravity is non-zero when this function is called from Lisp to
+     programmatically move a frame.  In that case, we call
+     x_check_expected_move() to discover if we have a "Type A" or "Type B"
+     window manager, and, for a "Type A" window manager, adjust the position
+     of the frame.
+
+     We call x_check_expected_move() if a programmatic move occurred, and
+     either the window manager type (A/B) is unknown or it is Type A but we
+     need to compute the top/left offset adjustment for this frame.
+     */
+
+  if (change_gravity != 0 &&
+      (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN ||
+       (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A &&
+	(FRAME_X_OUTPUT (f)->move_offset_left == 0 && FRAME_X_OUTPUT
(f)->move_offset_top == 0))))
     {
-      FRAME_X_OUTPUT (f)->check_expected_move = 1;
-      FRAME_X_OUTPUT (f)->expected_top = f->top_pos;
-      FRAME_X_OUTPUT (f)->expected_left = f->left_pos;
+    x_check_expected_move(f, modified_left, modified_top);
     }

   UNBLOCK_INPUT;
@@ -8284,37 +8299,93 @@
     }
 }

-/* If frame parameters are set after the frame is mapped, we need to move
-   the window.
-   Some window managers moves the window to the right position, some
-   moves the outer window manager window to the specified position.
-   Here we check that we are in the right spot.  If not, make a second
-   move, assuming we are dealing with the second kind of window manager. */
+/* This function is called by x_set_offset() to determine whether the window
+   manager interfered with the positioning of the frame.  Type A window
+   managers position the surrounding window manager decorations a small
+   amount above and left of the user-supplied position.  Type B window
+   managers position the surrounding window manager decorations at the
+   user-specified position.  If we detect a Type A window manager, we
+   compensate by moving the window right and down by the proper amount. */
+
 static void
-x_check_expected_move (f)
+x_check_expected_move (f, expected_left, expected_top)
      struct frame *f;
+     int expected_left;
+     int expected_top;
 {
-  if (FRAME_X_OUTPUT (f)->check_expected_move)
-  {
-    int expect_top = FRAME_X_OUTPUT (f)->expected_top;
-    int expect_left = FRAME_X_OUTPUT (f)->expected_left;
+  int count = 0, current_left = 0, current_top = 0;
+
+  /* x_real_positions() returns the left and top offsets of the outermost
+     window manager window around the frame. */
+
+  x_real_positions (f, &current_left, &current_top);

-    if (expect_top != f->top_pos || expect_left != f->left_pos)
+  if (current_left != expected_left || current_top != expected_top)
       {
+    /* It's a "Type A" window manager. */
+
         FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_A;
-        FRAME_X_OUTPUT (f)->move_offset_left = expect_left - f->left_pos;
-        FRAME_X_OUTPUT (f)->move_offset_top = expect_top - f->top_pos;
+    FRAME_X_OUTPUT (f)->move_offset_left = expected_left - current_left;
+    FRAME_X_OUTPUT (f)->move_offset_top = expected_top - current_top;
+
+    /* Now fix the mispositioned frame's location. */

-        f->left_pos = expect_left;
-        f->top_pos = expect_top;
-        x_set_offset (f, expect_left, expect_top, 0);
+    int adjusted_left = expected_left + FRAME_X_OUTPUT (f)->move_offset_left;
+    int adjusted_top = expected_top + FRAME_X_OUTPUT (f)->move_offset_top;
+
+    XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
+		 adjusted_left, adjusted_top);
+
+    x_sync_with_move(f, expected_left, expected_top, 0);
       }
-    else if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
+  else
+  {
+    /* It's a "Type B" window manager.  We don't have to adjust the
+       frame's position. */
+
       FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_B;
+  }
+}
+

-    /* Just do this once */
-    FRAME_X_OUTPUT (f)->check_expected_move = 0;
+/* Wait for XGetGeometry() to return up-to-date position information for a
+   recently-moved frame.  Call this immediately after calling XMoveWindow().
+   If FUZZY is non-zero, then LEFT and TOP are just estimates of where the
+   frame has been moved to, so we use a fuzzy position comparison instead
+   of an exact comparison. */
+
+static void
+x_sync_with_move(f, left, top, fuzzy)
+    struct frame * f;
+    int left, top, fuzzy;
+{
+  int count = 0;
+
+  while (count++ < 50)
+  {
+    int current_left = 0, current_top = 0;
+
+    /* In theory, this call to XSync() only needs to happen once, but
in practice,
+       it doesn't seem to work, hence the need for the surrounding loop. */
+
+    XSync(FRAME_X_DISPLAY (f), False);
+    x_real_positions (f, &current_left, &current_top);
+
+    if (fuzzy)
+    {
+      /* The left fuzz-factor is 10 pixels.  The top fuzz-factor is
40 pixels. */
+
+      if (abs(current_left - left) <= 10 && abs(current_top - top) <= 40)
+	return;
   }
+    else if (current_left == left && current_top == top)
+      return;
+  }
+
+  /* As a last resort, just wait 0.5 seconds and hope that XGetGeometry()
+     will then return up-to-date position info. */
+
+  wait_reading_process_output(0, 500000, 0, 0, Qnil, NULL, 0);
 }

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    I am currently running with this patch installed, and I'm seeing
    completely deterministic frame positioning that accurately
    compenstates for my Type A window manager (FVWM). I will test it
    against twm and mwm.  If others can test it against their favorite WM,
    I would greatly appreciate it.

Thanks very much for workimg on this.
If you have indeed solved this problem, it will
end quite a bit of annoyance.

I await word of others' testing of this patch.

By the way, in GNU we reject the practice of writing "()" after
a function name to say it is a function.  `foo()' isn't the function
`foo', it is a call to `foo' with no arguments.

Re: Bug in CVS Emacs frame positioning under X

[Jan D.]

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



Fran Litterio wrote:

> Frankly, this is not an elegant solution -- it is periliously close to
> busy-waiting (except that XSync() blocks). But my testing has shown
> that it guarantees that we know the up-to-date position of
> recently-moved frames, which is a requirement for detecting Type A
> window managers (i.e., WMs that misposition frames by the width and
> height of the WM decorations). Also, the loop in x_sync_with_move() is
> bounded so that it cannot excessively consume cycles.

It is the window manager that intercepts the X events to and from Emacs that 
introduces this timing problems.  Some WMs are better that others, with 
sawfish the loop count rarely goes over 1 or 2, with metacity 45-48 is not 
uncommon.  I didn't want a blocking solution, but if that is the way to go, so 
be it.

> I am currently running with this patch installed, and I'm seeing
> completely deterministic frame positioning that accurately
> compenstates for my Type A window manager (FVWM). I will test it
> against twm and mwm.  If others can test it against their favorite WM,
> I would greatly appreciate it.

I've cleaned up your patch so it is more in the Gnu coding style (attached). 
I'll test it on some more window managers later.

	Jan D.


[-- Attachment #2: frame-pos.patch --]
[-- Type: text/x-patch, Size: 10057 bytes --]

Index: src/xterm.h
*** src/xterm.h.orig	2006-03-21 15:31:30.000000000 +0100
--- src/xterm.h	2006-04-03 09:01:38.000000000 +0200
***************
*** 637,654 ****
       FocusOut and LeaveNotify clears EXPLICIT/IMPLICIT. */
    int focus_state;
  
-   /* The latest move we made to FRAME_OUTER_WINDOW.  Saved so we can
-      compensate for type A WMs (see wm_type in dpyinfo above).  */
-   int expected_top;
-   int expected_left;
- 
    /* The offset we need to add to compensate for type A WMs.  */
    int move_offset_top;
    int move_offset_left;
  
!   /* Nonzero if we have made a move and needs to check if the WM placed us
!      at the right position.  */
!   int check_expected_move;
  };
  
  #define No_Cursor (None)
--- 637,650 ----
       FocusOut and LeaveNotify clears EXPLICIT/IMPLICIT. */
    int focus_state;
  
    /* The offset we need to add to compensate for type A WMs.  */
    int move_offset_top;
    int move_offset_left;
  
!   /* The frame's left/top offsets before we call XMoveWindow.  See
!      x_check_expected_move.  */
!   int left_before_move;
!   int top_before_move;
  };
  
  #define No_Cursor (None)
Index: src/xterm.c
*** src/xterm.c.orig	2006-04-03 08:30:23.000000000 +0200
--- src/xterm.c	2006-04-03 09:03:25.000000000 +0200
***************
*** 366,372 ****
  					    Lisp_Object *, Lisp_Object *,
  					    unsigned long *));
  static void x_check_fullscreen P_ ((struct frame *));
! static void x_check_expected_move P_ ((struct frame *));
  static int handle_one_xevent P_ ((struct x_display_info *, XEvent *,
  				  int *, struct input_event *));
  
--- 366,373 ----
  					    Lisp_Object *, Lisp_Object *,
  					    unsigned long *));
  static void x_check_fullscreen P_ ((struct frame *));
! static void x_check_expected_move P_ ((struct frame *, int, int));
! static void x_sync_with_move P_ ((struct frame *, int, int, int));
  static int handle_one_xevent P_ ((struct x_display_info *, XEvent *,
  				  int *, struct input_event *));
  
***************
*** 6661,6671 ****
                && GTK_WIDGET_MAPPED (FRAME_GTK_OUTER_WIDGET (f)))
  #endif
              {
- 	      /* What we have now is the position of Emacs's own window.
- 		 Convert that to the position of the window manager window.  */
  	      x_real_positions (f, &f->left_pos, &f->top_pos);
  
- 	      x_check_expected_move (f);
  	      if (f->want_fullscreen & FULLSCREEN_WAIT)
  		f->want_fullscreen &= ~(FULLSCREEN_WAIT|FULLSCREEN_BOTH);
              }
--- 6662,6669 ----
***************
*** 8212,8219 ****
  {
    int modified_top, modified_left;
  
!   if (change_gravity > 0)
      {
        f->top_pos = yoff;
        f->left_pos = xoff;
        f->size_hint_flags &= ~ (XNegative | YNegative);
--- 8210,8220 ----
  {
    int modified_top, modified_left;
  
!   if (change_gravity != 0)
      {
+       FRAME_X_OUTPUT (f)->left_before_move = f->left_pos;
+       FRAME_X_OUTPUT (f)->top_before_move = f->top_pos;
+ 
        f->top_pos = yoff;
        f->left_pos = xoff;
        f->size_hint_flags &= ~ (XNegative | YNegative);
***************
*** 8231,8237 ****
    modified_left = f->left_pos;
    modified_top = f->top_pos;
  
!   if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
      {
        /* Some WMs (twm, wmaker at least) has an offset that is smaller
           than the WM decorations.  So we use the calculated offset instead
--- 8232,8238 ----
    modified_left = f->left_pos;
    modified_top = f->top_pos;
  
!   if (change_gravity != 0 && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
      {
        /* Some WMs (twm, wmaker at least) has an offset that is smaller
           than the WM decorations.  So we use the calculated offset instead
***************
*** 8243,8255 ****
    XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
                 modified_left, modified_top);
  
!   if (FRAME_VISIBLE_P (f)
!       && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
!     {
!       FRAME_X_OUTPUT (f)->check_expected_move = 1;
!       FRAME_X_OUTPUT (f)->expected_top = f->top_pos;
!       FRAME_X_OUTPUT (f)->expected_left = f->left_pos;
!     }
  
    UNBLOCK_INPUT;
  }
--- 8244,8269 ----
    XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
                 modified_left, modified_top);
  
!   x_sync_with_move (f, f->left_pos, f->top_pos,
!                     FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN
!                     ? 1 : 0);
! 
!   /* change_gravity is non-zero when this function is called from Lisp to
!      programmatically move a frame.  In that case, we call
!      x_check_expected_move to discover if we have a "Type A" or "Type B"
!      window manager, and, for a "Type A" window manager, adjust the position
!      of the frame.
! 
!      We call x_check_expected_move if a programmatic move occurred, and
!      either the window manager type (A/B) is unknown or it is Type A but we
!      need to compute the top/left offset adjustment for this frame.  */
! 
!   if (change_gravity != 0 &&
!       (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN
!        || (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A
!            && (FRAME_X_OUTPUT (f)->move_offset_left == 0
!                && FRAME_X_OUTPUT (f)->move_offset_top == 0))))
!     x_check_expected_move (f, modified_left, modified_top);
  
    UNBLOCK_INPUT;
  }
***************
*** 8284,8320 ****
      }
  }
  
! /* If frame parameters are set after the frame is mapped, we need to move
!    the window.
!    Some window managers moves the window to the right position, some
!    moves the outer window manager window to the specified position.
!    Here we check that we are in the right spot.  If not, make a second
!    move, assuming we are dealing with the second kind of window manager. */
  static void
! x_check_expected_move (f)
       struct frame *f;
  {
!   if (FRAME_X_OUTPUT (f)->check_expected_move)
!   {
!     int expect_top = FRAME_X_OUTPUT (f)->expected_top;
!     int expect_left = FRAME_X_OUTPUT (f)->expected_left;
  
!     if (expect_top != f->top_pos || expect_left != f->left_pos)
        {
          FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_A;
!         FRAME_X_OUTPUT (f)->move_offset_left = expect_left - f->left_pos;
!         FRAME_X_OUTPUT (f)->move_offset_top = expect_top - f->top_pos;
  
!         f->left_pos = expect_left;
!         f->top_pos = expect_top;
!         x_set_offset (f, expect_left, expect_top, 0);
        }
!     else if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
        FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_B;
  
!     /* Just do this once */
!     FRAME_X_OUTPUT (f)->check_expected_move = 0;
    }
  }
  
  
--- 8298,8395 ----
      }
  }
  
! /* This function is called by x_set_offset to determine whether the window
!    manager interfered with the positioning of the frame.  Type A window
!    managers position the surrounding window manager decorations a small
!    amount above and left of the user-supplied position.  Type B window
!    managers position the surrounding window manager decorations at the
!    user-specified position.  If we detect a Type A window manager, we
!    compensate by moving the window right and down by the proper amount.  */
! 
  static void
! x_check_expected_move (f, expected_left, expected_top)
       struct frame *f;
+      int expected_left;
+      int expected_top;
  {
!   int count = 0, current_left = 0, current_top = 0;
  
!   /* x_real_positions returns the left and top offsets of the outermost
!      window manager window around the frame.  */
! 
!   x_real_positions (f, &current_left, &current_top);
! 
!   if (current_left != expected_left || current_top != expected_top)
      {
+       /* It's a "Type A" window manager. */
+ 
+       int adjusted_left;
+       int adjusted_top;
+ 
        FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_A;
!       FRAME_X_OUTPUT (f)->move_offset_left = expected_left - current_left;
!       FRAME_X_OUTPUT (f)->move_offset_top = expected_top - current_top;
  
!       /* Now fix the mispositioned frame's location. */
! 
!       adjusted_left = expected_left + FRAME_X_OUTPUT (f)->move_offset_left;
!       adjusted_top = expected_top + FRAME_X_OUTPUT (f)->move_offset_top;
! 
!       XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
!                    adjusted_left, adjusted_top);
! 
!       x_sync_with_move (f, expected_left, expected_top, 0);
      }
!   else
!     /* It's a "Type B" window manager.  We don't have to adjust the
!        frame's position. */
! 
      FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_B;
+ }
+ 
+ 
+ /* Wait for XGetGeometry to return up-to-date position information for a
+    recently-moved frame.  Call this immediately after calling XMoveWindow.
+    If FUZZY is non-zero, then LEFT and TOP are just estimates of where the
+    frame has been moved to, so we use a fuzzy position comparison instead
+    of an exact comparison.  */
  
! static void
! x_sync_with_move (f, left, top, fuzzy)
!     struct frame *f;
!     int left, top, fuzzy;
! {
!   int count = 0;
! 
!   while (count++ < 50)
!     {
!       int current_left = 0, current_top = 0;
! 
!       fprintf(stderr, "Count is %d\n", count);
! 
!       /* In theory, this call to XSync only needs to happen once, but in
!          practice, it doesn't seem to work, hence the need for the surrounding
!          loop.  */
! 
!       XSync (FRAME_X_DISPLAY (f), False);
!       x_real_positions (f, &current_left, &current_top);
! 
!       if (fuzzy)
!         {
!           /* The left fuzz-factor is 10 pixels.  The top fuzz-factor is 40
!              pixels.  */
! 
!           if (abs (current_left - left) <= 10 && abs (current_top - top) <= 40)
!             return;
          }
+       else if (current_left == left && current_top == top)
+         return;
+     }
+ 
+   /* As a last resort, just wait 0.5 seconds and hope that XGetGeometry
+      will then return up-to-date position info. */
+ 
+   wait_reading_process_output (0, 500000, 0, 0, Qnil, NULL, 0);
  }
  
  

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

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

Re: Bug in CVS Emacs frame positioning under X

[Jan D.]

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


> 
> I've cleaned up your patch so it is more in the Gnu coding style 
> (attached). I'll test it on some more window managers later.
> 


Sorry, I left a debug printf in there.  A better patch attached.

	Jan D.

[-- Attachment #2: frame-pos.patch --]
[-- Type: text/x-patch, Size: 10005 bytes --]

Index: src/xterm.h
*** src/xterm.h.orig	2006-03-21 15:31:30.000000000 +0100
--- src/xterm.h	2006-04-03 09:01:38.000000000 +0200
***************
*** 637,654 ****
       FocusOut and LeaveNotify clears EXPLICIT/IMPLICIT. */
    int focus_state;
  
-   /* The latest move we made to FRAME_OUTER_WINDOW.  Saved so we can
-      compensate for type A WMs (see wm_type in dpyinfo above).  */
-   int expected_top;
-   int expected_left;
- 
    /* The offset we need to add to compensate for type A WMs.  */
    int move_offset_top;
    int move_offset_left;
  
!   /* Nonzero if we have made a move and needs to check if the WM placed us
!      at the right position.  */
!   int check_expected_move;
  };
  
  #define No_Cursor (None)
--- 637,650 ----
       FocusOut and LeaveNotify clears EXPLICIT/IMPLICIT. */
    int focus_state;
  
    /* The offset we need to add to compensate for type A WMs.  */
    int move_offset_top;
    int move_offset_left;
  
!   /* The frame's left/top offsets before we call XMoveWindow.  See
!      x_check_expected_move.  */
!   int left_before_move;
!   int top_before_move;
  };
  
  #define No_Cursor (None)
Index: src/xterm.c
*** src/xterm.c.orig	2006-04-03 08:30:23.000000000 +0200
--- src/xterm.c	2006-04-03 09:31:02.000000000 +0200
***************
*** 366,372 ****
  					    Lisp_Object *, Lisp_Object *,
  					    unsigned long *));
  static void x_check_fullscreen P_ ((struct frame *));
! static void x_check_expected_move P_ ((struct frame *));
  static int handle_one_xevent P_ ((struct x_display_info *, XEvent *,
  				  int *, struct input_event *));
  
--- 366,373 ----
  					    Lisp_Object *, Lisp_Object *,
  					    unsigned long *));
  static void x_check_fullscreen P_ ((struct frame *));
! static void x_check_expected_move P_ ((struct frame *, int, int));
! static void x_sync_with_move P_ ((struct frame *, int, int, int));
  static int handle_one_xevent P_ ((struct x_display_info *, XEvent *,
  				  int *, struct input_event *));
  
***************
*** 6661,6671 ****
                && GTK_WIDGET_MAPPED (FRAME_GTK_OUTER_WIDGET (f)))
  #endif
              {
- 	      /* What we have now is the position of Emacs's own window.
- 		 Convert that to the position of the window manager window.  */
  	      x_real_positions (f, &f->left_pos, &f->top_pos);
  
- 	      x_check_expected_move (f);
  	      if (f->want_fullscreen & FULLSCREEN_WAIT)
  		f->want_fullscreen &= ~(FULLSCREEN_WAIT|FULLSCREEN_BOTH);
              }
--- 6662,6669 ----
***************
*** 8212,8219 ****
  {
    int modified_top, modified_left;
  
!   if (change_gravity > 0)
      {
        f->top_pos = yoff;
        f->left_pos = xoff;
        f->size_hint_flags &= ~ (XNegative | YNegative);
--- 8210,8220 ----
  {
    int modified_top, modified_left;
  
!   if (change_gravity != 0)
      {
+       FRAME_X_OUTPUT (f)->left_before_move = f->left_pos;
+       FRAME_X_OUTPUT (f)->top_before_move = f->top_pos;
+ 
        f->top_pos = yoff;
        f->left_pos = xoff;
        f->size_hint_flags &= ~ (XNegative | YNegative);
***************
*** 8231,8237 ****
    modified_left = f->left_pos;
    modified_top = f->top_pos;
  
!   if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
      {
        /* Some WMs (twm, wmaker at least) has an offset that is smaller
           than the WM decorations.  So we use the calculated offset instead
--- 8232,8238 ----
    modified_left = f->left_pos;
    modified_top = f->top_pos;
  
!   if (change_gravity != 0 && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A)
      {
        /* Some WMs (twm, wmaker at least) has an offset that is smaller
           than the WM decorations.  So we use the calculated offset instead
***************
*** 8243,8255 ****
    XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
                 modified_left, modified_top);
  
!   if (FRAME_VISIBLE_P (f)
!       && FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
!     {
!       FRAME_X_OUTPUT (f)->check_expected_move = 1;
!       FRAME_X_OUTPUT (f)->expected_top = f->top_pos;
!       FRAME_X_OUTPUT (f)->expected_left = f->left_pos;
!     }
  
    UNBLOCK_INPUT;
  }
--- 8244,8269 ----
    XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
                 modified_left, modified_top);
  
!   x_sync_with_move (f, f->left_pos, f->top_pos,
!                     FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN
!                     ? 1 : 0);
! 
!   /* change_gravity is non-zero when this function is called from Lisp to
!      programmatically move a frame.  In that case, we call
!      x_check_expected_move to discover if we have a "Type A" or "Type B"
!      window manager, and, for a "Type A" window manager, adjust the position
!      of the frame.
! 
!      We call x_check_expected_move if a programmatic move occurred, and
!      either the window manager type (A/B) is unknown or it is Type A but we
!      need to compute the top/left offset adjustment for this frame.  */
! 
!   if (change_gravity != 0 &&
!       (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN
!        || (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_A
!            && (FRAME_X_OUTPUT (f)->move_offset_left == 0
!                && FRAME_X_OUTPUT (f)->move_offset_top == 0))))
!     x_check_expected_move (f, modified_left, modified_top);
  
    UNBLOCK_INPUT;
  }
***************
*** 8284,8320 ****
      }
  }
  
! /* If frame parameters are set after the frame is mapped, we need to move
!    the window.
!    Some window managers moves the window to the right position, some
!    moves the outer window manager window to the specified position.
!    Here we check that we are in the right spot.  If not, make a second
!    move, assuming we are dealing with the second kind of window manager. */
  static void
! x_check_expected_move (f)
       struct frame *f;
  {
!   if (FRAME_X_OUTPUT (f)->check_expected_move)
!   {
!     int expect_top = FRAME_X_OUTPUT (f)->expected_top;
!     int expect_left = FRAME_X_OUTPUT (f)->expected_left;
  
!     if (expect_top != f->top_pos || expect_left != f->left_pos)
        {
          FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_A;
!         FRAME_X_OUTPUT (f)->move_offset_left = expect_left - f->left_pos;
!         FRAME_X_OUTPUT (f)->move_offset_top = expect_top - f->top_pos;
  
!         f->left_pos = expect_left;
!         f->top_pos = expect_top;
!         x_set_offset (f, expect_left, expect_top, 0);
        }
!     else if (FRAME_X_DISPLAY_INFO (f)->wm_type == X_WMTYPE_UNKNOWN)
        FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_B;
  
!     /* Just do this once */
!     FRAME_X_OUTPUT (f)->check_expected_move = 0;
    }
  }
  
  
--- 8298,8393 ----
      }
  }
  
! /* This function is called by x_set_offset to determine whether the window
!    manager interfered with the positioning of the frame.  Type A window
!    managers position the surrounding window manager decorations a small
!    amount above and left of the user-supplied position.  Type B window
!    managers position the surrounding window manager decorations at the
!    user-specified position.  If we detect a Type A window manager, we
!    compensate by moving the window right and down by the proper amount.  */
! 
  static void
! x_check_expected_move (f, expected_left, expected_top)
       struct frame *f;
+      int expected_left;
+      int expected_top;
  {
!   int count = 0, current_left = 0, current_top = 0;
  
!   /* x_real_positions returns the left and top offsets of the outermost
!      window manager window around the frame.  */
! 
!   x_real_positions (f, &current_left, &current_top);
! 
!   if (current_left != expected_left || current_top != expected_top)
      {
+       /* It's a "Type A" window manager. */
+ 
+       int adjusted_left;
+       int adjusted_top;
+ 
        FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_A;
!       FRAME_X_OUTPUT (f)->move_offset_left = expected_left - current_left;
!       FRAME_X_OUTPUT (f)->move_offset_top = expected_top - current_top;
  
!       /* Now fix the mispositioned frame's location. */
! 
!       adjusted_left = expected_left + FRAME_X_OUTPUT (f)->move_offset_left;
!       adjusted_top = expected_top + FRAME_X_OUTPUT (f)->move_offset_top;
! 
!       XMoveWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
!                    adjusted_left, adjusted_top);
! 
!       x_sync_with_move (f, expected_left, expected_top, 0);
      }
!   else
!     /* It's a "Type B" window manager.  We don't have to adjust the
!        frame's position. */
! 
      FRAME_X_DISPLAY_INFO (f)->wm_type = X_WMTYPE_B;
+ }
+ 
+ 
+ /* Wait for XGetGeometry to return up-to-date position information for a
+    recently-moved frame.  Call this immediately after calling XMoveWindow.
+    If FUZZY is non-zero, then LEFT and TOP are just estimates of where the
+    frame has been moved to, so we use a fuzzy position comparison instead
+    of an exact comparison.  */
  
! static void
! x_sync_with_move (f, left, top, fuzzy)
!     struct frame *f;
!     int left, top, fuzzy;
! {
!   int count = 0;
! 
!   while (count++ < 50)
!     {
!       int current_left = 0, current_top = 0;
! 
!       /* In theory, this call to XSync only needs to happen once, but in
!          practice, it doesn't seem to work, hence the need for the surrounding
!          loop.  */
! 
!       XSync (FRAME_X_DISPLAY (f), False);
!       x_real_positions (f, &current_left, &current_top);
! 
!       if (fuzzy)
!         {
!           /* The left fuzz-factor is 10 pixels.  The top fuzz-factor is 40
!              pixels.  */
! 
!           if (abs (current_left - left) <= 10 && abs (current_top - top) <= 40)
!             return;
          }
+       else if (current_left == left && current_top == top)
+         return;
+     }
+ 
+   /* As a last resort, just wait 0.5 seconds and hope that XGetGeometry
+      will then return up-to-date position info. */
+ 
+   wait_reading_process_output (0, 500000, 0, 0, Qnil, NULL, 0);
  }
  
  

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

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

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

Thanks for cleaning up the patch.

Did you replace it, or just clean it up?
It is large enough that we need papers to install it,
but if your version does not use his code, we could install
your version now.

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

> Did you replace it, or just clean it up?

He just cleaned it up. It's still substantially the patch I submitted.

> It is large enough that we need papers to install it,
> but if your version does not use his code, we could install
> your version now.

I've emailed a request for copyright assignment. When I get the
assignment form, I'll fill it out and send it back immediately.
--
Fran Litterio

Re: Bug in CVS Emacs frame positioning under X

[Jan D.]

Fran Litterio wrote:
>> Did you replace it, or just clean it up?
> 
> He just cleaned it up. It's still substantially the patch I submitted.

s/substantially//

:-)

	Jan D.

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    I've emailed a request for copyright assignment. When I get the
    assignment form, I'll fill it out and send it back immediately.

Thanks.

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

Richard,

Just an update: I received the copyright assignment form, signed it,
and mailed it back. I don't know if you need to physically receive the
form to apply my patch, but I thought I'd let you know it's on the
way.
--
Fran Litterio

On 4/4/06, Richard Stallman <rms@gnu.org> wrote:
>     I've emailed a request for copyright assignment. When I get the
>     assignment form, I'll fill it out and send it back immediately.
>
> Thanks.
>

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

    Just an update: I received the copyright assignment form, signed it,
    and mailed it back. I don't know if you need to physically receive the
    form to apply my patch, but I thought I'd let you know it's on the
    way.

We do need to wait for the papers to arrive.

Re: Bug in CVS Emacs frame positioning under X

[Fran Litterio]

On 5/9/06, Richard Stallman <rms@gnu.org> wrote:
>     Just an update: I received the copyright assignment form, signed it,
>     and mailed it back. I don't know if you need to physically receive the
>     form to apply my patch, but I thought I'd let you know it's on the
>     way.
>
> We do need to wait for the papers to arrive.

FYI, I mailed the papers about a week ago, so the FSF should have them by now.
--
Francis Litterio

Re: Bug in CVS Emacs frame positioning under X

[Richard Stallman]

If you have not got mail about it, that means we have not yet received
and recorded it.  It can easily take more than a week, but I suggest
you ask assist@gnu.org on Friday, and if it hasn't arrived by then, we
can try over.

Re: Bug in CVS Emacs frame positioning under X

[Jan D.]

Richard Stallman wrote:
> Thanks for cleaning up the patch.
> 
> Did you replace it, or just clean it up?
> It is large enough that we need papers to install it,
> but if your version does not use his code, we could install
> your version now.

I just cleaned up indentation, whitespace, parentesis and such.  The patch is 
still his, so we need papers.

	Jan D.

Re: Bug in CVS Emacs frame positioning under X

[Francis Litterio]

On 3/22/06, I (Fran Litterio) wrote:

> On 3/22/06, Richard Stallman wrote:
>
> > In the latest CVS Emacs under X, there seems to be a frame positioning bug
> > that is timing related.
> >
> > Does it happen in older CVS versions too?  For instance,
> > did it happen a month ago?  6 months ago?
>
> Unknown. I'll check-out the older sources and rebuild. I'll let you know.

I have checked out the CVS Emacs source code from October 1, 2005, and
the bug existed at that time.  Additionally, I have found a simple way
to reproduce the problem:

  1. Start Emacs under the X Window system using the command: emacs -q

  2. Evaluate the following Elisp:

	(dolist (i '(1 2 3 4 5 6 7 8 9 a b c))
	  (let ((frame (make-frame '((top . 50) (left . 50)))))
	    (set-frame-position frame 200 200)
	    (set-frame-position frame 300 300)
	    (sit-for 0.5)))

You may have to evaluate the above Elisp more than once to see the
malfunction (although I see it every time).

The symptom is that not all of the frames end up positioned at x/y
coordinates 300/300.  Some frames are positioned at 300+A/300+B, where
A is the width of the left border drawn by the window manager and B is
the height of the top border drawn by the window manager.  Some frames
even end up at 400/400 !

I think the cause of this problem is the fact that src/xterm.c assumes
(incorrectly) that, after calling XMoveWindow() for a given frame, the next
ConfigureNotify event to be received for that frame will contain the position
data from the call to XMoveWindow().  In fact, the ConfigureNotify event can
sometimes contain the position of the frame before the call to XMoveWindow()
(this may be a race condition, but I'm not sure).

This incorrect assumption interacts badly with the code in
x_check_expected_move(), which attempts to use the position data from the most
recently received ConfigureNotify to validate that the most recent XMoveWindow()
call correctly positioned the frame.

I don't yet know how to fix this.
--
Fran Litterio