x-popup-menu pops up at funny positions

x-popup-menu pops up at funny positions

[Jan D.]

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

Hello.

While trying to reproduce a reported problem with x-popup-menu (that problem 
seems to be gone), I saw that x-popup-menu doesn't put the menu in the correct 
position if the (x y) variant is used, like this:

(x-popup-menu
  (list '(1 1) (selected-window))
  (list "title" (cons "title" '(("ignore" 'ignore)))
        ))

For lucid and motif, this puts the popup menu at a position that would be 
correct if the Emacs edit window is at (0 0) on the root window.  If Emacs is
anywhere else on the root window, this is wrong.

Here is a patch to fix that.  Is it OK to check in?

	Jan D.

[-- Attachment #2: xmenu.c.patch --]
[-- Type: text/plain, Size: 2049 bytes --]

*** src/xmenu.c.~1.239.~	2002-12-27 16:43:12.000000000 +0100
--- src/xmenu.c	2003-01-03 16:48:42.000000000 +0100
***************
*** 2279,2315 ****
  			   popup_deactivate_callback,
  			   menu_highlight_callback);
  
-   /* Adjust coordinates to relative to the outer (window manager) window.  */
-   {
-     Window child;
-     int win_x = 0, win_y = 0;
- 
-     /* Find the position of the outside upper-left corner of
-        the inner window, with respect to the outer window.  */
-     if (f->output_data.x->parent_desc != FRAME_X_DISPLAY_INFO (f)->root_window)
-       {
- 	BLOCK_INPUT;
- 	XTranslateCoordinates (FRAME_X_DISPLAY (f),
- 
- 			       /* From-window, to-window.  */
- 			       f->output_data.x->window_desc,
- 			       f->output_data.x->parent_desc,
- 
- 			       /* From-position, to-position.  */
- 			       0, 0, &win_x, &win_y,
- 
- 			       /* Child of window.  */
- 			       &child);
- 	UNBLOCK_INPUT;
- 	x += win_x;
- 	y += win_y;
-       }
-   }
- 
-   /* Adjust coordinates to be root-window-relative.  */
-   x += f->output_data.x->left_pos;
-   y += f->output_data.x->top_pos;
- 
    dummy.type = ButtonPress;
    dummy.serial = 0;
    dummy.send_event = 0;
--- 2279,2284 ----
***************
*** 2318,2327 ****
    dummy.root = FRAME_X_DISPLAY_INFO (f)->root_window;
    dummy.window = dummy.root;
    dummy.subwindow = dummy.root;
-   dummy.x_root = x;
-   dummy.y_root = y;
    dummy.x = x;
    dummy.y = y;
    dummy.state = (FRAME_X_DISPLAY_INFO (f)->grabbed >> 1) * Button1Mask;
    dummy.button = 0;
    for (i = 0; i < 5; i++)
--- 2287,2299 ----
    dummy.root = FRAME_X_DISPLAY_INFO (f)->root_window;
    dummy.window = dummy.root;
    dummy.subwindow = dummy.root;
    dummy.x = x;
    dummy.y = y;
+   /* Adjust coordinates to be root-window-relative.  */
+   x += f->output_data.x->left_pos;
+   y += f->output_data.x->top_pos;
+   dummy.x_root = x;
+   dummy.y_root = y;
    dummy.state = (FRAME_X_DISPLAY_INFO (f)->grabbed >> 1) * Button1Mask;
    dummy.button = 0;
    for (i = 0; i < 5; i++)

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

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

Re: x-popup-menu pops up at funny positions

[Jan D.]

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

Jan D. wrote:
> Hello.
> 
> While trying to reproduce a reported problem with x-popup-menu (that 
> problem seems to be gone), I saw that x-popup-menu doesn't put the menu 
> in the correct position if the (x y) variant is used, like this:
> 
> (x-popup-menu
>  (list '(1 1) (selected-window))
>  (list "title" (cons "title" '(("ignore" 'ignore)))
>        ))
> 
> For lucid and motif, this puts the popup menu at a position that would 
> be correct if the Emacs edit window is at (0 0) on the root window.  If 
> Emacs is
> anywhere else on the root window, this is wrong.
> 
> Here is a patch to fix that.  Is it OK to check in?

Since I normally don't use the menu bar, I got it wrong.  Here is a correct patch.

	Jan D.


