Add API for raw touch events (updated) #24610

dsa-t · 2024-06-13T21:15:19Z

Rebased version of #649

Tested with wxGTK on Linux Mint/X11 and with wxMSW on Windows 11 with a touch panel.

Includes wxMSW patch from https://groups.google.com/g/wx-dev/c/JdeuH95_bGQ/m/tcJCXMZUDgAJ

vadz

Thanks for bringing this up to date!

Globally this looks good to me and I'd definitely like to merge it for 3.3.0. I can address the minor comments I left myself, but if you could please do it, it would be even better, of course. I'd just like to ask you to use "fixup" commits (done by git commit --fixup) instead of force-pushing your changes, as this makes seeing what exactly has been changed more difficult.

The only relatively important question I see here concerns wxTouchEventBase: I'm just not sure if we really need it at all. It does save a tiny amount of duplication, but I don't think anybody is actually going to use it, i.e. handle gesture and raw touch events as objects of this class. Maybe we should remove it and just add position to wxMultiTouchEvent itself too?

Thanks again!

interface/wx/window.h

samples/touchtest/touchtest.cpp

vadz · 2024-06-15T15:38:18Z

samples/touchtest/touchtest.cpp

+    wxMenu *file_menu = new wxMenu;
+
+    file_menu->Append(wxID_EXIT);
+
+    wxMenuBar *menu_bar = new wxMenuBar;
+
+    menu_bar->Append(file_menu, wxT("&File"));
+    SetMenuBar(menu_bar);


This looks rather useless and could be just removed IMO.

It turned out to be useful to check if the pos is relative to the client rect, not the window rect.

Maybe some options can be added later to the menu.

vadz · 2024-06-15T15:48:49Z

src/gtk/window.cpp

+            event.SetSequenceId(wxTouchSequenceId(gdk_event->sequence));
+            event.SetPrimary(gdk_event->emulating_pointer);
+
+            win->GTKProcessEvent(event);


Is it correct to ignore its return value here?

Should we still call wxEmulateLeftDownEvent/wxEmulateMotionEvent/wxEmulateLeftUpEvent, even if the touch event was handled?

Note that MSW sends synthesized mouse events regardless, so it would be consistent with MSW.

vadz · 2024-06-15T15:49:46Z

src/msw/window.cpp

+                break;
+
+            HandleTouch(wParam, lParam);
+            // Let DefWindowProc handle the touch events too


It would be nice to explain why do we need it to always have them?

The value of processed doesn't appear to have any effect here.

If there is no good reason to not use the usual logic, I'd stick to it. I.e. just do

processed = HandleTouch(wParam, lParam);

src/msw/window.cpp

vadz · 2024-06-15T15:52:20Z

src/msw/window.cpp

+            event.SetPosition(pos);
+            event.SetSequenceId(wxTouchSequenceId(wxUIntToPtr(info[i].dwID + 1)));
+            event.SetPrimary( (info[i].dwFlags & TOUCHEVENTF_PRIMARY) != 0 );
+            HandleWindowEvent(event);


Shouldn't we remember if any of the events were processed and only return true in this case?

This also seems to be still relevant.

src/osx/cocoa/window.mm

dsa-t · 2024-06-15T18:30:00Z

Maybe we should remove it and just add position to wxMultiTouchEvent itself too?

Makes sense to me.

dsa-t · 2024-06-16T00:20:05Z

While a finger is held for too long on MSW, activating the "right mouse buton" gesture, the OS doesn't synthesizing mouse down/move events.

So might need to add a IsSynthesized method to mouse events.
The IsPrimary call does not help in this case.

Also in Qt, the primary touch point detection is not available.

dsa-t · 2024-06-16T16:44:30Z

