Skip to content

Introduce winVersion.WinVersion class to record Windows version information, removing winVersion.isWin10 function#11909

Merged
feerrenrut merged 57 commits into
nvaccess:masterfrom
josephsl:WinVersionClass
Mar 25, 2021
Merged

Introduce winVersion.WinVersion class to record Windows version information, removing winVersion.isWin10 function#11909
feerrenrut merged 57 commits into
nvaccess:masterfrom
josephsl:WinVersionClass

Conversation

@josephsl

@josephsl josephsl commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

Hi,

I think it would be best to target 2021.1:

Link to issue number:

Closes #11795
Closes #11837
Closes #11933
Replaces #11796
Replaces #11799

Summary of the issue:

Prior to October 2020 Update, Windows 10 feature updates (releases) were integers. Starting with October 2020 Update (build 19042), it is now a string, denoting which half of the year the given release was made public e.g. Version 20H2 denotes second half of 2020. This breaks winVersion.isWin10 function as it expects integers. Also, with the impending release (or advent) of Windows 10 X and in order to better detect Windows 10 in S mode, it became crucial to have a central location to record Windows version information and other properties.

Description of how this pull request fixes the issue:

Introduce winVersion.WinVersion class to record the following Windows version information:

  • Major version (integer)
  • Minor version (integer)
  • Build (integer)
  • Service pack (string)
  • Product type (string): one of "workstation", "domain controller", "server".
  • Version comparison operators (using functools.total_ordering decorator)
  • A repr of Windows release, service pack (if any), major.minor.build tuple, and product type, and private helper methods to obtain these.

Additional functions in winVersion module:

  • getWinVer: return WinVersion instance recording the current version of Windows in use.
  • getWinVerFromString: returning an instance of WinVersion class based on a string recording major.minor.build e.g. 6.1.7601.

winVersion module will also come with constants representing Windows releases to WinVersion class instances, as well as removing old winVersion.winVersion (sys.getwindowsversion() tuple) and winVersion.winVersionText to reduce developer confusion. With these changes, winVersion.isWin10 function is marked for removal in 2021.1, and affected code paths throughout NVDA will be edited.

Testing performed:

Tested with:

  • Source code (from NVDA itself and via Python interpreter)
  • Unit tests

Known issues with pull request:

If this PR is accepted, various parts of NVDA and add-ons calling winVersion.isWin10 and winVersion.* must be modified to use constants for comparisons. Also, add-ons using old winVersion.winVersion and its properties must be modified to call getWinVer() and friends (notably, this affects Windows 10 App Essentials add-on, for which an issue will be created on that add-ons' repo once this PR is accepted).

Change log entry:

Changes for developers:

getWinVer function was added to winVersion module to obtain a record of the version of Windows currently in use.
isWin10 function found in winVersion module is removed. To check for a specific Windows 10 release, compare winVersion.getWinVer() with a specific winVersion.WIN10_* constants.

Introduce winVersion.WinVersion class to serve as a hub of Windows version information for the system. The class (wrapped inside functools.total_ordering) will record version major, minor, build, and service pack information if present. It will also include a way to check for specific Windows 10 releases i.e. transplant winVersion.isWin10 function to winVersion.WinVersion.isWin10 method (later commit), as well as preparing for other checks such as Windows 10 X, Windows 10 on S mode, server, and others in the future. The class itself is wrapped inside functools.total_ordering decorator so that instnaces (Windows version data) can be compared easily.
Introduce winVersion.getWinVer function to return a WinVersion object representing the version info for the curretn system (version major, minor, build, service pack). This can be used to check for specific Windows releases (including Windows 10 feature updates) and eases comparisons among version info objects.
…nvaccess#11837.

winversion.getWinVerFromVersionText function can be used to construct WinVersion class instance from the Windows version string of the form major.minor.build i.e. '10.0.19041' will return a WinVersion class instance with major=10, minor=0, build=19041. This is the text counterpart to getWinVer function and is meant to prepare for a future scenario where an add-on or other NVDA components can perform actions based on a specific version text given.
Add unit tests for the following:
* winVersion.getWinVer (compared against what Python returns through sys.getwindowsversion)
* Convert a string representation of a Windows release to WinVersion class instance
* See if a specific major.minor.build is returned for a given release
…ally, greater than or equal i.e. newer Windows version). Re nvaccess#11837
…tionary. Re nvaccess#11795.

Convert Windows 10 release dictionary from an int:int map into an str:int map, necessary due to Version 20H2 (official documentation from Microsoft refers to October 2020 Update as 20H2, not 2009 as repoted by Windows Registry). This change is also necessary to tweak winVersion.isWin10 function (deprecated, however) to support integer arguments.
… Re nvaccess#11837.

Transfer the mechanism behind winversion.isWin10 function as a WinVersion class method. The new method takes string argument for release to properly recognize 20H2 (October 2020 Update). In order to guarantee compatibility for up to a year, '2009' will be transformed into '20H2' inside the new class method. Also, raise an exception instead of returning False if a given release is not a publicly released version of Windows 10.
…nstead. Re nvaccess#11837.