[-- Attachment #2: xmenu.c.patch --]
[-- Type: text/plain, Size: 2335 bytes --]

Index: src/xmenu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xmenu.c,v
retrieving revision 1.239
diff -c -r1.239 xmenu.c
*** src/xmenu.c	23 Dec 2002 18:06:42 -0000	1.239
--- src/xmenu.c	3 Jan 2003 17:12:49 -0000
***************
*** 2279,2315 ****
  			   popup_deactivate_callback,
  			   menu_highlight_callback);
  
-   /* Adjust coordinates to relative to the outer (window manager) window.  */
-   {
-     Window child;
-     int win_x = 0, win_y = 0;
- 
-     /* Find the position of the outside upper-left corner of
-        the inner window, with respect to the outer window.  */
-     if (f->output_data.x->parent_desc != FRAME_X_DISPLAY_INFO (f)->root_window)
-       {
- 	BLOCK_INPUT;
- 	XTranslateCoordinates (FRAME_X_DISPLAY (f),
- 
- 			       /* From-window, to-window.  */
- 			       f->output_data.x->window_desc,
- 			       f->output_data.x->parent_desc,
- 
- 			       /* From-position, to-position.  */
- 			       0, 0, &win_x, &win_y,
- 
- 			       /* Child of window.  */
- 			       &child);
- 	UNBLOCK_INPUT;
- 	x += win_x;
- 	y += win_y;
-       }
-   }
- 
-   /* Adjust coordinates to be root-window-relative.  */
-   x += f->output_data.x->left_pos;
-   y += f->output_data.x->top_pos;
- 
    dummy.type = ButtonPress;
    dummy.serial = 0;
    dummy.send_event = 0;
--- 2279,2284 ----
***************
*** 2318,2327 ****
    dummy.root = FRAME_X_DISPLAY_INFO (f)->root_window;
    dummy.window = dummy.root;
    dummy.subwindow = dummy.root;
-   dummy.x_root = x;
-   dummy.y_root = y;
    dummy.x = x;
    dummy.y = y;
    dummy.state = (FRAME_X_DISPLAY_INFO (f)->grabbed >> 1) * Button1Mask;
    dummy.button = 0;
    for (i = 0; i < 5; i++)
--- 2287,2302 ----
    dummy.root = FRAME_X_DISPLAY_INFO (f)->root_window;
    dummy.window = dummy.root;
    dummy.subwindow = dummy.root;
    dummy.x = x;
    dummy.y = y;
+   /* Adjust coordinates to be root-window-relative.  */
+   x += f->output_data.x->left_pos + f->output_data.x->x_pixels_diff;
+   y += f->output_data.x->top_pos
+     + f->output_data.x->menubar_height
+     + f->output_data.x->y_pixels_outer_diff;
+   
+   dummy.x_root = x;
+   dummy.y_root = y;
    dummy.state = (FRAME_X_DISPLAY_INFO (f)->grabbed >> 1) * Button1Mask;
    dummy.button = 0;
    for (i = 0; i < 5; i++)

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

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

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

Your patch changes two things:

1. It offsets the values for x_root and y_root but not for dummy.x and
dummy.y.

2. It deletes the code to read the current position.

Why did you do #2?

Re: x-popup-menu pops up at funny positions

[Jan D.]

> Your patch changes two things:
> 
> 1. It offsets the values for x_root and y_root but not for dummy.x and
> dummy.y.

When popping up a menu, only x_root and y_root are actually used.
But for correctnes sake, these should not be the same.

> 2. It deletes the code to read the current position.
> 
> Why did you do #2?

Because it did not in fact read the current position.  It read
the position of the inner window in coordinates that are valid
for the parent window.  Now, if the parent window is the root
window, then it reads the current position.  But in all other
cases it does not.  For example, a window manager may put
any number of windows between the root window and the inner
window.  It may even put a window that is identical to the
inner window in size and placement as a parent to the inner
window (as the window manager I was running while testing does).

One could replace parent_desc with the root window, but it
seemd pointless to do a query to the X server when Emacs
has all the information already and a simple addition finds
the current position.

