Track "mouse leave" event #12363

franciscojma86 · 2019-09-19T23:42:25Z

Reports to the engine when the mouse left the client area.

stuartmorgan-g · 2019-09-20T02:53:41Z

shell/platform/windows/win32_flutter_window.cc

@@ -125,6 +125,12 @@ void Win32FlutterWindow::OnPointerUp(double x, double y) {
  }
 }

+void Win32FlutterWindow::OnPointerLeave() {
+  if (process_events_) {


FWIW (not something to change in this PR), it's likely that process_events_ and the plugin handler hooks that toggle it are entirely cruft inherited from what the GLFW implementation did to interoperate reasonably with GTK dialogs.

Possibly. There was a contract that somewhere that required events to be disabled / enabled.

stuartmorgan-g · 2019-09-20T02:56:03Z

shell/platform/windows/win32_flutter_window.h

@@ -102,6 +105,9 @@ class Win32FlutterWindow : public Win32Window {
  // Reports mouse release to Flutter engine.
  void SendPointerUp(double x, double y);

+  // Reports mouse left the window client area.


Since the asymmetry is non-obvious (or at least was to me coming from a non-Windows background—macOS and GLFW both have enter and exit, apparently unlike Win32), maybe add a comment explicitly saying that there is no SendPointerEnter because it's automatically inferred from any other pointer event being sent.

stuartmorgan-g · 2019-09-20T03:01:53Z

shell/platform/windows/win32_window.cc

@@ -107,6 +107,7 @@ Win32Window::MessageHandler(HWND hwnd,
  UINT width = 0, height = 0;
  auto window =
      reinterpret_cast<Win32Window*>(GetWindowLongPtr(hwnd, GWLP_USERDATA));
+  static bool tracking_mouse_events = false;


This should be an ivar, not static; someone should be able to run two completely independent Flutter window(+engine)s in the same app if they want to.

stuartmorgan-g · 2019-09-20T03:04:21Z

shell/platform/windows/win32_window.cc

+          TRACKMOUSEEVENT tme;
+          tme.cbSize = sizeof(tme);
+          tme.hwndTrack = hwnd;
+          // Not tracking Hover since the engine handles that logic.


I was confused by this comment at first, not already being familiar with the APIs here. I think it would be clearer to a non-Windows-expert (which is probably most of the people dealing with this code, in practice) to remove this comment and instead comment the whole block as "Register for an event when the mouse leaves the window". Or even better, extract it to a helper function so it's self-documenting and out of the main flow.

stuartmorgan-g · 2019-09-20T03:05:37Z

shell/platform/windows/win32_window.cc

+          // Not tracking Hover since the engine handles that logic.
+          tme.dwFlags = TME_LEAVE;
+          TrackMouseEvent(&tme);
+          tracking_mouse_events = true;


How about tracking_mouse_leave since that's what it's actually being used for? The current name matches the API name, but is misleading otherwise (since we are getting most mouse events regardless of the state of this variable)

stuartmorgan-g · 2019-09-20T20:55:40Z

shell/platform/windows/win32_window.h

@@ -106,7 +109,13 @@ class Win32Window {

  UINT GetCurrentHeight();

+  // Set to true to be notified when the mouse leaves the window.
+  bool tracking_mouse_leave_ = false;


ivars need to be private, not protected, with accessors if necessary. In this case, rather than an accessor you can just put the toggling of the variable in the superclass. That would make a lot more sense anyway, because what it's tracking is whether TrackMouseEvent has fired, and that's superclass logic, not subclass logic.

stuartmorgan-g

LGTM!

[email protected]:flutter/engine.git/compare/e7fd442410f4...953ac71 git log e7fd442..953ac71 --no-merges --oneline 2019-09-24 [email protected] Roll src/third_party/dart acac9ab11b..d53d355c6c (12 commits) 2019-09-24 [email protected] Roll src/third_party/skia b4b1005d485f..c096654fa7c6 (1 commits) (flutter/engine#12415) 2019-09-24 [email protected] Roll src/third_party/skia b3542d95da65..b4b1005d485f (6 commits) (flutter/engine#12414) 2019-09-24 [email protected] Write MINIMAL_SDK to exception message (flutter/engine#11345) 2019-09-23 [email protected] Track "mouse leave" event (flutter/engine#12363) 2019-09-23 [email protected] Roll src/third_party/dart 77ff89b223..acac9ab11b (6 commits) 2019-09-23 [email protected] Don't send pointer events when the framework isn't ready yet (flutter/engine#12403) 2019-09-23 [email protected] Update test to verify that secondary isolate gets shutdown before root isolate exits. (flutter/engine#12342) 2019-09-23 [email protected] Roll src/third_party/skia 57ef68077574..b3542d95da65 (12 commits) (flutter/engine#12409) 2019-09-23 [email protected] Update --dart-vm-flags whitelist to include --write-service-info and --sample-buffer-duration (flutter/engine#12395) 2019-09-23 [email protected] Add system font change listener for windows (flutter/engine#12276) 2019-09-23 [email protected] Roll fuchsia/sdk/core/mac-amd64 from xPX7p... to kgFwg... (flutter/engine#12405) 2019-09-23 [email protected] Check for index bounds in RTL handling for trailing whitespace runs. (flutter/engine#12336) 2019-09-23 [email protected] Roll fuchsia/sdk/core/linux-amd64 from F-g18... to NY2A5... (flutter/engine#12407) 2019-09-23 [email protected] Roll src/third_party/dart f546362691..77ff89b223 (6 commits) 2019-09-23 [email protected] Updating text field location in IOS as a pre-work for spellcheck (flutter/engine#12192) 2019-09-23 [email protected] Roll buildroot and remove toolchain prefix. (flutter/engine#12324) 2019-09-23 [email protected] Roll src/third_party/dart 94dd49cdb6..f546362691 (1 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

franciscojma86 added the platform-windows label Sep 19, 2019

franciscojma86 requested review from stuartmorgan-g and clarkezone September 19, 2019 23:42

googlebot added cla: yes labels Sep 19, 2019

franciscojma86 force-pushed the mouse branch from 41d3a0e to 7830179 Compare September 20, 2019 00:24

stuartmorgan-g suggested changes Sep 20, 2019

View reviewed changes

clarkezone approved these changes Sep 20, 2019

View reviewed changes

franciscojma86 added 2 commits September 20, 2019 11:27

Track mouse leave

4ab6c8b

Address comments

7e59dd3

franciscojma86 force-pushed the mouse branch from 7830179 to 7e59dd3 Compare September 20, 2019 19:41

Whitespace

b1bfea6

stuartmorgan-g reviewed Sep 20, 2019

View reviewed changes

franciscojma86 added 2 commits September 20, 2019 16:14

Make private

89c11a9

Reformat

bcc6ed6

stuartmorgan-g approved these changes Sep 21, 2019

View reviewed changes

Merge branch 'master' into mouse

a9cbb66

franciscojma86 merged commit 035c9db into flutter:master Sep 23, 2019

franciscojma86 deleted the mouse branch September 23, 2019 23:44

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2019

035c9db Track "mouse leave" event (flutter/engine#12363)

acdd787

engine-flutter-autoroll mentioned this pull request Sep 24, 2019

Roll engine e7fd442410f4..953ac71749ee (18 commits) flutter/flutter#41163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Track "mouse leave" event #12363

Track "mouse leave" event #12363

Uh oh!

franciscojma86 commented Sep 19, 2019 •

edited

Loading

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

clarkezone Sep 20, 2019

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

franciscojma86 Sep 20, 2019

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

franciscojma86 Sep 20, 2019

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

franciscojma86 Sep 20, 2019

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

franciscojma86 Sep 20, 2019

Uh oh!

stuartmorgan-g Sep 20, 2019

Uh oh!

franciscojma86 Sep 20, 2019

Uh oh!

stuartmorgan-g left a comment

Uh oh!

Uh oh!

Track "mouse leave" event #12363

Track "mouse leave" event #12363

Uh oh!

Conversation

franciscojma86 commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franciscojma86 commented Sep 19, 2019 •

edited

Loading