Created attachment 320531 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

Works only on CSD windows (presumably, because the trigger
points are only called for CSD windows), non-CSD windows continue
to use WM modal loop for drag-resizing (and drag-moving, i think).

Has the advantage of being completely immune to AeroSnap.
AeroSnap only worked partially on CSD windows, with the only part
that worked being "don't let users drag window titlebar outside of
the desktop". Now AeroSnap doesn't work on CSD windows at all,
which is good, since they currently don't work well with it due to
the way shadows are drawn.

It's possible to also re-implement AeroSnap (or something similar),
but that is a story for another commit.

I would like to point out that this patch is somewhat experimental. That is another way of saying "nacho, stop with the nitpicking and style-bashing already!". I'm more interested in the implementation feedback than in style (that said, variable and function names are kind of awkward...).

Also, this change will work well with bug 748872, as doing window move, resize and contents update in a single call is what layered windows do.

nacho: LRN, it is hard to follow those changes by just looking at the patch

nacho: LRN, next week I'll try to have a look at the code and try to understand it and provide you some comments

LRN: yeah, well, it's kind of long, but nothing too complex. Basically, instead of telling WM that someone clicked on resize area or titlebar, we just record the data (starting point, window pointer, which edge was clicked) and process mouse events ourselves

LRN: calculation is, again, long, but simple: diff between the starting point and current point is the adjustment. It adjusts the rectangle, taking minmaxinfo into account, and checks whether it changed or not. If it changed, it emits a configure event.

LRN: Then it records that fact and waits for a redraw. While it waits, no configure events (the ones caused by mouse cursor movement anyway) are emitted (this is probably unnecessary).

LRN: Immediately before redraw, if there's a resize pending, it actually takes the window size (from GTK/GDK point of view) and applies it to the underlying native window, so that actual drawing is done on the correctly-sized surface

LRN: and so on, until you release the button

I've also separated some code into functions, because i needed a way to do what configure emitting function does, but in 2 steps. And i needed to get minmax info, so i've made that code into a function too.

Well, after more thorough examination i've discovered that this patch does not achieve the desired effect. There's still a discrepancy between window contents and window size.

Created attachment 321929 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v2:
* cleanup
* made context part of the window
* use window end_paint instead of clock before-paint
  (implementation detail: end-paint is called just before
   GTK copies temporary buffer into window surface).
* changed size-calculation logic (now precisely follows the
  cursor, no matter how fast it moves around)
* still doesn't achieve the ultimate goal, but that's OK

Notes:
* Allows resizing the window farther than the taskbar edge
  (native W32 resize code does not allow that).
  This is fixable, but i'm not sure whether it should block
  this patch.

Review of attachment 321929 [details] [review]:

See some nitpicks while I try to understand how this is going on.

::: gdk/win32/gdkevents-win32.c
@@ +1416,3 @@
 
+gboolean
+_gdk_win32_get_window_rect (GdkWindow *window, RECT *rect)

split params

@@ +1903,3 @@
 
+gboolean
+_gdk_win32_window_fill_minmaxinfo (GdkWindow *window, MINMAXINFO *mmi)

split parameters and the name of the method should be min_max_info