But I found more problems when x-popup-menu is supposed
to pop up at the mouse position.  It works if the mouse is in
the Emacs edit window, but not for any other case.  This is
because mouse_position_hook is called to get the pointer
position.  If the pointer is in the menu bar, x/y is the
position in the menu bar, not in the edit window.  So
if the pointer is in 1/1 in the menu bar, the code thinks
it is in 1/1 in the edit widget.  Similar errors occur
when the pointer is outside any Emacs window, or in the title
bar of the X window.

Replacing mouse_position_hook with a simple XQueryPointer
cures this.

	Jan D.

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

    Because it did not in fact read the current position.  It read
    the position of the inner window in coordinates that are valid
    for the parent window.  Now, if the parent window is the root
    window, then it reads the current position.  But in all other
    cases it does not.

Instead of just deleting that code, can you replace it with code
that does the right job?

    One could replace parent_desc with the root window, but it
    seemd pointless to do a query to the X server when Emacs
    has all the information already and a simple addition finds
    the current position.

I am not sure it is always up to date.  Could you check?

    Replacing mouse_position_hook with a simple XQueryPointer
    cures this.

Could you send the diff to do that?
(Or just install it?)

Re: x-popup-menu pops up at funny positions

[Jan D.]

>     Because it did not in fact read the current position.  It read
>     the position of the inner window in coordinates that are valid
>     for the parent window.  Now, if the parent window is the root
>     window, then it reads the current position.  But in all other
>     cases it does not.
>
> Instead of just deleting that code, can you replace it with code
> that does the right job?

I did that.  The additions to x/y before assigning them to dummy.root_x/y.

>     One could replace parent_desc with the root window, but it
>     seemd pointless to do a query to the X server when Emacs
>     has all the information already and a simple addition finds
>     the current position.
>
> I am not sure it is always up to date.  Could you check?

I will check some more on different window managers, but so far
it has been up to date.  I studied the code and I believe it keeps
things up to date.


>     Replacing mouse_position_hook with a simple XQueryPointer
>     cures this.
>
> Could you send the diff to do that?
> (Or just install it?)

I'll install it when I done the checking (and it still is OK :-)

	Jan D.

Re: x-popup-menu pops up at funny positions

[Jan D.]

>>     One could replace parent_desc with the root window, but it
>>     seemd pointless to do a query to the X server when Emacs
>>     has all the information already and a simple addition finds
>>     the current position.
>>
>> I am not sure it is always up to date.  Could you check?
>
> I will check some more on different window managers, but so far
> it has been up to date.  I studied the code and I believe it keeps
> things up to date.

There is one border case.  When a dialog is up and the Emacs frame is
moved, then the positions will be wrong.  Updating the positions
on EnterNotify by calling x_real_position fixes that.

	Jan D.

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

    > Instead of just deleting that code, can you replace it with code
    > that does the right job?

    I did that.  The additions to x/y before assigning them to dummy.root_x/y.

We are miscommunicating.  "The job" I'm talking about is to read the
current position.  I'm asking you to correct the code to read the
current position, instead of deleting it.

You're claiming that the current position is always accurate, so there
is no need to read it.  That might be true, but without being able to
prove this to ourselves informally, I don't think we should rely on it.

So how about writing code to read the current position, compare it
with the recorded position, and abort if they differ?  That way
we will find out if it isn't always right.

Re: x-popup-menu pops up at funny positions

[Jan D.]

>     > Instead of just deleting that code, can you replace it with code
>     > that does the right job?
> 
>     I did that.  The additions to x/y before assigning them to dummy.root_x/y.
> 
> We are miscommunicating.  "The job" I'm talking about is to read the
> current position.  I'm asking you to correct the code to read the
> current position, instead of deleting it.

I could add a call to x_real_positions.  Any correction
would look exactly like the code in x_real_positions.

> You're claiming that the current position is always accurate, so there
> is no need to read it.  That might be true, but without being able to
> prove this to ourselves informally, I don't think we should rely on it.

How about this:

There is no way to move a window in X without getting a ConfigureNotify
(disregarding buggy X servers, which we can't compensate for anyway).
The code for ConfigureNotify updates the real position by calling
x_real_positions.  So if Emacs gets to the case ConfigureNotify: in
the event switch, the position is correct.

Can we get a ConfigureNotify without hitting the case ConfigureNotify:
code?  Yes, if we are doing a recursive X event loop.  There are two such
instances, one in xmenu.c, popup_get_selection and one in xfns.c,
Fx_file_dialog.  The solution suggested is to take advantage of
the split that the GTK patch does, so the case ConfigureNotify code
is executed even for recursive X event loops.

> So how about writing code to read the current position, compare it
> with the recorded position, and abort if they differ?  That way
> we will find out if it isn't always right.

Abort sounds a bit drastic, how about popping up a dialog instead?

	Jan D.

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

    I could add a call to x_real_positions.  Any correction
    would look exactly like the code in x_real_positions.

Ok, how about calling x_real_positions?

I am pretty sure that x_real_positions was really necessary in the
past, at least.

      The solution suggested is to take advantage of
    the split that the GTK patch does, so the case ConfigureNotify code
    is executed even for recursive X event loops.

Can you tell me how that split is useful here?

    Abort sounds a bit drastic, how about popping up a dialog instead?

This abort would be temporary.  Once we find out whether they are ever
different, we would either fix the code so they are never different, or
remove the abort and just call x_real_positions here.

Re: x-popup-menu pops up at funny positions

[Jan D.]

>     I could add a call to x_real_positions.  Any correction
>     would look exactly like the code in x_real_positions.
> 
> Ok, how about calling x_real_positions?

Will do.

>       The solution suggested is to take advantage of
>     the split that the GTK patch does, so the case ConfigureNotify code
>     is executed even for recursive X event loops.
> 
> Can you tell me how that split is useful here?

In the current code we can't pass just one XEvent to the big switch
over event types in xterm.c.  With the split we can.  So instead of
calling XtDispatchEvent, we use the big switch so the event is
passed through the case for that event, and then goto OTHER
passes it to Xt.  In a slightly modified way, this is what the GTK code
does.

Since all event are passed through the big switch even when dialogs/popup
menus are posted, there is no need to save them for later.

>     Abort sounds a bit drastic, how about popping up a dialog instead?
> 
> This abort would be temporary.  Once we find out whether they are ever
> different, we would either fix the code so they are never different, or
> remove the abort and just call x_real_positions here.

Okay.

	Jan D.

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

    > Can you tell me how that split is useful here?

    In the current code we can't pass just one XEvent to the big switch
    over event types in xterm.c.  With the split we can.  So instead of
    calling XtDispatchEvent, we use the big switch so the event is
    passed through the case for that event, and then goto OTHER
    passes it to Xt.  In a slightly modified way, this is what the GTK code
    does.

I see.  Yes, that does seem clean.  If it works, then go for it.

Re: x-popup-menu pops up at funny positions

[Jan D.]

>     > Can you tell me how that split is useful here?
> 
>     In the current code we can't pass just one XEvent to the big switch
>     over event types in xterm.c.  With the split we can.  So instead of
>     calling XtDispatchEvent, we use the big switch so the event is
>     passed through the case for that event, and then goto OTHER
>     passes it to Xt.  In a slightly modified way, this is what the GTK code
>     does.
> 
> I see.  Yes, that does seem clean.  If it works, then go for it.

Checked in.

	Jan D.

Re: x-popup-menu pops up at funny positions

[Jan D.]

> 
> > So how about writing code to read the current position, compare it
> > with the recorded position, and abort if they differ?  That way
> > we will find out if it isn't always right.
> 
> Abort sounds a bit drastic, how about popping up a dialog instead?

Even the old code relied on these positions to always be correct, it
is not like we are introducing new requirements for correctness.

	Jan D.

Re: x-popup-menu pops up at funny positions

[Richard Stallman]

    Even the old code relied on these positions to always be correct, 

Where did it require that?  I thought it was supposed to call
x_real_positions to get the correct position when necessary.
But I see thats not how it worked.

I guess the code is ok as you've written it.  Please install it.

Thanks.

Re: x-popup-menu pops up at funny positions

[Jan D.]

>     Even the old code relied on these positions to always be correct, 
> 
> Where did it require that?  I thought it was supposed to call
> x_real_positions to get the correct position when necessary.
> But I see thats not how it worked.

The old code did this:
  /* Adjust coordinates to be root-window-relative.  */
  x += f->output_data.x->left_pos;
  y += f->output_data.x->top_pos;

> 
> I guess the code is ok as you've written it.  Please install it.

Done.

	Jan D.