Can you split the patch where you fix in one patch the pressure problems and in the other you add the touchscreen support?

Hi,

Plus, since this is probable for inclusion in 3.x, we need to insist that this code will build on Visual Studio 2008 (especially this is for Windows).  So, can you:

-Declare variables at the top of the block (please no mid-block declarations)
-Not use VLAs (this applies even for 3.89.x+).  Use a #define instead of a
 const int for an array length.

Thanks very much anyways for looking into this.

---
Hi Nacho,

Do you think we want to drop Vista support at this point?  This patch will mean the minimum Windows version that GTK+-3.22.x will be at Windows 7, if this goes in with #define _WIN32_WINNT 0x0601.

With blessings, thank you, and cheers!

---

Happy New Year and belated Merry Christmas!

Created attachment 342568 [details] [review]
revised patch (separated out Wintab changes, allow it to compile with Microsoft compiler)

Thanks for the quick feedback, Chun-wei and Ignacio.

I've uploaded a new patch:
*moved Wintab fix to bug 776572.
*allow code to be built with Microsoft compiler - I use Visual Studio all the time, but I don't have time to get all the dependencies of GTK+ to do a test build myself.

Also, I hard coded the capabilities for the tablet device. I couldn't find a native API for querying touchscreen devices. There was one for .NET (managed) I think.
 
"Do you think we want to drop Vista support at this point?"
I don't know anyone still using Vista, but maybe setting _WIN32_WINNT = 0x0601 won't actually cause a dependence on newer functions as long as you don't use them?

Nevermind about claiming to not use any Windows 7 functions. I do use RegisterTouchWindow().

I suppose if you want to keep Vista support, then just change it to get the function pointer dynamically like how the WinTab code does.

Hi Yale,

(Please note that I have not yet tested your new patch, but fixed the original to build on 2008), but few more things to note by a quick look in your new patch:

-There is still the use of lrint().  Do you think it is alright
 to use rint(), and copy the rint() implementation with the
 #ifndef HAVE_RINT ... #endif from gtk/fallback-c89.c into
 gdk/fallback-c89.c, and include math.h and gdk/fallback-c89.c
 in gdkevents-win32.c, since I think we are already cutting short from a
 double to a long whichever way.

-You will need to factor in HiDPI support--see the uses of
 impl->window_scale elsewhere in the code in gdk/win32, as on HiDPI
 displays the touch event is only registered for the top left hand
 quarter of the window.  Let me know if you don't have HiDPI displays
 to test this part.  This will probably apply to your fixes to the wintab
 code, but sorry I can't help with that as I don't have a wintab device.

-Use guint64 instead of uint64_t and stdint.h

Thanks though!  With blessings!

Review of attachment 342568 [details] [review]:

I am no win32 backend developer, so can't comment much on bumping version requirements, but if this is meant to target gtk3, I guess some dynamic lookup is due, as it's probably not ok to do it between 3.22.x stable releases.

The logic in the patch looks right to me, it has some code style issues wrt indenting and curly braces, but I will let the win32 backend maintainers to raise the nits there. I've got some additional comments though.

::: gtk+-3.22.5.orig/gdk/win32/gdkdevicemanager-win32.c
@@ +767,3 @@
 		    GDK_DEVICE_TYPE_SLAVE);
+
+  device_manager->touchscreen = create_touchscreen(device_manager,

It's indeed unfortunate if you can't detect touchscreen(s) availability, I guess we can live with an entirely silent device where there's no touchscreen though, that's already the case for other backends.

::: gtk+-3.22.5.orig/gdk/win32/gdkevents-win32.c
@@ +2118,3 @@
+        event->time = p->dwTime;
+        event->window = window;
+        event->state = event->type == GDK_TOUCH_END ? 0 : GDK_BUTTON1_MASK;

This is not right. event->state contains the modifier state at the time the event happened, that means BUTTON1_MASK is not set yet on GDK_TOUCH_BEGIN, and is still set on GDK_TOUCH_END.

For full correctness, the current keyboard state should be also looked up and set here.

::: gtk+-3.22.5.orig/tests/testinput.c
@@ +272,3 @@
+  // Microsoft printf doesn't recognize %z for sequence, so cast to ll
+  g_printf("touch type=%d seq=%llu ev=(%f %f) axes=(%f %f) state=%x emu=%d\n", event->type, (uint64_t)event->sequence, event->x, event->y, event->axes[0], event->axes[1], event->state, event->emulating_pointer);
+  return FALSE;    // return false so that default handler, gtk_widget_real_touch_event() is called. If not called, then touch events won't get translated to emulated mouse events

This should be already the implicit behavior, with the backend changes you should be already able to do 1 finger painting on testinput.

Did you add this for correctness? or you needed it to get the right behavior? that might imply there's an issue somewhere else.

If it's for correctness, I'd say don't bother with modifying testinput, it's pretty much stale already. There's in gtk3-demo a couple of demos ("gestures" and "touch and drawing tablets") that may help ensure this works alright.

Created attachment 342594 [details] [review]
revision 2

Chun-wei, I haven't heard of HiDPI scaling until now. My Lenovo P40 has a regular 1920x1080 screen with scale=150%. I do see the icons getting bigger compared to 100%, but I don't understand how that works if the GDK scaling factor is only limited to integers?

The new patch divides by the scale now. testinput & gtk3-demo work as before, but I'm not sure about the correctness since I just copied the logic from another place. I don't like all the expensive divides. It should be replaced with a floating point multiply.

I replaced lrint() with round which is available in the fallbacks. Theoretically, lrintf can be done in a single instruction while (int)round(x) would need 2 instructions.

Can you guys modernize the rounding functions. I see both round() and rint(), with rint() having the better, less biased IEEE round-to-even for the 0.5 case. 

But even this can be done in a single instruction:
inline int RoundToNearest(float x)
{
    // warning: assumes processor's rounding mode is set to nearest int
#if defined(__SSE__) && defined(__GNUC__)
    // very clever version that avoids conversion of float to SSE vector
    int rounded;
    __asm__ (
             "cvtss2si %1, %0\n"  // most compilers are smart enough to convert this to vcvtss2si to avoid expensive AVX-SSE transition penalties
             : "=r"(rounded)
             : "x"(x)
            );
    return rounded;
#elif defined(__SSE__)
    return _mm_cvtss_si32(_mm_set_ss(x));
#else
}

Carlos,

I've added the CTRL, shift, etc. modifiers to the event state. I don't know if the state should be prior to the event or after. I just duplicated the XInput2 behavior on Linux, which is the state after.

I just checked the XInput2 documentation again and it seems to agree with what you're saying:

"A TouchEnd event generates a pointer motion event to the location of the touch
  and/or to update the axis values if either have changed, followed by a button
  release event for button 1. The button state as seen from the protocol
  includes button 1 set."

(In reply to yzhang1985 from comment #9)
> Carlos,
> 
> I've added the CTRL, shift, etc. modifiers to the event state. I don't know
> if the state should be prior to the event or after. I just duplicated the
> XInput2 behavior on Linux, which is the state after.

On closer inspection, this seems to be the case of key modifiers on the wayland backend, and is a bug. Both the pointer button modifiers code paths in the wayland backend and the x11 backend in its entirety behaves according to the same semantics that GDK inherited from X11.

If you find other places where this is happening, please file bugs.

Needless to say, this patch needs updating :).

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/729.