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 741895 - gwin32: Add Simple API to check whether we are on at least a version of Windows
gwin32: Add Simple API to check whether we are on at least a version of Windows
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: win32
2.43.x
Other Windows
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
gtk-win32 maintainers
Depends on:
Blocks: 741849
 
 
Reported: 2014-12-23 06:58 UTC by Fan, Chun-wei
Modified: 2015-01-27 04:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gwin32: Add public API g_win32_windows_is_at_least() (5.69 KB, patch)
2014-12-23 07:25 UTC, Fan, Chun-wei
none Details | Review
gwin32: Add public API g_win32_check_windows_version() (6.75 KB, patch)
2015-01-21 04:20 UTC, Fan, Chun-wei
reviewed Details | Review
gwin32: Add public API g_win32_check_windows_version() (take ii) (7.02 KB, patch)
2015-01-26 03:19 UTC, Fan, Chun-wei
committed Details | Review
Update glib-sections.txt for g_win32_check_windows_version() (829 bytes, patch)
2015-01-26 03:20 UTC, Fan, Chun-wei
committed Details | Review

Description Fan, Chun-wei 2014-12-23 06:58:23 UTC
Hi,

As Windows 8.1 introduced some changes to GetVersion() and GetVersionInfoEx() [1][2], I think probably it would be a good idea if we have a new API in GLib to check whether we are at least running some specified version of Windows, including the service pack level and the type of Windows (i.e. workstation edition or server edition) where the code is being run on.  This also enables Windows version checks to be done in a more careful manner.

A few examples of which this might become useful:
-We need a certain version of Windows for OS-level functionality
 (for those that we don't need to venture into the GetProcAddress()
 thing).
-Some OS functionality changed in the way it worked, such as
 DWM (i.e. compositing) being always activated on Windows 8+/Server
 2012+, and cannot be switched off.  This is part of bug 741849, and
 hence 727316.

[1]: http://msdn.microsoft.com/zh-tw/library/windows/desktop/ms724451%28v=vs.85%29.aspx
[2]: http://msdn.microsoft.com/zh-tw/library/windows/desktop/ms724451%28v=vs.85%29.aspx

With blessings, thank you!
Comment 1 Fan, Chun-wei 2014-12-23 07:25:41 UTC
Created attachment 293224 [details] [review]
gwin32: Add public API g_win32_windows_is_at_least()

Hi,

This is my proposal for the code, with the additional note for the g_win32_get_windows_version() API due to the changes in GetVersion().

There are two things that I can think of regarding g_win32_get_windows_version() after this API addition:
-Simply deprecate this in favor of the new API.
-For Windows before 8.1 (checking via the new API),
 do things as before; otherwise retrieve the Windows
 version by calling the new API in a loop until it returns
 TRUE by going down a know versions list (or FALSE if we go
 the other way round).  The catch to this is that we need to
 maintain the list upon releases of Windows and possibly service
 packs.

With blessings, thank you!
Comment 2 Fan, Chun-wei 2015-01-20 07:50:55 UTC
Hi,

Any comments or suggestions about this new, simple-ish API?

Much appreciated.

With blessings, thank you!
Comment 3 Marc-Andre Lureau 2015-01-20 12:30:26 UTC
It looks like a nice API, although for consistency with other similar version check functions, it would be better named like "g_win32_check_windows_version". Also /G_WIN32_OS_WS/G_WIN32_OS_WORKSTATION would be nicer.

It would nicely fit in gtk+/gdk/win32/gdkwindow-win32.c instead of this rather unfriendly call:
              if (LOBYTE (g_win32_get_windows_version()) > 0x05 ||
                  LOWORD (g_win32_get_windows_version()) == 0x0105)
                {
                  /* Windows XP (5.1) or above */
                  wcl.style |= 0x00020000; /* CS_DROPSHADOW */

I don't see much other use in GNOME modules, spice-gtk does use it to set some security flags on named pipe with >= Vista. 

Tbh, I would make g_win32_get_windows_version() deprecated, since Microsoft doesn't recommend it: "[GetVersion may be altered or unavailable for releases after Windows 8.1. Instead, use the Version Helper APIs]"

Other than that, looks like a nice proposal.
Comment 4 Fan, Chun-wei 2015-01-21 04:20:23 UTC
Created attachment 295061 [details] [review]
gwin32: Add public API g_win32_check_windows_version()

Hello Marc-Andre,

(In reply to comment #3)
> It looks like a nice API,

Thanks, nice to hear that this could be useful.

> ...it would be better named like "g_win32_check_windows_version".
> Also /G_WIN32_OS_WS/G_WIN32_OS_WORKSTATION would be nicer.

Noted with thanks, updated in the patch

> I don't see much other use in GNOME modules, spice-gtk does use it to set some
> security flags on named pipe with >= Vista.

The other place I intend to use it is on DWM support in GTK+, as DWM is made standard and always activated for Windows 8 and later, which is used for composited windows.  Hopefully this can be handy in other places and applications as well, as it wraps some parts of VerifyVersionInfo().

> "[GetVersion may be altered or unavailable for releases
> after Windows 8.1. Instead, use the Version Helper APIs]"

Thanks, and noted in patch.  I re-worded some parts of the docs and made that API deprecated.  This is one of the major reasons behind this new API.

With blessings, thank you!
Comment 5 Matthias Clasen 2015-01-24 21:08:21 UTC
Review of attachment 295061 [details] [review]:

Looks good to me, otherwise.

::: glib/gwin32.h
@@ +142,3 @@
+  G_WIN32_OS_WORKSTATION,
+  G_WIN32_OS_SERVER,
+} GWin32OSType;

The comment should be turned into a doc comment:

/**
 * GWin32OSType:
 * @G_WIN32_OS_ANY:...

  ...


Also, don't forget to add the the new symbol (and the enum) to the sections.txt file
Comment 6 Fan, Chun-wei 2015-01-26 03:19:07 UTC
Created attachment 295407 [details] [review]
gwin32: Add public API g_win32_check_windows_version() (take ii)

Hello Matthias,

Here's the updated patch with the enum with GTK-Doc comments.

With blessings, thank you!
Comment 7 Fan, Chun-wei 2015-01-26 03:20:02 UTC
Created attachment 295408 [details] [review]
Update glib-sections.txt for g_win32_check_windows_version()

Hello Matthias,

As the name suggests, this is the update to glib-sections.txt.

With blessings.
Comment 8 Matthias Clasen 2015-01-26 20:04:58 UTC
Review of attachment 295407 [details] [review]:

looks good to me now.
Comment 9 Matthias Clasen 2015-01-26 20:05:27 UTC
Review of attachment 295408 [details] [review]:

sure
Comment 10 Fan, Chun-wei 2015-01-27 04:26:55 UTC
Hello Matthias,

Thanks for the reviews, the patches were committed as:
-Attachment 295407 [details]: be2d9b4
-Attachment 295408 [details]: e07cc89

Will close this bug now.

With blessings, thank you!