GNOME Bugzilla – Bug 763013
GDK W32: AeroSnap doesn't work on CSD windows
Last modified: 2016-04-02 14:24:49 UTC
Custom resize landed and now AeroSnap doesn't work completely. Except for shift+winkey+left/right, which was totally unexpected. Obviously, snapping is useful, and we should add it back.
Created attachment 322921 [details] [review] GDK W32: Re-implement AeroSnap for CSD windows It's called OreaSnap, and it works exactly like AeroSnap. Except for shift+win+left/right, which is left for AeroSnap to handle (AeroSnap takes action before we get the message, so there's no way for us to override it). The only thing that doesn't work is shift+win+left/right on a maximized window, for reasons unknown at the moment. This only implements winkey+stuff behaviour of AeroSnap, not the drag-to-the-edge-and-something-funny-happens one.
Created attachment 323180 [details] [review] GDK W32: Improve OreaSnap - don't resize windows that fit Applies on top of attachment 322921 [details] [review], further refines OreaSnap implementation so that it better matches AeroSnap behaviour.
Created attachment 323366 [details] [review] GDK W32: Re-implement AeroSnap for CSD windows v2: * Rebased on top of newer git master * Backported some of the changes from the third patch into this patch (mostly moving definitions into another header).
Created attachment 323367 [details] [review] GDK W32: Improve OreaSnap - don't resize windows that fit v2: * Rebased on top of newer git master * Backported some of the changes from the third patch into this patch (mostly moving definitions into another header). * Beautified the code in some places This is what AeroSnap does. If a window is being unsnapped on a new monitor, check if the work area is large enough for the window to fit in its normal size. If the window fits, just reposition it so that the ratio of left-window-edge-to-screen-edge / right-window-edge-to-screen-edge remains the same, without scaling the window.
Created attachment 323368 [details] [review] GDK W32: Add drag-to-snap feature to OreaSnap This implements the part of AeroSnap that snaps windows when you drag them (while moving or resizing) to the edge of the screen. It also fixes drag behaviour for snapped and maximized windows (if such windows are dragged, first they must be unmaximized/unsnapped). Note that this code does not take into account the shadow width, and because of that the under-pointer-position-preserving window moves might not look as such for maximized windows, which lack the shadow when maximized, but do have the shadow when unmaximized. This commit also doesn't cover some corner-cases the same way AeroSnap does. Also, the snapping indicator (which is supposed to be a window shape that shows where the window will be if the drag op is stopped at its current point) is not being drawn, all routines responsible for its creation, moving and drawing are stubs.
Review of attachment 323366 [details] [review]: Hi LRN, Some initial comments on the code... Thanks for the great work for GDK-Win32 though ::: gdk/win32/gdkwindow-win32.c @@ +46,1 @@ Let's include fallback-c89.c here, so that we ensure round() is available...
Review of attachment 323368 [details] [review]: One more comment... Took a go at it and looks quite nice! Just out of curiosity, is there a reasoning behind "OreaSnap" With blessings, thank you, and cheers! ::: gdk/win32/gdkwindow-win32.c @@ +3030,3 @@ +#undef _M_RIGHT + + gint i; Please declare this on the top of the block, thanks!
(In reply to Fan, Chun-wei from comment #7) > Review of attachment 323368 [details] [review] [review]: > > Just out of curiosity, is there a reasoning behind "OreaSnap" > Orea == Aero backwards. > > ::: gdk/win32/gdkwindow-win32.c > @@ +3030,3 @@ > +#undef _M_RIGHT > + > + gint i; > > Please declare this on the top of the block, thanks! Yeah, that code was for debugging. I'll remove region logging before this goes live.
Created attachment 323768 [details] [review] GDK W32: Draw snap indicators for OreaSnap Indicator is a bare layered click-through native window, painted completely by GDK, including animation. This commit also isolates some of the more spam-ish debug logging under ifdef. This commit also changes the system metric used for maximal window height for the snapping purposes. Turns out, SM_CYMAXTRACK is way too large, use SM_CYVIRTUALSCREEN instead. Apply on top of previous patches.
Review of attachment 323768 [details] [review]: See the nitpicks. ::: gdk/win32/gdkwindow-win32.c @@ +2969,3 @@ GdkScreen *screen; gint n_monitors, monitor, other_monitor; +#if defined(MORE_OREASNAP_DEBUGGING) just use ifdef @@ +3490,3 @@ +static gboolean +ensure_indicator_exists (GdkW32DragMoveResizeContext *context) ensure_shape_indicator_exists? @@ +3495,3 @@ + { + HWND handle; + ATOM klass; leave empty line after declare the vars @@ +3516,3 @@ + return FALSE; + + return TRUE; just do return context->shape_indicator != NULL @@ +3554,3 @@ +{ + /* TODO: make it a gsetting? Adjust for HiDPI? */ + gdouble mult; I don't like mult as a var name @@ +3578,3 @@ + /* TODO: make it a gsetting? */ +/* milliseconts */ +static guint indicator_animation_tick = 16; add it inside the method for now @@ +3581,3 @@ + +static guint +get_indicator_animation_tick () missing void @@ +3588,3 @@ + /* TODO: make it a gsetting? */ +/* microseconds */ +static gint64 indicator_animation_duration = 200 * 1000; add it inside the method @@ +3591,3 @@ + +static gint64 +get_indicator_animation_duration () void @@ +3597,3 @@ + + /* TODO: make it a gsetting? */ +static gdouble indicator_corner_radius = 5.0; inside the method @@ +3600,3 @@ + +static gdouble +get_indicator_corner_radius () void @@ +3606,3 @@ + + /* TODO: make it a gsetting? */ +static gdouble indicator_line_width = 3.0; inside @@ +3609,3 @@ + +static gdouble +get_indicator_line_width () void @@ +3624,3 @@ + GdkRGBA *fill, + GdkRGBA *outline) +{ I guess you should use cairo_save and cairo_restore around the context changes @@ +3672,3 @@ + GdkRectangle current_rect; + gint64 current_time = g_get_monotonic_time (); + gint64 animation_duration = get_indicator_animation_duration (); do the declaration here and the assigment right after for the vars calling methods @@ +3696,3 @@ + aniscale = 0; + + anicurve = curve (aniscale); anicurve is a bad name @@ +3825,3 @@ + +static GdkRectangle +unity_of_rects (GdkRectangle a, GdkRectangle b) split params
Review of attachment 323367 [details] [review]: See the comment. Apart from that I do not have an opinion. If it works go for it. ::: gdk/win32/gdkwindow-win32.c @@ +2949,3 @@ + if (!impl->snap_stash_int) + impl->snap_stash_int = g_new0 (GdkRectangle, 1); is it possible that you are leaking this when i.e closing the window?
(In reply to Ignacio Casal Quinteiro (nacho) from comment #10) > Review of attachment 323768 [details] [review] [review]: > ::: gdk/win32/gdkwindow-win32.c > @@ +2969,3 @@ > GdkScreen *screen; > gint n_monitors, monitor, other_monitor; > +#if defined(MORE_OREASNAP_DEBUGGING) > > just use ifdef I prefer #if defined(). Easier to extend, if you need to make the condition more complex. (In reply to Ignacio Casal Quinteiro (nacho) from comment #11) > Review of attachment 323367 [details] [review] [review]: > > ::: gdk/win32/gdkwindow-win32.c > @@ +2949,3 @@ > > + if (!impl->snap_stash_int) > + impl->snap_stash_int = g_new0 (GdkRectangle, 1); > > is it possible that you are leaking this when i.e closing the window? Unlikely. I do free snap_stash_int in impl finalization. snap_stash, however, is indeed leaked. Good catch. That said, i'll wait for other reviews. Maybe i'll be able to coerce pbor into evaluating this (at least conceptually, if not in the actual code).
Created attachment 323924 [details] [review] GDK W32: Re-implement AeroSnap for CSD windows v3: * Include round() fallback for broken C libraries
Created attachment 323925 [details] [review] GDK W32: Improve OreaSnap - don't resize windows that fit v3: * Fix a leak
Created attachment 323926 [details] [review] GDK W32: Draw snap indicators for OreaSnap v2: * Fix nitpicks
Review of attachment 323924 [details] [review]: I am ok with the patch, though from an X11 point of view it is mutter the one handling this kind of stuff. I wonder why windows does not do it for us... ::: gdk/win32/gdkwindow-win32.c @@ +2954,3 @@ + MONITORINFO hmonitor_info; + + placement.length = sizeof(WINDOWPLACEMENT); missing space before ( @@ +2971,3 @@ + return; + + if (!impl->snap_stash) change it to != NULL
Created attachment 323946 [details] [review] GDK W32: Re-implement AeroSnap for CSD windows v4: * fix a nitpick
Created attachment 323947 [details] [review] GDK W32: Improve OreaSnap - don't resize windows that fit v4: * Fix a nitpick
Created attachment 323948 [details] [review] GDK W32: Draw snap indicators for OreaSnap v3: * Fix a tpyo
Created attachment 323955 [details] [review] GDK W32: Add/subtract shadow when drag-resizing Implements gdk_win32_window_set_shadow_width(). Uses shadow width/height to adjust max tracking size, allowing windows to be drag-resized to cover the whole desktop. Also uses SM_C*VIRTUALSCREEN instead of SM_C*MAXTRACK. This patch should apply on top of master by itself (it's not related to OreaSnap), but it provides little value on its own, so i'll keep it in this bug.
Created attachment 323956 [details] [review] GDK W32: Add/subtract shadow when (un)snapping Now halfleft/halfright/fullup snaps do hug screen edges as intended. Documents AeroSnap behaviour when snapped windows are drag-resized (currently OreaSnap handles this in a very simplistic way). Don't believe GTK when it tells us that window shadow is 0, preserve previous values (but do remember that GTK wants no shadow, in case we need that). Fixes a couple of bugs in unsnapping (check offset against the half of the window; don't put pointer in the middle of the window vertically if it still fits in the top half).
attachment 323955 [details] [review] and attachment 323956 [details] [review] also fix bug 759826
Created attachment 324022 [details] [review] GDK W32: Use a dumb window class for decorative windows Currently only one kind of decorative window is in use - the shape indicator that is shown when snapping windows to the edge of the screen. When normal toplevel class is used, its window procedure expects certain motions from GDK (passing user data to CreateWindowEx(), registering handle in a hash map etc), and might crash if that is not done. Dumb window doesn't require anything, it can just be.
Created attachment 324317 [details] [review] GDK W32: Add drag-to-snap feature to OreaSnap v2: * nacho said "use a define btw", so i'm hardcoding snapping parameters as macros
Created attachment 324318 [details] [review] GDK W32: Draw snap indicators for OreaSnap v4: * nacho said "use a define btw", so i'm hardcoding animation and drawing parameters as macros
I have not managed to review this line by line, but here is my general comments as discussed on IRC - I prefer aero instead of orea because grep trumps humor - double check that everything is namespaced (seems some of the enums are not) - confirm it adds no new public API (nothing in the installed headers etc) Apart from that, it is a lot of code, but we knew that when we decided to go down the CSD rabbit hole. If those three things are done, I would say let's get it in on master, at least we may get some testing.
Commits (after applying changes outlined in comment 26) pushed as 0ce217cf94c2a0c1c9936daded9dd5b6183cea32 .. bde5281ae811179b11e8344c8312e58698e658cb .