Skip to content

Conversation

@CodeDoctorDE
Copy link

@CodeDoctorDE CodeDoctorDE commented Mar 17, 2025

This is my attempt for adding support for stylus on windows.
It's work in progress! I post it here to indicate that I'm working on it and that the same work is not done twice

I'm working on the flutter_window.cpp file. Maybe I need to touch some other code to allow additional information like pressure, rotation and more.

Would fix #102836 and #65248

There should be no breaking change other than stylus would be reported as stylus instead of touch.

Tested with flutter master on my test project:
image

https://github.com/CodeDoctorDE/flutter-input-demo

Tests are currently not there, I will add them next.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added engine flutter/engine related. See also e: labels. platform-windows Building on or for Windows specifically a: desktop Running on desktop labels Mar 17, 2025
@CodeDoctorDE
Copy link
Author

Looks like we can also identify hovering, I will work on this next

@CodeDoctorDE
Copy link
Author

Dont know why mac and linux ci fails, but my changes doesnt break any windows test so it looks good. Now i need to add my own tests

@Piinks
Copy link
Contributor

Piinks commented Jun 4, 2025

Hey @CodeDoctorDE welcome! Thanks for contributing! It looks like this never entered the review queue since it was marked a draft. Is this still something you would like to work on?

@CodeDoctorDE
Copy link
Author

Hello, oh yeah, but it currently has some problems I need to fix

@Piinks
Copy link
Contributor

Piinks commented Aug 5, 2025

Hello again @CodeDoctorDE, this came back around to stale PR triage. :)
Still have plans to work on this?

@CodeDoctorDE
Copy link
Author

Hello,
Yeah, Im happy to work again here, but in the current time, I hadn't enough time because of studying. I will continue here in a month

@Piinks
Copy link
Contributor

Piinks commented Aug 6, 2025

Quite alright! Thanks for letting us know. I am going to close this for now to remove it from our review queue and CI tasks. Feel free to reopen when you are ready to return to it. Thanks!

@Piinks Piinks closed this Aug 6, 2025
@CodeDoctorDE
Copy link
Author

Hello, I finished my changes but I don't have the option to reopen it.
Can you reopen it or should I make a new one?

@fluttergithubbot
Copy link
Contributor

An existing Git SHA, 7446780b1c60f45e90729911b67d6ad0f7d1ea28, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.

by @Demohstens

Adds six unittests for different pointer events.
Added methods to WindowsProcTable for Pointer functions to allow for overloading in tests.
Ensured all unittest pass

Co-authored-by: Demohstens <Demohstens@gmail.com>
@CodeDoctorDE CodeDoctorDE marked this pull request as ready for review August 11, 2025 07:47
@CodeDoctorDE
Copy link
Author

PR can be reviewed, not sure why mac fais

@tirth-patel-nc
Copy link
Member

PR can be reviewed, not sure why mac fais

It is passing. Tree status would be green on its own

