Created attachment 302178 [details] [review]
W32: Fold some repeatedly used chunks of code into a function

Review of attachment 302178 [details] [review]:

See the comments.

::: gdk/win32/gdkevents-win32.c
@@ +452,2 @@
 static GdkWindow *
+window_from_point (POINT pt, gboolean *ignore_leave, HWND *synaptics_quirk_msg_hwnd)

split parameters

@@ +476,3 @@
+     this window */
+  if ((ignore_leave || synaptics_quirk_msg_hwnd != NULL) &&
+      GetClassNameA (hwnd, classname, sizeof(classname)) &&

please use the W method instead

@@ +491,3 @@
+	    hwndc = ChildWindowFromPoint (hwnd, point);
+	    ClientToScreen (hwnd, &point);
+	  } while (hwndc != hwnd && (hwnd = hwndc, 1));

this coding style for do while is wrong

(In reply to Ignacio Casal Quinteiro (nacho) from comment #2)
> Review of attachment 302178 [details] [review] [review]:
> 
> See the comments.

Note that code here is a copy-paste, so most style weirdness is from the original code.

> @@ +476,3 @@
> +     this window */
> +  if ((ignore_leave || synaptics_quirk_msg_hwnd != NULL) &&
> +      GetClassNameA (hwnd, classname, sizeof(classname)) &&
> 
> please use the W method instead

Unnecessary. This is intended for a specific check (SYNAPSIS_ICON_WINDOW_CLASS), not generic use.

Review of attachment 302178 [details] [review]:

I'm not sure the helper is really helping making the code easier to read in the synaptics case, especially since it handles everything a bit differently there (i.e. we ignore the return value in that case and use an out parameter only.
Actually I'm not even sure that case is right. As it now adds a PtInRect() call that was not there before.

Maybe a better approach is to add another helper hwnd_is_synapsis(), and then have the window_from_point helper not have any synapsis magic.
Then for the ignore_leave case one can just call window_from_point(), and then ignore_leave = hwnd_is_synapsis (window->handle).

For the other case, its different enough that we can just not use the helper there.

(In reply to Alexander Larsson from comment #4)
> Review of attachment 302178 [details] [review] [review]:
> 
> I'm not sure the helper is really helping making the code easier to read in
> the synaptics case, especially since it handles everything a bit differently
> there (i.e. we ignore the return value in that case and use an out parameter
> only.
> Actually I'm not even sure that case is right. As it now adds a PtInRect()
> call that was not there before.
> 
> Maybe a better approach is to add another helper hwnd_is_synapsis(), and
> then have the window_from_point helper not have any synapsis magic.
> Then for the ignore_leave case one can just call window_from_point(), and
> then ignore_leave = hwnd_is_synapsis (window->handle).
> 
> For the other case, its different enough that we can just not use the helper
> there.

Right. What i wanted the most is for someone to verify that i haven't messed up and that my folded code does the same thing original code did. I'll see about making the changes you've mentioned.

Created attachment 302529 [details] [review]
W32: GDK backend optimization

Restructured the code as suggested.

Merged code from bug 748328 (because there's no intermediate coordinate
fix commit between them anymore).

Review of attachment 302529 [details] [review]:

See nitpicks

::: gdk/win32/gdkevents-win32.c
@@ +411,3 @@
 
+static GdkWindow *
+_gdk_window_from_point (POINT pt, HWND hwnd, GdkWindow *guess)

I still think this should be split

@@ +442,3 @@
+   */
+  return GetClassNameA (hwnd, classname, sizeof(classname)) &&
+      strcmp (classname, SYNAPSIS_ICON_WINDOW_CLASS) == 0;

align these

@@ +446,3 @@
+
+static HWND
+synaptics_hack (HWND hwnd, POINT *pt)

same here with the split

Created attachment 302613 [details] [review]
W32: GDK backend optimization

v2:
* Fixed nitpicks
* Removed a stray hwnd from _gdk_window_from_point()

Review of attachment 302613 [details] [review]:

Minor comment and feel free to push it after fixing that.

::: gdk/win32/gdkevents-win32.c
@@ +438,3 @@
+  char classname[64];
+
+  /* The synapitics trackpad drivers have this irritating

typo in synapitics

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new