After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 758483 - GDK W32: Incorrectly uses SetWindowLong() to set/unset WS_EX_TOPMOST
GDK W32: Incorrectly uses SetWindowLong() to set/unset WS_EX_TOPMOST
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Win32
unspecified
Other All
: Low normal
: ---
Assigned To: gtk-win32 maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-11-22 03:42 UTC by LRN
Modified: 2015-11-26 16:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST (2.63 KB, patch)
2015-11-22 03:42 UTC, LRN
none Details | Review
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST (2.98 KB, patch)
2015-11-26 12:04 UTC, LRN
none Details | Review
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST (3.04 KB, patch)
2015-11-26 13:05 UTC, LRN
none Details | Review
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST (3.06 KB, patch)
2015-11-26 14:08 UTC, LRN
committed Details | Review

Description LRN 2015-11-22 03:42:03 UTC
As it became evident while looking for a fix for bug 746745, doing this
does not work. One has to use SetWindowPos() to set/unset WS_EX_TOPMOST.
Comment 1 LRN 2015-11-22 03:42:09 UTC
Created attachment 316041 [details] [review]
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST

While searching for bug 746745 it was discovered that one could
not set WS_EX_TOPMOST extended window style with SetWindowLong(),
but must use SetWindowPos() for that purpose.

This was never a problem most likely because it is highly unlikely for windows
to acquire/lose WS_EX_TOPMOST after they are created, by means other
than SetWindowPos() (which GTK does use to raise/lower windows and
set/remove keep_above).
Comment 2 Ignacio Casal Quinteiro (nacho) 2015-11-24 11:25:49 UTC
Review of attachment 316041 [details] [review]:

See the comments.

::: gdk/win32/gdkwindow-win32.c
@@ +2462,3 @@
 
+      SetWindowLong (GDK_WINDOW_HWND (window), GWL_EXSTYLE,
+		     new_exstyle & (~WS_EX_TOPMOST));

so, why is new_style having this flag set? should we ensure that we never get it here?

@@ +2477,3 @@
+  if ((new_style & WS_EX_TOPMOST) &&
+      !(old_style & WS_EX_TOPMOST))
+  {

indentation is wrong
Comment 3 LRN 2015-11-24 11:35:08 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #2)
> Review of attachment 316041 [details] [review] [review]:
> 
> ::: gdk/win32/gdkwindow-win32.c
> @@ +2462,3 @@
>  
> +      SetWindowLong (GDK_WINDOW_HWND (window), GWL_EXSTYLE,
> +		     new_exstyle & (~WS_EX_TOPMOST));
> 
> so, why is new_style having this flag set? should we ensure that we never
> get it here?
>

That is certainly possible, but might take more code (although with the latest iteration of this patch it might already be taking too much code anyway, so maybe that would be better).

Is that all?
Comment 4 Ignacio Casal Quinteiro (nacho) 2015-11-26 07:43:37 UTC
yeah, I did not spot anything else. Feel free to make the changes.
Comment 5 LRN 2015-11-26 12:04:57 UTC
Created attachment 316307 [details] [review]
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST

V2:
Remove WS_EX_TOPMOST from exstyle

The testing i've done indicates that it's not strictly necessary to do so,
but it's probably cleaner (and more readable) that way
Comment 6 Ignacio Casal Quinteiro (nacho) 2015-11-26 12:29:41 UTC
Review of attachment 316307 [details] [review]:

Double check the indentation since it seems wrong.

::: gdk/win32/gdkwindow-win32.c
@@ +2428,2 @@
   if (window->window_type == GDK_WINDOW_TEMP)
+  {

indentation
Comment 7 LRN 2015-11-26 13:05:54 UTC
Created attachment 316314 [details] [review]
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST

V3:
Changed indentation to use the same brainded scheme used by the surrounding code.
Comment 8 Ignacio Casal Quinteiro (nacho) 2015-11-26 13:18:18 UTC
Review of attachment 316314 [details] [review]:

still indentation issues.

::: gdk/win32/gdkwindow-win32.c
@@ +2433,2 @@
   else if (impl->type_hint == GDK_WINDOW_TYPE_HINT_UTILITY)
+   {

missing one space this indentation

@@ +2490,2 @@
+  if (will_be_topmost && !was_topmost)
+  {

here is missing the 2 spaces

@@ +2493,3 @@
+  }
+  else if (was_topmost && !will_be_topmost)
+  {

same here

@@ +2497,3 @@
+  }
+  else
+  {

and here
Comment 9 LRN 2015-11-26 14:08:47 UTC
Created attachment 316316 [details] [review]
GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST

V4:
Even more braindead indentation.
Comment 10 Ignacio Casal Quinteiro (nacho) 2015-11-26 14:13:06 UTC
Review of attachment 316316 [details] [review]:

Patch seems fine, but should you update the commit message? Feel free to update it and push it.
Comment 11 LRN 2015-11-26 16:30:25 UTC
Attachment 316316 [details] pushed as db1b242 - GDK W32: Don't use SetWindowLong() to set/unset WS_EX_TOPMOST