auto that = static_cast<FlutterWindow*>(cs->lpCreateParams);
that->window_handle_ = window;
that->text_input_manager_->SetWindowHandle(window);
RegisterTouchWindow(window, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you confirm that multi-touch gestures like pinching on a trackpad still works after this change?

(This line was added by flutter/engine#27863 for multi-touch support, I'd like to double-check we're not regressing that behavior with this new implementation!)

Copy link

@Demohstens Demohstens Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this line disables WM_GESTURE & WM_TOUCH messages. The first is not handled and the latter is replaced by the new implementation so this should not impact the trackpad at all. Local tests seem to confirm this but i'm not entirely confident they cover all cases. This might have been necessary for supporting windows 7 and older as WM_POINTER* were not supported before that.

Copy link
Member

@loic-sharma loic-sharma Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha, thanks for the details!

cc @jnschulze in case you have any concerns since you added this back in: flutter/engine#27863

} else if (message == WM_POINTERUPDATE) {
// POINTER_FLAG_INCONTACT indicates the stylus is touching the screen
// When not set, it means the stylus is hovering
OnPointerMove(x, y, device_kind, touch_id, rotation, pressure, 0);
Copy link
Member

@loic-sharma loic-sharma Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these non-obvious default values, could you add a comment like /*argument_name=*/ to make these a little more obvious? For example:

Suggested change
OnPointerMove(x, y, device_kind, touch_id, rotation, pressure, 0);
OnPointerMove(x, y, device_kind, touch_id, rotation, pressure, /* modifiers_state=*/0);

* Added kMaxPenPressure constant

* Added cmath as dependancy to windows
@loic-sharma
Copy link
Member

I'll be on vacation for the next 10 days and won't be able to review. Apologies for the delay!

@Demohstens
Copy link

I'm having some issues with my setup and am unable to build the engine on my laptop atm.

I will get to your comments, @loic-sharma, next week when I'm home!

@loic-sharma
Copy link
Member

@Demohstens please feel free to ping me when this is ready for another review :)

@Demohstens
Copy link

Should be ready @loic-sharma - I totally forgot to mention you, sorry!

If someone has a pen with rotation capabilities it'd be great to test that property. The tests don't account for the transformation as they intercept the function before it's sent to the framework and my pen does not support rotation :/

auto state = GetOrCreatePointerState(device_kind, device_id);
state->buttons |= flutter_button;
state->rotation = rotation;
state->pressure = pressure;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reset these values in OnPointerUp?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a month ago I looked at it, but I think it could also be useful to get the last value that is captured instead of just having a default value.

@CodeDoctorDE
Copy link
Author

Sorry for the late update, the start of the semester held me up. It’s ready now!

@loic-sharma
Copy link
Member

cc @mattkae

Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but nothing huge on my side. I have no hardware to test though, so I'll have to trust 😄

Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests 😄 I just have one final piece of feedback here

Co-authored-by: Matthew Kosarek <matt.kosarek@canonical.com>
Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am perfectly happy with this, thanks for putting in so much work on testing 🎉

@loic-sharma Would you have anything to add? I cannot test on my end unfortunately

@loic-sharma
Copy link
Member

The tests that failed look like infra problems. I reran them!

@loic-sharma
Copy link
Member

FYI it looks like you have a formatting issue: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8695098116792788577/+/u/test:_test:_Check_formatting/stdout

To fix, run `et format` or:
git apply <<DONE
diff --git a/engine/src/flutter/shell/platform/windows/flutter_window.cc b/engine/src/flutter/shell/platform/windows/flutter_window.cc
index ef55c834cbe..00000000000 100644
--- a/engine/src/flutter/shell/platform/windows/flutter_window.cc
+++ b/engine/src/flutter/shell/platform/windows/flutter_window.cc
@@ -577,7 +577,8 @@ FlutterWindow::HandleMessage(UINT const message,
device_kind = kFlutterPointerDeviceKindTrackpad;
break;
default:
-            FML_LOG(ERROR) << "Unrecognized device key " << pointerInfo.pointerType;
+            FML_LOG(ERROR) << "Unrecognized device key "
+                           << pointerInfo.pointerType;
break;
}
if (message == WM_POINTERDOWN) {
DONE

@loic-sharma
Copy link
Member

@CodeDoctorDE I suspect the Google testing check is failing because this branch is on an old master commit. Could you rebase or merge the latest master?

@loic-sharma
Copy link
Member

Hello, it looks like you have another test failure: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8694011246384793233/+/u/build_ci_host_debug_test_flutter:unittests/stdout

../../../flutter/shell/platform/windows/flutter_window_unittests.cc(40,9): error: no matching constructor for initialization of 'FlutterWindow'
   40 |       : FlutterWindow(width, height, proc_table) {}
      |         ^             ~~~~~~~~~~~~~~~~~~~~~~~~~
../../..\flutter/shell/platform/windows/flutter_window.h(41,3): note: candidate constructor not viable: no known conversion from 'shared_ptr<WindowsProcTable>' to 'const shared_ptr<DisplayManagerWin32>' for 3rd argument
   41 |   FlutterWindow(int width,
      |   ^
   42 |                 int height,
   43 |                 std::shared_ptr<DisplayManagerWin32> const& display_manager,
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../..\flutter/shell/platform/windows/flutter_window.h(391,32): note: candidate constructor not viable: requires 1 argument, but 3 were provided
  391 |   FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindow);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
../../..\flutter/fml/macros.h(28,3): note: expanded from macro 'FML_DISALLOW_COPY_AND_ASSIGN'
   28 |   TypeName(const TypeName&) = delete;          \
      |   ^        ~~~~~~~~~~~~~~~
../../..\flutter/shell/platform/windows/flutter_window.h(202,3): note: candidate constructor not viable: requires 0 arguments, but 3 were provided
  202 |   FlutterWindow();
      |   ^
1 error generated.
ninja: build stopped: subcommand failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop engine flutter/engine related. See also e: labels. platform-windows Building on or for Windows specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flutter on Windows ignores stylus input

7 participants