Added support for tablet events in MSW (PR #26223)
Iulian Serbanoiu
I finished the implementation related to tablet events for Windows.
The sample if working fine and takes into account pressure and eraser features. My tablet doesn't have tilt, rotation to test this. Any feedback is welcome.
Tested on Windows 11.
Don't know exactly if the documentation from inteface is proper, I gave my best try in copy pasting from other places and adapt it.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26223
Commit Summary
- 365bbb3 Added the wxTabletEvent handling pen events - for graphical tablets
- cea43de Fixed the missing break that would make all pointer events a pointer update
- 387d307 sample code in 'tabletpen' folder to show tablet capabilities
- 2c432c4 updated library to make use of the eraser functionality
- 5a73236 updated the documentation of the newly added interface
- fa061b8 Merge branch 'wxWidgets:master' into master
File Changes
(17 files)
- M include/wx/event.h (62)
- M include/wx/msw/window.h (2)
- M interface/wx/event.h (82)
- M samples/makefile.gcc (8)
- M samples/makefile.vc (10)
- M samples/samples.bkl (1)
- M samples/samples_vc17.sln (19)
- A samples/tabletpen/Makefile.in (203)
- A samples/tabletpen/makefile.gcc (233)
- A samples/tabletpen/makefile.unx (100)
- A samples/tabletpen/makefile.vc (444)
- A samples/tabletpen/tabletpen.bkl (12)
- A samples/tabletpen/tabletpen.cpp (252)
- A samples/tabletpen/tabletpen.vcxproj (450)
- A samples/tabletpen/tabletpen.vcxproj.filters (23)
- M src/common/event.cpp (6)
- M src/msw/window.cpp (80)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26223.patch
- https://github.com/wxWidgets/wxWidgets/pull/26223.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver pushed 1 commit.
- 435eaad fixed the pipeline reported issue in the documentation
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver pushed 1 commit.
- fb1b14c fixed warning treated as error during pipeline execution - unused variable
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver pushed 1 commit.
- 0ad9eff yet another unused variable warning treated as error
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this is a good start, but I have a couple of global questions in addition to the small remarks below.
- Terminology: is the word "tablet" actually clear? Windows doesn't seem to use it (it uses only "pointer" AFAICS), GTK seems to use "pad" (but I'm not even sure if it's for the same thing) and Cocoa also seems to use "pointing device". So is it going to be really clear what this is about or should we, perhaps, do a global rename of "Tablet" to "Pointer" or something else?
- I'm not sure if I had already mentioned this and sorry if I didn't, but I'd like to avoid another sample just for this, especially considering that currently it's completely useless under non-Windows systems. I think it would be better to add another frame to either
eventortouchtestsample instead.
> @@ -2203,6 +2209,50 @@ class WXDLLIMPEXP_CORE wxPressAndTapEvent : public wxGestureEvent
wxDECLARE_DYNAMIC_CLASS_NO_ASSIGN(wxPressAndTapEvent);
};
+class WXDLLIMPEXP_CORE wxTabletEvent : public wxEvent
+{
+public:
+ wxTabletEvent(wxWindowID winid = 0, wxEventType type = wxEVT_NULL)
+ : wxEvent(winid, type), m_pressure(-1.0), m_tiltX(-360.0), m_tiltY(-360.0), m_rotation(-1.0), m_pos(-1,-1), m_usingEraser(false)
Minor, but we should really initialize members in their declarations in the new code (this is not done in the vast majority of existing code because it predates C++11 by a couple of decades).
> + void SetRotation(wxDouble r) { m_rotation = r; }
+
+ const wxPoint& GetPosition() const { return m_pos; }
+ void SetPosition(const wxPoint& pos) { m_pos = pos; }
+
+ bool IsUsingEraser() const { return m_usingEraser; }
+ void SetUsingEraser(bool eraser) { m_usingEraser = eraser; }
+
+ wxNODISCARD virtual wxEvent* Clone() const override { return new wxTabletEvent(*this); }
+
+protected:
+ wxDouble m_pressure; // range [ 0.0, 1.0 ]
+ wxDouble m_tiltX; // range [ -90.0, 90.0 ]
+ wxDouble m_tiltY; // range [ -90.0, 90.0 ]
+ wxDouble m_rotation; // range [ 0, 360.0 ]
+ wxPoint m_pos; // position in pixels relative to the Window receiving the event
It would be useful say if this is in client or window coordinates?
> +
+ wxDouble GetTiltY() const { return m_tiltY; }
+ void SetTiltY(wxDouble t) { m_tiltY = t; }
+
+ wxDouble GetRotation() const { return m_rotation; }
+ void SetRotation(wxDouble r) { m_rotation = r; }
+
+ const wxPoint& GetPosition() const { return m_pos; }
+ void SetPosition(const wxPoint& pos) { m_pos = pos; }
+
+ bool IsUsingEraser() const { return m_usingEraser; }
+ void SetUsingEraser(bool eraser) { m_usingEraser = eraser; }
+
+ wxNODISCARD virtual wxEvent* Clone() const override { return new wxTabletEvent(*this); }
+
+protected:
I would make this private, this class is not really supposed to be inherited from anyhow, is it?
> + This event gives the application the chance to perform specific tablet events
+ processing based on the current position of the pen, pressure and other
+ parameters.
+
+ @beginEventTable{wxTabletEvent}
+ @event{EVT_POINTER_DOWN(func)}
+ Process a @c wxEVT_POINTER_DOWN event.
+ @event{EVT_POINTER_UP(func)}
+ Process a @c wxEVT_POINTER_UP event.
+ @event{EVT_POINTER_UPDATE(func)}
+ Process a @c wxEVT_POINTER_UPDATE event.
+ @endEventTable
+
+ @library{wxcore}
+ @category{events}
+ */
Please add this:
⬇️ Suggested change- */ + + @since 3.3.2 + */
> @@ -5159,6 +5159,88 @@ class wxSetCursorEvent : public wxEvent
*/
void SetCursor(const wxCursor& cursor);
};
+/**
+ @class wxTabletEvent
+
+ A wxTabletEvent is generated from wxWindow when the graphical pen is used.
+
+ This event gives the application the chance to perform specific tablet events
+ processing based on the current position of the pen, pressure and other
+ parameters.
+
+ @beginEventTable{wxTabletEvent}
+ @event{EVT_POINTER_DOWN(func)}
+ Process a @c wxEVT_POINTER_DOWN event.
We really need to have some explanation of what this event corresponds to because I am honestly not even sure myself. Is it tablet button event? Tablet stylus being lifted event? Something else?
The same goes for the 2 other events below, of course.
> + /** + Set the pressure value. + */ + void SetPressure(wxDouble p);
I'm not really sure we want to document all the SetXXX() functions, aren't they all supposed to be used only by wxWidgets itself?
We need to make the same changes in all the other samples_vcNN.sln files too if we add a new sample.
> +{
+ wxUnusedVar(lParam);
+ wxEventType type;
+ switch(message)
+ {
+ case WM_POINTERDOWN:
+ type = wxEVT_POINTER_DOWN;
+ break;
+ case WM_POINTERUP:
+ type = wxEVT_POINTER_UP;
+ break;
+ case WM_POINTERUPDATE:
+ type = wxEVT_POINTER_UPDATE;
+ break;
+ default:
+ wxLogLastError(wxT("Unexpected pointer message"));
Use of wxLogLastError() is incorrect here, there is no last error (::GetLastError()) as this is a logic bug in wx code, not a Win32 error. I would either remove it entirely or replace it with `wxFAIL_MSG("Unknown pointer message")``
> @@ -6338,6 +6344,81 @@ bool wxWindowMSW::HandleTouch(WXWPARAM wParam, WXLPARAM lParam)
return allHandled;
}
+bool wxWindowMSW::HandlePointer(WXUINT message, WXWPARAM wParam, WXLPARAM lParam)
+{
+ wxUnusedVar(lParam);
+ wxEventType type;
+ switch(message)
This is minor, of course, but it would be nice to format all new code consistently with the existing coding style, i.e.
⬇️ Suggested change- switch(message) + switch ( message )
> + bool handled = HandleWindowEvent(event); + return handled;
Very minor, but why do this instead of
⬇️ Suggested change- bool handled = HandleWindowEvent(event); - return handled; + return HandleWindowEvent(event);
?
> + type = wxEVT_POINTER_DOWN;
+ break;
+ case WM_POINTERUP:
+ type = wxEVT_POINTER_UP;
+ break;
+ case WM_POINTERUPDATE:
+ type = wxEVT_POINTER_UPDATE;
+ break;
+ default:
+ wxLogLastError(wxT("Unexpected pointer message"));
+ return false;
+ }
+
+ const UINT32 pointerId = GET_POINTERID_WPARAM(wParam);
+ POINTER_INPUT_TYPE pType = 0;
+ if (GetPointerType(pointerId, &pType))
We typically use explicit scope for Win32 functions to make it easier to distinguish them visually:
⬇️ Suggested change- if (GetPointerType(pointerId, &pType)) + if ( ::GetPointerType(pointerId, &pType) )
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
undergraver left a comment (wxWidgets/wxWidgets#26223)
- Terminology: is the word "tablet" actually clear?
Based on my investigation on both GTK https://docs.gtk.org/gdk3/enum.InputSource.html and Windows https://learn.microsoft.com/en-us/windows/win32/api/winuser/ne-winuser-tagpointer_input_type I believe that "tablet" is the right choice since I am filtering only messages from PT_PEN pointer type and the event information is held in a POINTER_PEN_INFO structure ( https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-pointer_pen_info ).
Of course, another solution would be to use the pointer terminology but I consider it too confusing.
- .... I think it would be better to add another frame to either
eventortouchtestsample instead.
I am not against this approach; personally I found out the touchtest sample way too confusing for a copy paste user that wants something ready to modify. Based on recent experiences I would say that users want to go "straight to business" without filtering parts of source code to extract their desired code excerpt.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#26223)Stylus?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver pushed 1 commit.
- 30b95f1 resolving review remarks
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver commented on this pull request.
I don't have the means to test this unfortunately.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
undergraver left a comment (wxWidgets/wxWidgets#26223)Stylus?
I would consider it as viable.
I personally would also think of wxPenEvent because "pen" seems to be found in both gtk/msw documentation.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
It's ok to change them blindly, they're almost identical to the MSVS 2022 version anyhow. But, of course, not adding a new sample would obviate this :-)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26223)
- Terminology: is the word "tablet" actually clear?
Based on my investigation on both GTK https://docs.gtk.org/gdk3/enum.InputSource.html and Windows https://learn.microsoft.com/en-us/windows/win32/api/winuser/ne-winuser-tagpointer_input_type I believe that "tablet" is the right choice since I am filtering only messages from PT_PEN pointer type and the event information is held in a
POINTER_PEN_INFOstructure ( https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-pointer_pen_info ).Of course, another solution would be to use the
pointerterminology but I consider it too confusing.
I agree, and wxPenEvent is also confusing because there is wxPen which has nothing to do with this. But I do like Robert's suggestion, so why not wxStylusEvent?
- .... I think it would be better to add another frame to either
eventortouchtestsample instead.I am not against this approach; personally I found out the
touchtestsample way too confusing for a copy paste user that wants something ready to modify. Based on recent experiences I would say that users want to go "straight to business" without filtering parts of source code to extract their desired code excerpt.
This is true, but OTOH it's good to avoid having too many samples. I think having all tablet/stylus-related code in a single frame (shown by a menu command from the main one) would be acceptable.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
undergraver left a comment (wxWidgets/wxWidgets#26223)This is true, but OTOH it's good to avoid having too many samples. I think having all tablet/stylus-related code in a single frame (shown by a menu command from the main one) would be acceptable.
Alright, I agree then with wxStylusEvent but I would still like to have a separate sample.
Related to .sln files. Couldn't we generate them somehow with cmake or similar stuff, I find it dreadful to edit .sln files?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26223)Alright, I agree then with wxStylusEvent
Just in case this advice might be useful: the way I do such global renames in new code is to make a patch (git format-patch or just git diff), load it into a text editor and do a search and replace on all new lines (in Vim with Abolish extension it's just %g/^+/S/Table/Stylus/g), then reset and apply the changed patch (git am or git apply). It's simpler/faster than doing it in all of the files.
but I would still like to have a separate sample.
This is not a hill I'm going to die on, but it definitely feels strange to have a sample doing absolutely nothing on the platforms which don't implement support for generating these events.
BTW, we probably should define wxHAS_STYLUS_EVENTS for the platforms that do support them, to avoid programs using __WXMSW__ for testing for this.
Related to .sln files. Couldn't we generate them somehow with
cmakeor similar stuff, I find it dreadful to edit .sln files?
It's not much fun but I don't know of any tool that would be able to generate what we want, i.e. using the current structure, without making things even worse.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Iulian Serbanoiu
@undergraver pushed 1 commit.
- e43ceb5 renamed wxTabletEvent to wxStylusEvent - phase 1
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.