GNOME Bugzilla – Bug 758484
GDK W32: Some calls to W32 routines are rather unclear
Last modified: 2015-11-26 18:01:56 UTC
1) The second argument to SetWindowPos() is a handle of a window on top of which the window being moved should be placed (i.e. how Z-order should be changed) or a special value (-2, -1, 0 or 1) to make it always-on-top or not always-on-top or move it to the top/bottom of the Z-order. However, if SWP_NOZORDER flag is specified in the last argument, the second argument is ignored. In some places the second argument is given as NULL in such case (which is technically equivalent to giving 0: HWND_TOP), in other places other values are given. This is confusing. 2) Windows with the WS_EX_TRANSPARENT extended style are hidden via SetWindowPos() instead of ShowWindow(). Nothing around the call to SetWindowPos() explains the difference.
Created attachment 316042 [details] [review] GDK W32: Clarify the use of the second argument to SetWindowPos()
Created attachment 316043 [details] [review] GDK W32: Add a comment for clarity This is a copy of a similar comment in another place, which explains why WS_EX_TRANSPARENT windows get a special treatment.
Review of attachment 316042 [details] [review]: See teh comments. ::: gdk/win32/gdkwindow-win32.c @@ +66,3 @@ + * NULL is equivalent to HWND_TOP. + */ +#define SWP_NOZORDER_IS_SPECIFIED HWND_TOP To be honest I think we should just use HWND_TOP directly since this define kind of obfuscates what we want to get passed in. @@ -1221,3 @@ if (GetWindowLong (GDK_WINDOW_HWND (window), GWL_EXSTYLE) & WS_EX_TRANSPARENT) { - SetWindowPos (GDK_WINDOW_HWND (window), HWND_BOTTOM, here it is bottom, but you are changing it to TOP with this new define.
Review of attachment 316043 [details] [review]: Looks good.
(In reply to Ignacio Casal Quinteiro (nacho) from comment #3) > Review of attachment 316042 [details] [review] [review]: > > ::: gdk/win32/gdkwindow-win32.c > @@ +66,3 @@ > + * NULL is equivalent to HWND_TOP. > + */ > +#define SWP_NOZORDER_IS_SPECIFIED HWND_TOP > > To be honest I think we should just use HWND_TOP directly since this define > kind of obfuscates what we want to get passed in. We don't want to pass anything. Or, rather, we can pass absolutely anything, it wouldn't matter, because the second argument is ignored. This macro is to ensure that anyone reading this code is not confused about the second argument: it's completely, utterly bogus, because SWP_NOZORDER flag is specified. Passing HWND_TOP, HWND_BOTTOM or anything else is confusing, because you read it and think: "OK, it puts the window on top or lower it to the bottom", and then you see that there's a SWP_NOZORDER flag, so the second argument does nothing, and you think "Wait, is this intentional? Did the original author make a mistake and specified SWP_NOZORDER where it shouldn't be, screwing up the Z-order changing? Or is the second argument just a placeholder that truly was never intended to mean anything?". SWP_NOZORDER_IS_SPECIFIED (that name is not sent in stone, by the way; have a better idea?) is weird, but makes it absolutely clear that the person who wrote the code (or, in this case, the person who edited the code, i.e. me) fully understands what the code should be doing, and that yes, the second argument does nothing, and the flag is there for a reason. > > @@ -1221,3 @@ > if (GetWindowLong (GDK_WINDOW_HWND (window), GWL_EXSTYLE) & > WS_EX_TRANSPARENT) > { > - SetWindowPos (GDK_WINDOW_HWND (window), HWND_BOTTOM, > > here it is bottom, but you are changing it to TOP with this new define. See above. In this case second argument is ignored.
I see, now this makes more sense thanks. I'd remove the IS_ from the name though: SWP_NOZORDER_SPECIFIED. Apart from that feel free to push it.
Created attachment 316335 [details] [review] GDK W32: Clarify the use of the second argument to SetWindowPos() V2: * Removed "_IS" from the macro name
Attachment 316043 [details] pushed as 3701a60 - GDK W32: Add a comment for clarity Attachment 316335 [details] pushed as 8b7783c - GDK W32: Clarify the use of the second argument to SetWindowPos()