I can confirm this too.

I should also add that this has been tested against GIMP, Inkscape, and MyPaint as well. The typical symptoms tend to be that if you have the extended input capabilities configured, the on-canvas coordinates are offset by whatever additional widgets are to the top or left of the canvas. So, in Inkscape, the the more toolbars you have at the top will invisibly offset the input from the cursor on the canvas by the same height.

Created attachment 142353 [details]
Offset coordinates

This attachment shows the offset (visible by the arrow coordinates on the ruler) in relation to 0,0 on Inkscape's canvas. Note that there are three toolbars above canvas and the toolbox on the left side, as well as the rulers. This is a cropped screenshot fyi.

A more in-depth discussion about the issue can be found at:
https://bugs.launchpad.net/bugs/410267

This seems to be due to events coming from input devices not being translated by _gdk_windowing_got_event(), so coordinates/window translation doesn't happen.

I will try to work on it on my xi2 branch so this specific fix can be backported to master.

Yeah, the input events handling is quite lame, but then it was pretty lame initially. However, i though i fixed this by always making the window native if you request input events on it, maybe that got broken somehow?

Of course, ideally we should make the input event handling not so lame.

I'd like to point out that fixing the coordinates/window translation would be good, but is not a sufficient step as far as I'm concerned. (Xournal works around coordinate/window translation issues when XInput events are sent to its main drawing area). From Xournal's perspective it would be much better if widgets just received core events and no extended input events. 

Most XInput device drivers out there send extended events with completely meaningless (x,y) coordinates. Even those that do send meaningful (x,y) coordinates will still get mangled by GTK+ if the device resolution gets reconfigured while the application is running (think: tablet PC screen rotation). Xournal's workaround for the device reconfigure problem is to set the input device axes in GDK_AXIS_IGNORE mode and calculate the window-relative coordinates of XInput events manually; when a device's axes are set to GDK_AXIS_IGNORE, event coordinate translation within GDK does not happen, and there's no way that a widget that has no XInput awareness would know what to do with the event.

Anyway: anything would be better than the current situation, but my vote would be to stop sending extended input events to widgets that weren't meant to handle them, and just make sure they receive the corresponding core pointer event instead. (Note: in recent GTK+, core pointer events tend to get discarded silently whenever they match with an extended input event from an enabled device -- in my view this is a misfeature, I'd rather receive both and be able to handle whichever one I like or even both if I wish to).

Thanks in advance.
Denis

Two additional related problems in the GTK+ 2.17 extended event code:

1. a pretty bad crasher: clicking in a combo box with an enabled XInput device results in a segfault in gdk_input_translate_coordinates()  (e.g.: in GIMP, the combo box that controls the zoom level at the bottom of the drawing window; also, in Xournal, the combo box that controls layers). This is due to gdk_input_translate_coordinates() being called to process extended events on a GdkWindow for which impl_window->input_window is NULL. The same happens with other widgets too (e.g. the popup generated when moving selected text within a GtkTextView, again assuming the GtkTextView resides within an xinput-aware window).

2. if gdk_input_set_extension_events() is called to disable extension events while one is being processed, the interface becomes unresponsive to *all* pointer events, as display->ignore_core_events remains stubbornly set to 1... the code should check and reset display->ignore_core_events when a window stops being aware of extended events.

Denis

It¿'s been over a month since this issue was commented, I'd like to know what has been done. My tablet can't be used properly yet.

Created attachment 143621 [details] [review]
Fix x/y coordinates for extended events.

Extended events were being reported to virtual windows without taking into
account the position of the virtual window inside the native one.

Created attachment 143622 [details] [review]
Block extended events to windows that didn't select them.

Now XSelectExtensionEvent() is called on the native window, so there
may be virtual windows inside that shouldn't receive extended events.

Created attachment 143623 [details] [review]
Always report XInput events to the grab window if any.

The grab window should be the first option to send events to, else we may
get unpaired events, making display->ignore_core_events go crazy.

Those looked good to me, so i commited them all. Does this fix everyones problem?

Thanks, absolute devices work perfectly now.  However, I'm still having trouble with relative devices: They don't work at all -- at least on xserver master.  Could someone test this on a 1.6 xserver? I have a feeling this is due to device coordinates being reported differently between server versions.