Deprecate winVersion.isWin10, replaced by calling getWinVer().isWin10 function. Because the latter takes strings, convert version integers into strings before calling the WinVersion class method.
…11837.

Test two aspects of isWin10 class method:
* Testing to see if running on Windows 10. On Windows 7 and 8.x, it will say 'False' by calling assertFalse.
* Test if the method raises ValueError if a nonexistent version such as '2003' is passed in.
@josephsl josephsl changed the title Introduce winVersion.WinVersion class to record Windows version information, deprecating winVersion.isWin10 function in the process Introduce winVersion.WinVersion class to record Windows version information, deprecating winVersion.isWin10 function Dec 7, 2020
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit be9e0f785c

Comment thread tests/unit/test_winVersion.py Outdated
Comment on lines +47 to +49
self.assertGreaterEqual(
winVersion.getWinVer(), minimumWinVer
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests shouldn't be tied to data specific the machine they are running on, replace winVersion.getWinVer with a hardcoded version.

… getWinVer when checking operators. Re nvaccess#11837.

An important reminder from Reef Turner (NV Access): make unit tests system-independent i.e. do not use winVersion.getWinVer function when testing operators. Instead, test 'Windows 8' a.k.a. first version to support audio ducking.
Comment thread source/winVersion.py Outdated

def isWin10(version: int = 1507, atLeast: bool = True):
"""
@deprecated: use getWinVer().isWin10 instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree about this approach and I think it makes sense, we can as well remove this entirely as part of the 2021.1 release.

@josephsl

josephsl commented Dec 8, 2020 via email

Copy link
Copy Markdown
Contributor Author

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking promising, thanks @josephsl

Can the winVersion variable (line 11) be removed (made private or be made into a WinVersion instance), or at least deprecated? I think the similar name and purpose introduces unnecessary confusion.

As part of this it would be good to be clear on the main uses cases for developers. I think generally it is easier for a developer to read / understand code that uses the release name (eg "20H2") than it is to work with the version 3-tuple. But, it is necessary to be able to convert in both directions. The design of this should encourage the in code use of the release name, while under the hood the version tuple is used.

I think one thing that is missing here is pretty printing the WinVersion class for the log. This should include both the tuple and the release string.

Comment thread source/winVersion.py Outdated
Comment on lines +36 to +62
if release in (None, ""):
self.major = major
self.minor = minor
self.build = build
elif release == "7":
self.major = 6
self.minor = 1
self.build = 7601
elif release == "8":
self.major = 6
self.minor = 2
self.build = 9200
elif release == "8.1":
self.major = 6
self.minor = 3
self.build = 9600
elif release in ("10", "1507"):
self.major = 10
self.minor = 0
self.build = 10240
elif release in WIN10_VERSIONS_TO_BUILDS:
self.major = 10
self.minor = 0
self.build = WIN10_VERSIONS_TO_BUILDS[release]
else:
raise ValueError("Cannot create Windows version information for the specified release")
self.servicePack = "1" if release == "7" else ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to put the heuristics based approach in another function. In a similar way to getWinVerFromVersionText. I.E. getWinVerFromReleaseName

Perhaps these methods should be 'static' methods on the WinVersion class. So then a client would write:

startOfSupportForX = WinVersion.fromVersionText("10.0.10240")
startOfSupportForY = WinVersion.fromReleaseName("20H2")

Also, for ease of use/reading, it would be helpful to define module level constants for the common versions:

WIN10_20H2 = WinVersion.fromReleaseName("20H2")
# Allows you to write code like this
if WinVersion.getWinVer() > WIN10_20H2:
    print("Welcome to the future.")

Actually these constants would make the fromReleaseName method irrelevant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I agree with the latter point - using a pool of Windows versions (constants) not only saves us from defining a function to handle complex nature of version checks, it allows easy maintenance in the future in that it will let future contributors define a WinVersion instance representing a specific release. It also makes any part of nVDA calling winVersion module easier to read (app module handler is a key example where we check for Windows 7 versus 8 in some places and these use platform version tuple).

Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Also, I'll go ahead and add WinVersion instances representing Windows 7 (SP1) and 8.x for sake of completeness.

Thanks.

Comment thread source/winVersion.py Outdated
>= (other.major, other.minor, other.build)
)

def isWin10(self, release: str = "1507", atLeast: bool = True):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the vast range of Win10 releases (which looks like it will grow indefinitely) I think this function looses meaning. I think code that used this would be better off being written like:

from winVersion import WIN10_1507, getWinVer
if getWinVer() >= WIN10_1507:
    print("On Win 10")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Agreed, and that affirms Leonard's suggestion (removing isWin10 completely). I'll take care of this next week.

Thanks.

Comment thread source/winVersion.py Outdated
@@ -28,38 +126,28 @@ def isUwpOcrAvailable():


WIN10_VERSIONS_TO_BUILDS = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand these strings are actually 'release names'. If others agree, it would be good if we could rename this.

Suggested change
WIN10_VERSIONS_TO_BUILDS = {
WIN10_RELEASE_NAME_TO_BUILDS = {

@josephsl josephsl changed the title Introduce winVersion.WinVersion class to record Windows version information, deprecating winVersion.isWin10 function Introduce winVersion.WinVersion class to record Windows version information, removing winVersion.isWin10 function Dec 11, 2020
nvaccess#11837.

Define constants representing Windows releases to WinVersion instances (i.e. WIN7_SP1 = WinVersion(major,6, minor=1, build=7601, servicePack='1')). This eases comparisons from other NVDA modules and add-ons, along with removing the ned for a dedicated Windows 10 feature update checks. Note that server releases are not specified to avoid complicating release mappings, especially more so for Windows Server 2016 and later which are based on specific Windows 10 releases with server components added. Also, Windows 7 original release (build 7600) was added for sake of completeness.
…inVer, removing the need to look up old winVersion tuple. Re nvaccess#11837.

Rather than using now old winVersion tuple, call sys.getwindowsversion function directly from getWinVer function.
…r supported OS and full screen magnification.

Testing getWinVer function: compare getWinVer output with that of Windows release constants when checking for supported OS (Windows 7 SP1) and full screen magnification (Windows 8).
…ccess#11837.

Rather than using winVersion.isWin10, check Windows 10 release constants (e.g. sinWversion.WIN10_1607) directly, as these constants will record major, minor, build. Specifically:
* UIA handler and OneCore synthesizer: check winVersion.WIN10
* Windows Console support: check winVersion.WIN10_1607
* Emoji panel in May 2019 Update: check winVersion.WIN10_1903
…s#11837.

Remove winVersion.isWin10 function, both from the module and from WinVersion class, as it is now possible to compare releases via constants. Also, remove unit tests associated with this function.
…11837.

Remove 'release' from constructor argument and add 'product type' to mirror old winVersion.winVersion tuple data. The bulk of the old constructor (namely definig major.minor.build for a specific release) will be transferred to a new static method (next commit).
… map. Re nvaccess#11837.

Suggested by Reef Turner (NV Access): clarify that Windows 10 versions to builds map clearly records release names.
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 07fe7f0823

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that fromVersionText and fromReleaseName aren't used in NVDA I think they should be removed. If people wish to make comparisons with other versions, the WinVersion class can be constructed manually.

Comment thread source/easeOfAccess.py

# Windows >= Vista
isSupported = winVersion.major >= 6
isSupported = winVersion.getWinVer().major >= 6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this should be removed. Though I'm happy for this to happen in a followup.

Comment thread tests/unit/test_winVersion.py Outdated
@josephsl

josephsl commented Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Hi,

I'll take care of Ease of Access in a follow-up PR.

Thanks.

…nel releases (1709 and 1803) instead of using builds directly. Re nvaccess#11795.

Comment from Reef Turner (NV Access): rather than using getWinVer().build, define variables to hold classic emoji panel releases (1709 = initial release, 1803 = feature update with proper window open event) and test membership.
…. Re nvaccess#11795.

As fromVersionText and fromReleaseName are not used anywhere, remove these static methods.
… rework nonExistentVersion test (assignment test). Re nvaccess#11795.
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit a39b4cc5a8

@josephsl

Copy link
Copy Markdown
Contributor Author

While concerning, system test failure is expected. Thanks.

@feerrenrut feerrenrut self-requested a review March 18, 2021 09:55

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks very close to ready!

with self.assertRaises(ValueError):
winVersion.WinVersion.fromReleaseName("2003")
with self.assertRaises(AttributeError):
may2020Update = winVersion.WIN10_2003 # NOQA: F841

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment above this line to specify what F841 is.

emojiPanelWindowOpenEvent = winVersion.WIN10_1803
if (
winVersion.getWinVer().build <= 17134
winVersion.getWinVer() in (emojiPanelInitial, emojiPanelWindowOpenEvent)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be misunderstanding (I'm not that familiar with the timeline of the emoji panel), but wouldn't this be more accurate as a comparison for a range of values eg emojiPanelInitial <= winVersion.getWinVer() <= emojiPanelWindowOpenEvent. Or is it actually accurate that only for these two precise builds this alternative behavior was required.

I think generally it would be safer to do these comparisons as ranges with limits where MS made / removed things.

I ask because there are other "release" builds between, but also in case there could have been (or even could still be) people running insider builds that may still benefit from this that don't exactly equal any of the reference build numbers listed in the constants. For instance running 17763: https://blogs.windows.com/windows-insider/2018/09/18/announcing-windows-10-insider-preview-build-17763/

@josephsl

josephsl commented Mar 25, 2021 via email

Copy link
Copy Markdown
Contributor Author

Comment from Reef Turner (NV Access): use comparisons when checking builds. Although Windows Insider Preview builds come with expiration dates, it might be possible that not all Insiders are running latest build.

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @josephsl

@feerrenrut feerrenrut added the deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release label Mar 25, 2021
@feerrenrut feerrenrut merged commit ff79f51 into nvaccess:master Mar 25, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Mar 25, 2021
@josephsl josephsl deleted the WinVersionClass branch March 25, 2021 11:07
@seanbudd seanbudd mentioned this pull request Apr 23, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release

Projects

None yet

6 participants