Also in Qt, a nomousefromtouch windows platform parameter can be passed to skip these synthesized events at Qt level
(see https://doc.qt.io/qt-6/qguiapplication.html#platform-specific-arguments)

dsa-t · 2024-06-17T09:00:41Z

On my GTK system, the mouse pointer emulation code from #2539 doesn't seem to be needed.

If I return false from touch_callback, GTK sends the mouse events by itself.
If I return true, GTK doesn't send any emulated mouse events.

Current touch_callback has a void return type.

Could it be that the void function return value evaluated to something other than false in #19265?

dsa-t · 2024-06-17T09:30:43Z

Another question is if the user app has handled the touch event, should we still emit mouse events synthesized by the OS or block them?

dsa-t · 2024-06-17T09:32:33Z

It might be simplest to emit the synthesized mouse events anyway.

dsa-t · 2024-06-17T10:06:14Z

If we return false in touch_callback, the event order is interesting:

OnMouseMove leftdown 0 pos 224 439

touch_callback Emulating 1 widget 0x555555859120 dev 0x555555634820 dev 0 

OnMouseMove leftdown 1 pos 224 439
OnMouseDown leftdown 1 pos 224 439

touch_callback Emulating 1 widget 0x555555859120 dev 0x555555634820 dev 0 

OnMouseUp leftdown 0 pos 224 439

There's a OnMouseMove with LeftIsDown() == true coming before OnMouseDown.

vadz · 2024-07-06T23:15:24Z

@dsa-t Do you plan to make more changes here?

dsa-t · 2024-07-07T05:58:30Z

Another questions are:

Should we use floating point for precise positioning on high-DPI screens?
Maybe get rid of wxTouchSequenceId and use int, like Qt does?

vadz · 2024-07-07T13:17:57Z

* Should we use floating point for precise positioning on high-DPI screens?

What do the different OS provide to us, i.e. what type of coordinates they use?

* Maybe get rid of `wxTouchSequenceId` and use `int`, like [Qt does](https://doc.qt.io/qt-6/qeventpoint.html#id-prop)?

I'm always in favour of using opaque types if possible. I'm still not sure what exactly is wxTouchSequenceId for, i.e. how will it be used, but it doesn't seem like it should be just a plain integer.

dsa-t · 2024-07-07T17:08:25Z

What do the different OS provide to us, i.e. what type of coordinates they use?

Logical pixels == DIPs on wxQt, so it would be most noticable on smartphones.

I'm still not sure what exactly is wxTouchSequenceId for, i.e. how will it be used, but it doesn't seem like it should be just a plain integer.

It's an arbitrary value to identify touch points. On wxMSW it's something like:

0x0000000000003763
0x0000000000003764
0x0000000000003765
0x0000000000003766
0x0000000000003767
0x0000000000003768
0x0000000000003769
0x000000000000376a
0x000000000000376b
0x000000000000376c

on wxQt on Windows it's something like:

0x0000000002000001
0x0000000002000002
0x0000000002000003
0x0000000002000004
0x0000000002000005
0x0000000002000006
0x0000000002000007
0x0000000002000008
0x0000000002000009
0x000000000200000a

On wxGTK, it's a GdkEventSequence* (https://docs.gtk.org/gdk3/struct.EventTouch.html), but it's value is usually not a real pointer - it acts as an int.

vadz · 2024-07-07T17:25:37Z

What do the different OS provide to us, i.e. what type of coordinates they use?

Logical pixels == DIPs on wxQt, so it would be most noticable on smartphones.

Sorry, what I meant was whether it was integer or floating point?

I'm still not sure what exactly is wxTouchSequenceId for, i.e. how will it be used, but it doesn't seem like it should be just a plain integer.

It's an arbitrary value to identify touch points.

Thanks for the explanation and the examples. If we really can't do anything with it, except comparing them, presumably, it should remain opaque and allow doing just this (and copying).

dsa-t · 2024-07-07T17:30:40Z

Sorry, what I meant was whether it was integer or floating point?

on GTK, it's

  gdouble x;
  gdouble y;

on Qt, it's qreal aka double in QPointF:

QPointF TouchPoint::screenPos() const

on OSX, it's also a floating point type what wx rounds to int:

https://developer.apple.com/documentation/appkit/nstouch/1534031-normalizedposition?language=objc

vadz · 2024-07-07T17:32:56Z

In this case I agree that we should use doubles in wx API too.

Some ports do not support drawing onto a wxClientDC.

Co-authored-by: VZ <vz-github@zeitlins.org>

Allows the user application to distinguish between real mouse events and events synthesized from touch events by the OS.

Some ports may not set this flag.

dsa-t · 2024-07-14T07:28:25Z

Changed position type to wxPoint2DDouble.
The way ScreenToClient is used is not great.
Should ScreenToClient be implemented for floatinig point separately, and is there other suggestions?

CI Error in Cirrus CI / FreeBSD wxGTK 3 seems unrelated.

dsa-t · 2024-07-14T07:33:05Z

wxMouseEvent positions are floating point in GTK, Qt and OSX.
Should wx also use floating point types for these?

vadz

Thanks for the updates, I think this is pretty close to being merged, but I'd like to understand why do we need to pass the extra argument everywhere in the MSW code instead of just getting it when needed. Do we really have to complicate things like this?

vadz · 2024-07-16T18:02:55Z

include/wx/event.h

+class wxTouchSequenceId : public wxItemId<void*>
+{
+public:
+    wxTouchSequenceId() : wxItemId<void*>() { }


Minor, but why not

Suggested change

wxTouchSequenceId() : wxItemId<void*>() { }

wxTouchSequenceId() = default;

?

vadz · 2024-07-16T18:06:44Z

include/wx/event.h

+    explicit wxTouchSequenceId(void* pItem) : wxItemId<void*>(pItem) { }
+};
+
+class WXDLLIMPEXP_CORE wxMultiTouchEvent : public wxEvent


I'm a bit surprised that this class is not associated with (e.g. by inheriting from it) wxKeyboardState. Couldn't we have special handling for some touch events when Control (or Shift or whatever) is pressed etc?

vadz · 2024-07-16T18:09:35Z

include/wx/geometry.h

+    inline wxPoint GetFloor() const;
+    inline wxPoint GetRounded() const;


Should be added to interface/wx/geometry.h too (which is quite empty, but still not completely useless, I guess...).

vadz · 2024-07-16T20:02:22Z

include/wx/osx/cocoa/private.h

@@ -61,10 +61,10 @@ class wxWidgetCocoaNativeKeyDownSuspender
 public:
    // stops sending keydown events for text inserted into this widget
    explicit wxWidgetCocoaNativeKeyDownSuspender(wxWidgetCocoaImpl *target);
-    
+


It would be really great to avoid mixing whitespace-only and significant changes. Please configure your editor/IDE to avoid automatically stripping existing whitespace on save.

vadz · 2024-07-16T20:03:01Z

include/wx/msw/window.h

@@ -368,10 +368,10 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase

    bool HandleDropFiles(WXWPARAM wParam);

-    bool HandleMouseEvent(WXUINT msg, int x, int y, WXUINT flags);
-    bool HandleMouseMove(int x, int y, WXUINT flags);
+    bool HandleMouseEvent(WXUINT msg, int x, int y, WXUINT flags, WXLPARAM extraInfo);


It's a bit annoying to pass this extraInfo everywhere, can't we just call ::GetMessageExtraInfo() when it's needed?

It's nice to consistently pass just the WPARAM/LPARAM to HandleXXX() functions.

vadz · 2024-07-16T22:17:21Z

src/msw/textctrl.cpp

@@ -2809,7 +2809,7 @@ bool wxTextCtrl::MSWOnNotify(int idCtrl, WXLPARAM lParam, WXLPARAM *result)
                    int x = GET_X_LPARAM(msgf->lParam),
                        y = GET_Y_LPARAM(msgf->lParam);

-                    HandleMouseEvent(msg, x, y, flags);
+                    HandleMouseEvent(msg, x, y, flags, ::GetMessageExtraInfo());


These changes could be avoided if we called ::GetMessageExtraInfo() in HandleMouseEvent() itself too.

vadz · 2024-07-16T22:18:40Z

src/osx/cocoa/window.mm

@@ -28,6 +28,7 @@
 #include "wx/private/bmpbndl.h"

 #include "wx/evtloop.h"
+#include "wx/vector.h"


We can just use <vector> on master.

vadz · 2024-07-16T22:21:52Z

src/msw/window.cpp

@@ -236,6 +236,27 @@ class GestureFuncs
        return ms_pfnSetGestureConfig;
    }

+    typedef BOOL (WINAPI *RegisterTouchWindow_t)(HWND,ULONG);


Actually we shouldn't need to load these functions dynamically any longer, they're all available since Windows 7 and we don't support anything earlier (or even it) any longer. If you prefer, we can keep this code as is for this PR, but we could (and should) just remove all this dynamic loading stuff later.

vadz · 2024-07-16T22:25:32Z

src/msw/window.cpp

+                break;
+
+            HandleTouch(wParam, lParam);
+            // Let DefWindowProc handle the touch events too


If there is no good reason to not use the usual logic, I'd stick to it. I.e. just do

processed = HandleTouch(wParam, lParam);

vadz · 2024-07-16T22:26:25Z

src/msw/window.cpp

+            event.SetPosition(pos);
+            event.SetSequenceId(wxTouchSequenceId(wxUIntToPtr(info[i].dwID + 1)));
+            event.SetPrimary( (info[i].dwFlags & TOUCHEVENTF_PRIMARY) != 0 );
+            HandleWindowEvent(event);


This also seems to be still relevant.

vadz · 2024-07-16T22:30:57Z

wxMouseEvent positions are floating point in GTK, Qt and OSX. Should wx also use floating point types for these?

I guess we could add wxPoint2DDouble m_pos to wxMouseState but we'd still have to keep m_x and m_y for compatibility and ensure they're always synchronized and I'm not sure if it's worth the complexity — considering how (im)precise the mouse/gestures are, it probably doesn't make much difference in practice unless you use a high-precision tablet. Do you notice any problems due to rounding when testing this?

The way ScreenToClient is used is not great.
Should ScreenToClient be implemented for floatinig point separately

Yes, I think it could be nice to add an overload using doubles.

CI Error in Cirrus CI / FreeBSD wxGTK 3 seems unrelated.

Yes, it was already fixed in master.

dsa-t force-pushed the raw-touch-1 branch 4 times, most recently from a8a1e74 to a7550a9 Compare June 14, 2024 16:19

vadz mentioned this pull request Jun 15, 2024

Add API for raw touch events (see #17077) #649

Closed

vadz reviewed Jun 15, 2024

View reviewed changes

vadz added this to the 3.3.0 milestone Jun 15, 2024

vadz mentioned this pull request Jun 19, 2024

touch zoom doesn’t work on bookworm with X11 #24623

Open

dsa-t force-pushed the raw-touch-1 branch from 2a8e7f1 to c2d7367 Compare July 7, 2024 02:03

e9925248 and others added 7 commits July 13, 2024 17:43

Implement API for raw touch events

3796a4c

GTK3 MulttTouch events

7f5247a

MSW multi-touch events

7505698

OS X raw touch events

a001ed6

Add touchtest sample for raw touch events.

9b1db7a

Add touchtest sample to CMake.

9b4161a

Fix raw touch events for GTK.

d310b90

dsa-t and others added 20 commits July 13, 2024 17:43

touchtest: paint onto a wxBitmap first.

7f657d3

Some ports do not support drawing onto a wxClientDC.

Apply suggestions

ded6cfe

Co-authored-by: VZ <vz-github@zeitlins.org>

Remove trailing whitespace.

8ca8d54

Use range-based loop in wxWindowMSW::HandleTouch.

c4cc6d8

Use non-deprecated macros in touchtest.

9e2e1c1

touchtest: fix flickering on MSW.

b66c5ec

touchtest: increase default frame size.

ac2b71a

Implement wxMouseEvent::IsFromTouch.

8200ba8

Allows the user application to distinguish between real mouse events and events synthesized from touch events by the OS.

touchtest: use special drawing style for mouse pointer.

d107bd7

touchtest: use special value for mouse id.

401c3bf

touchtest: ignore IsPrimary flag.

fa1bce6

Some ports may not set this flag.

GTK: return a value from touch_callback.

157adce

Rename IsFromTouch to IsSynthesized.

a7cccd3

wxQt: implement wxMultiTouchEvent events.

1a14b92

Set synthesized flag for mouse events synthesized by wx on wxGTK.

9e12764

Fix build on GTK2.

9742b5e

wxQt: compute client position for multitouch events.

2cf0472

Remove wxT() from touchtest.

787e585

Use wxEvent instead of wxTouchEventBase.

69cf179

Fix wxTouchSequenceId interface description.

188dacf

dsa-t force-pushed the raw-touch-1 branch from 94340b0 to 188dacf Compare July 13, 2024 14:44

dsa-t added 5 commits July 13, 2024 21:02

Use wxPoint2DDouble for wxMultiTouchEvent position.

23e3902

Use wxPoint2DDouble in touchtest.

dae7cb8

Fix iPhone wxFromNSPointF.

8ec5240

Fix OSX definition.

174f0d4

Fix OSX build.

4252926

vadz reviewed Jul 16, 2024

View reviewed changes

	wxTouchSequenceId() : wxItemId<void*>() { }
	wxTouchSequenceId() = default;

		inline wxPoint GetFloor() const;
		inline wxPoint GetRounded() const;

Add API for raw touch events (updated) #24610

Are you sure you want to change the base?

Add API for raw touch events (updated) #24610

Conversation

dsa-t commented Jun 13, 2024 • edited Loading

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsa-t Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsa-t commented Jun 15, 2024

dsa-t commented Jun 16, 2024 • edited Loading

dsa-t commented Jun 16, 2024

dsa-t commented Jun 17, 2024

dsa-t commented Jun 17, 2024

dsa-t commented Jun 17, 2024

dsa-t commented Jun 17, 2024

vadz commented Jul 6, 2024

dsa-t commented Jul 7, 2024 • edited Loading

vadz commented Jul 7, 2024

dsa-t commented Jul 7, 2024 • edited Loading

vadz commented Jul 7, 2024

dsa-t commented Jul 7, 2024 • edited Loading

vadz commented Jul 7, 2024

dsa-t commented Jul 14, 2024

dsa-t commented Jul 14, 2024

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadz commented Jul 16, 2024

dsa-t commented Jun 13, 2024 •

edited

Loading

dsa-t Jun 21, 2024 •

edited

Loading

dsa-t commented Jun 16, 2024 •

edited

Loading

dsa-t commented Jul 7, 2024 •

edited

Loading

dsa-t commented Jul 7, 2024 •

edited

Loading

dsa-t commented Jul 7, 2024 •

edited

Loading