(In reply to comment #13)
> Thanks, absolute devices work perfectly now.  However, I'm still having trouble
> with relative devices: They don't work at all -- at least on xserver master. 
> Could someone test this on a 1.6 xserver? I have a feeling this is due to
> device coordinates being reported differently between server versions.

In general I see the same behavior than non-csw GTK+ versions with a fairly recent Xorg git build, could you try running apps with GDK_NATIVE_WINDOWS=1 or linking to your system libg[dt]k+-2.0.so? That should help distinguish where the bug is

>       x_scale = gdk_screen_get_width (gdk_drawable_get_screen (window)) / device_width;
>       y_scale = gdk_screen_get_height (gdk_drawable_get_screen (window)) / device_height;
 
This is wrong.  If the device is relative, axis_min == axis_max == -1, so we devide by zero.  I haven't tested this, but I think scale should be 1 in this case.

Tom: The pre-csw code does exactly the same thing though...

(In reply to comment #16)
> Tom: The pre-csw code does exactly the same thing though...

I don't know how we've gotten away with this for so long, but this is clearly not correct.  Coordinate translation should be functionally equivalent to how the X server translates SD coordinate systems to MD ones (rescaleValuatorAxis in dix/getevents.c).

Created attachment 143764 [details] [review]
step in the right direction...

This fixes one of the issues with the current code, namely the fact that devices are allowed to set max_value == min_value == -1.

Recent versions of evdev send only the valuators that have changed since the last event, i.e. first_axis may be non-zero and axes_count may be less then num_axes.  In this case axis_data[i] represents the (i+first_axis)th valuator.

I've posted some more information about the problem here:

http://mail.gnome.org/archives/gtk-devel-list/2009-September/msg00055.html

I've also noticed another problem: with ENABLE_XINPUT_BUGFIX (which basically takes over coordinate translation from gtk) disabled, coordinates are off after scrolling.

Tom, in reply to your last comment: the issue you observe with ENABLE_XINPUT_BUGFIX disabled is a bug in xournal's code, not in GTK+.
I'll fix it very soon in the CVS.

Denis

(In reply to comment #20)
> Tom, in reply to your last comment: the issue you observe with
> ENABLE_XINPUT_BUGFIX disabled is a bug in xournal's code, not in GTK+.
> I'll fix it very soon in the CVS.
> 
> Denis

Thanks, I see.  Sorry about the noise.

Tom: I don't see how that patch can help? If width == 0 then we would have divided by zero before we reach the changed code, no?

(In reply to comment #22)
> Tom: I don't see how that patch can help? If width == 0 then we would have
> divided by zero before we reach the changed code, no?

Yes, but that's fine since we're not actually using x_scale.

Created attachment 143770 [details] [review]
Slightly improved patch

We still get crazy coordinates (I think this is due to XQueryDeviceState returning garbage on master), but at least I can draw in gimp with my track point now.

TODO: Bounds checks, always select motion events, and select and interpret DeviceState events.

Tom: How is that fine? doesn't the divide by zero cause a SIGFPE?

Created attachment 143944 [details] [review]
proposed patch

This patch implements strategy (b) from my mailing list post and should make gtk+ work with recent builds of the evdev driver.

Caveat: there seems to be a bug in xserver master (not sure about earlier versions) that prevents DeviceStatusNotify events from being sent.  This means that if you Alt+Tab into your window without and then click before moving the mouse, you'll get wrong coordinates for your click.  I think this is something we can live with.

You seem to be leaking axis_data.

If you want to correctly emulate button motion, don't you need to select for all button events too.

Emulation of GDK_POINTER_MOTION_HINT_MASK is a quite complex thing, and I'm not sure that the code there really does this right.

Emulation of GDK_BUTTON1_MOTION_MASK etc is wrong, I think. It doesn't only depend on the first button pressed. You need to keep track of the full bitmask of which buttons are down.

(In reply to comment #27)
> You seem to be leaking axis_data.
The existing code seems to be already leaking axes, so I thought I was fine.

> If you want to correctly emulate button motion, don't you need to select for
> all button events too.
Button events are always automatically selected (cf _gdk_input_select_events).

> Emulation of GDK_POINTER_MOTION_HINT_MASK is a quite complex thing, and I'm not
> sure that the code there really does this right.
To be honest, I'm not even sure what it is supposed to do.  My code just treats it as if it was GDK_POINTER_MOTION_MASK.

> Emulation of GDK_BUTTON1_MOTION_MASK etc is wrong, I think. It doesn't only
> depend on the first button pressed. You need to keep track of the full bitmask
> of which buttons are down.
I always assumed ButtonXMotion events behaved the same as grabs, which stay active until the last button is released, but I'll have to test that to make sure.

Created attachment 143972 [details] [review]
revised patch

You were correct.  ButtonXMask only cares about the logical state of button X.  This actually simplifies things a little bit.

axes is freed in gdkinput.c:gdk_device_dispose()

GDK_POINTER_MOTION_HINT_MASK is a hint that is combined with the other motion masks to limit the amount of motion events sent:

• PointerMotionHintMask
  If PointerMotionHintMask is selected in combination with one or more of the 
  above masks, the X server is free to send only one MotionNotify event (with 
  the is_hint member of the XPointerMovedEvent structure set to NotifyHint) to 
  the client for the event window, until either the key or button state changes,
  the pointer leaves the event window, or the client calls XQueryPointer or 
  XGetMotionEvents. The server still may send MotionNotify events without 
  is_hint set to NotifyHint.

Rather than doing anything that is not a full implementation of this i recommend to just strip out and ignore that flag. Its optional after all.

+      if ((gdkdev->button_state & 1 << 1) && (priv->extension_events & GDK_BUTTON1_MOTION_MASK))

Surely button state mask for button "1 << 0" (== 1), not "1 << 1" (== 2)

Created attachment 144007 [details] [review]
revised patch

(In reply to comment #30)
> axes is freed in gdkinput.c:gdk_device_dispose()
> 
> GDK_POINTER_MOTION_HINT_MASK is a hint that is combined with the other motion
> masks to limit the amount of motion events sent:
> 
> [...]
> 
> Rather than doing anything that is not a full implementation of this i
> recommend to just strip out and ignore that flag. Its optional after all.

That seems fair, especially since the functionality is gone in XI2 anyway.


(In reply to comment #31)
> +      if ((gdkdev->button_state & 1 << 1) && (priv->extension_events &
> GDK_BUTTON1_MOTION_MASK))
> 
> Surely button state mask for button "1 << 0" (== 1), not "1 << 1" (== 2)
No, this is the way the original code worked (gdkdev->button_state |= 1 << xdbe->button).  But you raise a good point:  XI allows up to 255 buttons, so button_state was to small to hold all the buttons anyway.

I don't understand how this can work with relative devices. 
I suppose those generally send zero for every axis except the ones the changed, and then only the delta since last event in those directions, right?

If so, how can we just take this value and add the x/y_offset (basically converting this delta to absolute root coords). Shouldn't we keep track of the
last position, and add the delta to that? And, why are we always treating relative devices like GDK_MODE_SCREEN? That doesn't seem right either, why can't?

Or, are we assuming that apps should know that a device is relative and expect events to work in a totally different way? Wouldn't it make more sense to have a relative mode then? Anyway, if we're doing this, should we really add x/y_offset to the relative axis data?

My code was based on my experience with X server 1.7.  I should have known that older X servers behave totally differently.  X server 1.6 does indeed send relative data in axis_data, so we have no choice but to make gtk's behavior dependent on the X server version.  If we get correct DeviceStateNotify events (I haven't tested this yet), we can indeed do what you suggest and just increment axis values by the delta.  But if we don't -- we're basically screwed and our best bet will be to use the outdated x_root and y_root values.

I figured GDK_MODE_WINDOW didn't make sense for relative devices, but maybe I'm wrong, this will be an easy fix once we've dealt with the more critical issues.

A realtive mode would be nice to have, but we'll have to wait for XI2 raw events (which also require grabs IIRC) for that since even the data that xserver-1.6 uses is subject to clipping.

I guess the only good news in all of this is that we're not breaking anything that used to work before.

I still don't understand
If you get a relative event with dx, dy we report an event at 
x = x_offset + dx;
y = y_offset + dy;

Where x/y_offset is the offset from the top left corner to the root window origin.

So, say the event window is 100x100 at position 100,100 in root window coords. Then we get an event with dx = 10, and dy = 0.

Given the above equations we report an event with coords x = -90 y = -100.

We then get another event with dx = 0 and dy = 10, so we report an event with coordinates x = -100 y = -90.

How is this useful, or am i missing something here?

You are correct on xserver-1.6, which sends events containing relative (clipped) valuators for relative devices.  xserver-1.7 always sends absolute coordinates, even for relative devices, so my code was working fine there.

Unfortunately, neither xserver version seems to send the appriopriate DeviceStateNotify events.

I have things working now under both xserver versions.  The trick is to realize that x_root and y_root always contain the state of the valuators (converted to screen coordinates) before the event took place.  If things don't match, use XDeviceQueryState to fetch updated valuator information for the device.

Created attachment 144235 [details] [review]
Use strstr instead of g_strrstr.

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Sorry, please ignore the last attachment. git-bz didn't do what I expected.

Created attachment 144236 [details] [review]
Refactor _gdk_input_other_event

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144237 [details] [review]
XInput allows up to 255 buttons

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144238 [details] [review]
Report XInput button motion events until all buttons are released.

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144239 [details] [review]
Always select all XInput motion events

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144240 [details] [review]
Filter out events that the current window didn't select for

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144241 [details] [review]
Keep track of axis values

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144242 [details] [review]
Select for DeviceStateNotify

Signed-off-by: Thomas Jaeger <ThJaeger@gmail.com>

Created attachment 144243 [details] [review]
Interpret min_value == max_value correctly

+  if (hypot(x_root - x_screen, y_root - y_screen) > 1.0)

There is generally no need to actually do the square-root when doing a comparison like this.

Also, i'd like to have a lot more comments about why we do things this way, since its not very obvious with version checks, comparisons to root_x/y and whatnot.

rescale_valuator_axis has some weird non-gtk+-style indentation

Also, is gdk_input_get_axes right for relative devices?
man XQueryDeviceState says:

Valuators on the device report 0 if they are reporting relative information, and the current value if they are reporting  absolute information.

But you pass in absolute=TRUE

am I right thinking in this as a "new feature"? AFAICS relative devices weren't ever meant to work properly before, and the patches are somewhat disruptive with the XInput2 work.

I've just filled bug #596725 in to track the XInput2 changes, perhaps this bug should be marked as dependent on that one?

Attachment 144236 [details] pushed as abf7742 - Refactor _gdk_input_other_event
Attachment 144237 [details] pushed as 2635fb7 - XInput allows up to 255 buttons
Attachment 144238 [details] pushed as acdecb6 - Report XInput button motion events until all buttons are released.
Attachment 144239 [details] pushed as 51f2a99 - Always select all XInput motion events
Attachment 144240 [details] pushed as 67728ac - Filter out events that the current window didn't select for

I commited the initial stuff, as that seemed obviously correct and the event filtering seems important.

I'm more hesitant with the relative events stuff. Its complex, version dependent and it never worked before, so I'm hesitant to land this when we're planning such a large rewrite with the xi2 stuff.

(In reply to comment #51)
> I commited the initial stuff, as that seemed obviously correct and the event
> filtering seems important.
> 
> I'm more hesitant with the relative events stuff. Its complex, version
> dependent and it never worked before, so I'm hesitant to land this when we're
> planning such a large rewrite with the xi2 stuff.

Let's drop the patch in comment #44 then, especially since relative devices will work fine on xserver-1.7 either way.

Created attachment 144297 [details] [review]
0001-Keep-track-of-axis-values.patch

This patch is still necessary to get correct behavior with the new evdev behavior.  There are absolute devices out there that are handled by evdev, and the current code will send wrong coordinates whenever there is horizontal or vertical motion.

Created attachment 144299 [details] [review]
0002-Select-for-DeviceStateNotify.patch

This patch doesn't have any practical implications, since I haven't actually managed to get the server send DeviceStateNotify events, but it's technically needed to get correct coordinates.

Created attachment 144303 [details] [review]
0003-Interpret-min_value-max_value-correctly.patch

I've rewritten this patch to make it less intrusive.  It only touches codepaths that were previously dividing by zero, so it's clearly safe and as an added benefit makes relative devices work on xserver-1.7.

(In reply to comment #49)
> am I right thinking in this as a "new feature"? AFAICS relative devices weren't
> ever meant to work properly before, and the patches are somewhat disruptive
> with the XInput2 work.
> 
> I've just filled bug #596725 in to track the XInput2 changes, perhaps this bug
> should be marked as dependent on that one?

It's more than just relative devices.  The biggest problem was that we now have devices (plenty of them) that don't send all their valuators in each event.  You'll face the same problem on the xi2 branch with absolute axes:  the valuator state needs to be cached on the client side since you can't just send zero if the device didn't report that particular valuator in the event.

I commited these. I guess we can mark this bug as fixed now? And open a new one about relative devices?

Unfortunately, the new behavior for relative devices in 2.18.1 is IMHO a regression. Previously a GTK+ application that listens to a relative device would receive events with coordinates (inf,inf) and it would be easy to discard those events or request correct coordinates by another mechanism. Now the events arrive at coordinates close to the upper-left corner of the root window (converted to window coordinates), which is much harder to weed out as "obviously wrong".

I'll open it as a new bug, since this is no longer the same topic.

Denis

please do so, I'm closing this one since its getting unwieldly and the core issue is fixed.