Introduce winVersion.WinVersion class to record Windows version information, removing winVersion.isWin10 function#11909
Conversation
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.
See test results for failed build of commit be9e0f785c |
| self.assertGreaterEqual( | ||
| winVersion.getWinVer(), minimumWinVer | ||
| ) |
There was a problem hiding this comment.
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.
|
|
||
| def isWin10(version: int = 1507, atLeast: bool = True): | ||
| """ | ||
| @deprecated: use getWinVer().isWin10 instead. |
There was a problem hiding this comment.
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.
|
Hi, that could be a follow-up PR. Not only this must be done from winVersion module, other modules and add-ons must be told not to use the deprecated function. I’m happy to look into doing just that. CC @codeofdusk if he has any comments on this.
|
feerrenrut
left a comment
There was a problem hiding this comment.
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.
| 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 "" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hi,
Also, I'll go ahead and add WinVersion instances representing Windows 7 (SP1) and 8.x for sake of completeness.
Thanks.
| >= (other.major, other.minor, other.build) | ||
| ) | ||
|
|
||
| def isWin10(self, release: str = "1507", atLeast: bool = True): |
There was a problem hiding this comment.
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")There was a problem hiding this comment.
Hi,
Agreed, and that affirms Leonard's suggestion (removing isWin10 completely). I'll take care of this next week.
Thanks.
| @@ -28,38 +126,28 @@ def isUwpOcrAvailable(): | |||
|
|
|||
|
|
|||
| WIN10_VERSIONS_TO_BUILDS = { | |||
There was a problem hiding this comment.
As I understand these strings are actually 'release names'. If others agree, it would be good if we could rename this.
| WIN10_VERSIONS_TO_BUILDS = { | |
| WIN10_RELEASE_NAME_TO_BUILDS = { |
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.
…sed by anything other than WinVersion module.
See test results for failed build of commit 07fe7f0823 |
feerrenrut
left a comment
There was a problem hiding this comment.
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.
|
|
||
| # Windows >= Vista | ||
| isSupported = winVersion.major >= 6 | ||
| isSupported = winVersion.getWinVer().major >= 6 |
There was a problem hiding this comment.
Yep, I think this should be removed. Though I'm happy for this to happen in a followup.
|
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.
See test results for failed build of commit a39b4cc5a8 |
|
While concerning, system test failure is expected. Thanks. |
feerrenrut
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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/
|
Hi, preview builds have an expiration date, so it is safe to use build lists for emoji panel as these workarounds are applicable to 1709 and 1803 only (by the way, both are out of support for consumers, and 1803 is retiring this May). The biggest change to emoji panel support won’t be done until 2022 as Python 3.10’s pattern matching syntax is perfectly applicable to emoji panel interface detection in 1809 and later (UIA Automation Id strings can be compared). Thanks.
|
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.
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:
Additional functions in winVersion module:
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:
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.