@@ +1914,3 @@
+
+  GDK_NOTE (EVENTS, g_print (" (mintrack:%ldx%ld maxtrack:%ldx%ld "
+				 "maxpos:%+ld%+ld maxsize:%ldx%ld)",

this seems wrongly indented

@@ +1950,3 @@
+  else
+    {
+      HMONITOR winmon;

call it monitor?

@@ +1951,3 @@
+    {
+      HMONITOR winmon;
+      MONITORINFO moninfo;

I'd say monitor_info

@@ +2571,3 @@
+
+      if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
+      {

wrong indentation

@@ +2577,3 @@
+      {
+        GDK_NOTE (EVENTS,
+		g_print (" don't do moreresize because op %u == 0",

wrong indentation

@@ +2582,3 @@
+
+      if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
+        {

I do not like empty ifs, just reverse the check

@@ +2894,3 @@
+
+      if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
+        gdk_win32_window_end_moveresize_drag (window);

move_resize

@@ +3254,2 @@
+      if (_gdk_win32_window_fill_minmaxinfo (window, mmi))
+      {

wrong indentation

::: gdk/win32/gdkprivate-win32.h
@@ +545,3 @@
+void     _gdk_win32_do_emit_configure_event    (GdkWindow *window, RECT rect);
+void      gdk_win32_window_do_moveresize_drag  (GdkWindow *window, gint x, gint y);
+void      gdk_win32_window_end_moveresize_drag (GdkWindow *window);

move_resize...

::: gdk/win32/gdkwindow-win32.c
@@ +2662,2 @@
 static void
+setup_dragmoveresize_context (GdkWindow                   *window,

drag_move_resize...

@@ +2686,2 @@
+  GDK_NOTE (EVENTS, g_print ("begin drag moveresize: op %u, edge %d, device %p, button %d, coord %d:%d, time %u\n",
+                             context->op, context->edge, context->device, context->button, context->start_root_x, context->start_root_y, context->timestamp));

this is really long... split it in several lines

@@ +2696,3 @@
+  context->op = GDK_WIN32_DRAGOP_NONE;
+  GDK_NOTE (EVENTS, g_print ("end drag moveresize: op %u, edge %d, device %p, button %d, coord %d:%d, time %u\n",
+                             context->op, context->edge, context->device, context->button, context->start_root_x, context->start_root_y, context->timestamp));

ditto

@@ +2810,3 @@
 
+    if (height > mmi.ptMaxTrackSize.y)
+    {

indentation

::: gdk/win32/gdkwindow-win32.h
@@ +89,3 @@
+   * the window size and poistion to the native window.
+   */
+  gboolean           native_moveresize_pending;

move_resize...

@@ +132,3 @@
   HBITMAP          saved_dc_bitmap; /* Original bitmap for dc */
+
+  GdkW32DragMoveResizeContext dragmoveresize_context;

drag_move_resize_context

Review of attachment 321929 [details] [review]:

In general the patch looks ok to me. See though some other questions and nitpicks around. Also, have you tested this with multiple monitors? if not we should probably point it out somewhere that we should test it for that.

::: gdk/win32/gdkevents-win32.c
@@ +1957,3 @@
+      moninfo.cbSize = sizeof (moninfo);
+
+      if (GetMonitorInfoA (winmon, &moninfo))

should we use the W version? though I guess we could do it on a different patch

::: gdk/win32/gdkwindow-win32.c
@@ +2725,2 @@
+  switch (context->op)
+  {

wrong indent

@@ +2729,1 @@
     {

wrong indent

@@ +2862,3 @@
+  {
+    context->native_moveresize_pending = TRUE;
+    _gdk_win32_do_emit_configure_event (window, new_rect);

why do we emit it here? is the size to change already at this point? I guess the magic happens on the end_paint so you just do the configure here but the actual size/position changes on the end_paint?

@@ +2917,3 @@
+
+  if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
+    gdk_win32_window_end_moveresize_drag (window);

why do we need to end the move_resize_drag? I would expect this to happen when end_resize_drag is called...

@@ +2919,3 @@
+    gdk_win32_window_end_moveresize_drag (window);
+
+  setup_dragmoveresize_context (window, &impl->dragmoveresize_context, GDK_WIN32_DRAGOP_RESIZE, edge, device, button, root_x, root_y, timestamp);

too long

@@ +2951,3 @@
+
+  if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
+    gdk_win32_window_end_moveresize_drag (window);

same question here.

::: gdk/win32/gdkwindow-win32.h
@@ +69,3 @@
+
+  /* Not used */
+  gint               button;

if not used do we need it?

(In reply to Ignacio Casal Quinteiro (nacho) from comment #7)
> Review of attachment 321929 [details] [review] [review]:
> 
> In general the patch looks ok to me. See though some other questions and
> nitpicks around. Also, have you tested this with multiple monitors? if not
> we should probably point it out somewhere that we should test it for that.
I'll try to do a multi-monitor test. I have a spare screen i can connect for that purpose.

> 
> ::: gdk/win32/gdkevents-win32.c
> @@ +1957,3 @@
> +      moninfo.cbSize = sizeof (moninfo);
> +
> +      if (GetMonitorInfoA (winmon, &moninfo))
> 
> should we use the W version? though I guess we could do it on a different
> patch

There are A and W versions because there's an GetMonitorInfoEx[A|W]() function that fills up MONITORINFOEX[A|W] structure, which does have a string in it.
MONITORINFO doesn't, so it doesn't matter which kind of function we're using.

> @@ +2862,3 @@
> +  {
> +    context->native_moveresize_pending = TRUE;
> +    _gdk_win32_do_emit_configure_event (window, new_rect);
> 
> why do we emit it here? is the size to change already at this point? I guess
> the magic happens on the end_paint so you just do the configure here but the
> actual size/position changes on the end_paint?

Yes. When cursor drag triggers a size change, we don't just change the window size, we generate a configure event instead and then wait for a redraw, THEN change the size, to ensure that underlying window size change and contents update happen at the same time.
(this didn't really work out all that well, obviously, but it is required for the layered windows patch)
Except for when there's no size change, just position change, in which case we just move the window right away.

> 
> @@ +2917,3 @@
> +
> +  if (impl->dragmoveresize_context.op != GDK_WIN32_DRAGOP_NONE)
> +    gdk_win32_window_end_moveresize_drag (window);
> 
> why do we need to end the move_resize_drag? I would expect this to happen
> when end_resize_drag is called...
This is mostly cosmetic/symmetry stuff. If there's a drag op already in place, and we begin another one, we should wrap up the one we have first. Granted, gdk_win32_window_end_moveresize_drag() doesn't really do much, as moveresize drag haven't got any context to clean up, but still...

> ::: gdk/win32/gdkwindow-win32.h
> @@ +69,3 @@
> +
> +  /* Not used */
> +  gint               button;
> 
> if not used do we need it?

I just stashed all the data we get at the beginning of the drag into the context. Maybe some of it will become useful for something. Like, dragging with right or middle mouse button. Dunno.

Created attachment 321961 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v3:
* Fixed behaviour in multi-monitor environment (incorrect
  handling of _gdk_offset_*)
* Fixed most nitpicks

For the record, i think that indentation rules in GTK are ridiculous.
I mean, just look at that switch...

Created attachment 322199 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v4:
* fixed a few typos

Review of attachment 322199 [details] [review]:

A couple more nitpicks :)

::: gdk/win32/gdkevents-win32.c
@@ +1448,3 @@
+
+void
+_gdk_win32_do_emit_configure_event (GdkWindow *window, RECT rect)

params should be split

::: gdk/win32/gdkprivate-win32.h
@@ +538,3 @@
+void      gdk_win32_window_do_move_resize_drag  (GdkWindow *window, gint x, gint y);
+void      gdk_win32_window_end_move_resize_drag (GdkWindow *window);
+gboolean _gdk_win32_window_fill_min_max_info    (GdkWindow *window, MINMAXINFO *mmi);

in the prototypes the params should be split as well

Review of attachment 322199 [details] [review]:

Its possible as a programmer to automatically trigger these things, using gtk_window_begin_resize_drag(). 
For instance, the "resize grips" test in tests/testgtk call these. Can you try those for a non-csd window?

Otherwise, from a quick glance this looks sane to me. Of course, i did zero testing, so it may be completely busted :)

::: gdk/win32/gdkwindow-win32.c
@@ +211,3 @@
+	                   x - _gdk_offset_x, y - _gdk_offset_y,
+                           w, h,
+	                   SWP_NOACTIVATE | SWP_NOZORDER));

I wonder if you should also pass SWP_NOREDRAW, as we're going to paint it anyway in a second.

Works on non-CSD windows, although not without issues (fixing these...i think it's about window size vs client area size).
I also found a bug in MINMAXINFO handling, will fix that one as well.

I'll try SWP_NOREDRAW.

Created attachment 322254 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v5:
* Clarified that _gdk_win32_get_window_rect() gives client rectangle.
* Use primary display for maximized window size calculation when
  filling MINMAXINFO (actual maximization, is handled elsewhere and
  does not seem to use this size).
* Use SM_CXMAXTRACK and SM_CYMAXTRACK for maximal non-maximized
  size when filling MINMAXINFO, as MSDN advices. This is the size
  of the virtual desktop.
* Fix more nitpicks.
* Call _gdk_win32_adjust_client_rect() before resizing/moving
  windows. Now customresize code should work correctly on non-CSD
  windows (when it *is* used on non-CSD windows - it requires
  explicit GDK resize calls, i.e. from resize grips test in testgtk;
  normally native W32 WM resizer is used for non-CSD windows).
* Memset MINMAXINFO to 0 when not handling WM_GETMINMAXINFO
* Use SWP_NOREDRAW flag when resizing a window shortly before redraw.

Created attachment 322315 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v6:
* Split off minmaxinfo-related stuff into separate patch (bug 762629)

Created attachment 322715 [details] [review]
GDK W32: custom (non-WM) drag-move and drag-resize code

v7:
* Removed a GDK_NOTE() that spammed too much
* Updated commit message to reflect actual changes, behaviour
  and future plans

This is the last version of this patch (possible nitpicks aside),
any other changes will go into separate patches

Reporting from the IRC conversation:

I think we should get this in sooner rather than later: the patch adds non trivial amount of code to the win32 backend, but it fixes real issues and does not causes regressions for the users (CSD windows was not working with half-maximixe etc even before). The patch is also somewhat self contained and easy to disable.

If someone with intimate knowledge of GDK wants to weight in, all comments are very welcome, but on the Win32 side I do not think we have anyone that is able to dig into this issue deeper than LRN, so IMHO we should not block.


One thing that was mentioned on IRC: before committing let's test on Win10

Limited Windows 10 testing shows that this code and the code from other related patches do work as intended.

Hi,

I would say, first of all, a great thanks to LRN for this.

+1 for Paolo--I think it is better to get this code in, and let more people try this out (I will test build it with Visual Studio to see whether there are any issues), and let them see if there are any outstanding issues involved.

I don't have Windows 10 though, so I can't say anything about it, but I will see how things go on Windows 7 and 8.1.

My take on this.  With blessings!

Attachment 322715 [details] pushed as e03946b - GDK W32: custom (non-WM) drag-move and drag-resize code