winVersion.isWin10: accept both int and str for Windows 10 version parameter

[josephsl]

Link to issue number:

Fixes #11795

Summary of the issue:

Starting with Windows 10 October 2020 Update, version scheme is YYHx, not YYMM. This means going forward, integer version parameter will not work.

Description of how this pull request fixes the issue:

Windows 10 version map keys are now strings. As a result, isWin10 function can now accept either an integer or a string for version parameter. Also, edited log.error call to use formatted string literals.

Testing performed:

Tested in source code version under 20H2 with the following conditions:

• Passing in version parameter in integer and string.
• Passing in an invalid int or str.

Known issues with pull request:

None

Change log entry:

Changes for developers (optional):

winVersion.isWin10 function can now take a string denoting Windows 10 version.

Thanks.

[codeofdusk]

Oops, must've forgotten about this one...

Suggested change

	# Copyright (C) 2006-2020 NV Access Limited, Joseph Lee
	# Copyright (C) 2006-2020 NV Access Limited, Bill Dengler, Joseph Lee

[codeofdusk]

I don't like the sudden API change from build IDs to xxhx strings. Can we somehow support both? (in other words, 1903 is also 19H1)

[codeofdusk]

I like this

[josephsl]

Hi, I’ve been meaning to ask if you wanted your name to be included in the copyright header, and now I have the answer. Regarding build ID’s, YYHx became a thing with 20H2, therefore 1903 is kept as is (besides, in enterprise-level documentation, May 2019 Update is listed as Windows 10, version 1903). We can potentially add several duplicate keys to deal with 190x and 2004 in the short term. Thanks.

[codeofdusk]

@DHowett Any thoughts on how we should handle/refer to Windows versions?

[josephsl]

Hi,

Although Bill's pull request proposes interesting solutions, I still think that we should go with public information from Microsoft.

Thanks.

[feerrenrut]

I think this could become a mistake from and API sense. This will result in a very hard to untangle confusion between version and build.

It's also not clear on first glance (without reading the docs / implementation) that this function tells you if the OS is a particular version or greater or if it tells you that the versions string is a valid one for Windows 10.

To fix this it might be helpful to keep but deprecate the version keyword, and introduce matchesVersionOrLater: str and a matchesBuildOrLater: int keywords. Alternatively have two separate public functions (which could share a private implementation if it is appropriate).

Another alternative, would be a new getWinVer() function that returned a class type that handled comparison operators and could also be constructed by a build string / int. Allowing you to write code like"

if winVer('20H2') < getWinVer() < winVer('21H2'):
    print("in expected range or releases")

[feerrenrut]

Please could you also add unit tests as part of this PR.

[josephsl]

Hi, I think separate keyword arguments would be best given that sometimes we need a way to test NVDA inside Insider Preview builds. The “version” keyword could be deprecated in 2021.1 cycle. As for unit test request, I’ll do something about that. Thanks.

[codeofdusk]

This function was originally added to NVDA to ease (what is now) #10964, but with microsoft/terminal#8182 we might not even use it for that case any more. However, it is used for a few other things in NVDA (EnhancedTermTypedCharSupport for example).

[josephsl]

Hi, Windows 10 App Essentials add-on uses this function for feature update build checks, but I can instruct the add-on to check build range, which is way simpler than what we have now. Given this new information from Bill, I think a much better path forward would be deprecating and removing the entire Windows 10 check and versions to builds map altogether, reducing headache for everyone. If deprecation and removal is adopted by NV Access and contributors, I think we should do it in 2021 or 2022. Thanks.

[codeofdusk]

I agree, but contingent on how #10964 works out.

[josephsl]

Hi,

After thinking about Bill's comments, I'm wondering a much better approach would be detecting console host version? I know about a request to do this from Microsoft in terms of UIA Automation Id, but it breaks automated tests (Automation ID's are meant to be unique and it shines in code and tests where you need to look for a specific element).

As a reference, winVersion.isWin10 function is employed in NVDA Core in:

• Loading OneCore speech synthesizer
• Console support
• Adding additional UIA events

By the way, as of yesterday, Windows 10 App Essentials add-on was edited to use winVersion.winVersion.build instead of winVersion.isWin10 function based on Bill's comments.

Thanks.

[josephsl]

Hi,

After reading Reef's comment more closely, I think a much better approach would be a new winVersion.getWinVer function that returns a 3-tuple with major, minor, and build fields. This will allow us to not only deprecate isWin10 function and Windows 10 feature update build map, it will help us make more code fragments readable such as:

• Console support check in Advanced settings panel (gui.settingsDialogs)
• Touch support check
• OneCore synthesizer
• File Explorer

In short, the new getWinVer function will be a glorified sys.getwindowsversion().platform_version tuple - platform version tuple is incorrect under 1909 and 20H2 because they change a flag while keeping ntoskrnl.exe file version intact.

There are downsides and issues to consider:

• Why do we reinvent the wheel when winVersion.winVersion (or for that matter, sys.getwindowsversion() tuple) gives us what we want?
• Is it better to only check build (integer) which is guaranteed to increase as development progresses unless Microsoft decides to use a different type to represent the Windows build one day?
• Should we keep using what it could be a deprecated and potentially removable function just because a component needs it, and what if there is a much better way for that component (console support) to detect the runtime environment?
• What if NvDA drops support for Windows 7 and 8.x in the future i.e. runs only on Windows 10? If that happens, isWin10 function becomes redundant. We can scale that function back to checking for feature update builds only but then we return to the starting point of this pull request.

As for follow-up activities, several possibilities come to my mind:

1. Keep working on this pull request using suggestions from Reef while keeping Bill's comments in mind.
2. Use a new issue/PR pair to deal specifically with conhost.exe version detection.
3. Withdraw this pull request for now.
4. Create a new issue/PR pair to change isWin10 function call to winVersion.build checks throughout NVDA source code (and add-ons) provided that isWin10 is marked for deprecation and will be removed in a future NVDA release, again contingent on conhost version detection.

Of these, I'm torn between 1 and 3, the ideal outcome being 2 followed by 4.

Thanks.

[feerrenrut]

Why do we reinvent the wheel when winVersion.winVersion (or for that matter, sys.getwindowsversion() tuple) gives us what we want?

For concise semantics when comparing windows versions, also to abstract the details of these comparisons. Allow the underlying details to change without requiring changes to already written code, eg during a transition from '2004' to '20H2' schemes.

Is it better to only check build (integer) which is guaranteed to increase as development progresses unless Microsoft decides to use a different type to represent the Windows build one day?

This is one reason I was suggesting an abstraction. A type that creates a barrier between our internal windows version comparisons and the exact types returned by windows. I would be extremely surprised if older version schemes are ever modified, which means that if we have a type that can be created from any of the older version schemes, and be adapted to be created from any future version schemes, and it knows how these versions relate to each other, then we can be safe from API changes in the future.

Should we keep using what it could be a deprecated and potentially removable function just because a component needs it, and what if there is a much better way for that component (console support) to detect the runtime environment?

If we decide there is a better way to do this, we should deprecate the current function and update all core usages. Then remove the deprecated function in the next API breaking release.

What if NvDA drops support for Windows 7 and 8.x in the future i.e. runs only on Windows 10? If that happens, isWin10 function becomes redundant. We can scale that function back to checking for feature update builds only but then we return to the starting point of this pull request.

I think it makes sense to have a generic winVer type that can compare regardless of win 7, 8, 10 or other variants. One complication is thinking how the variants fit in, EG 10s and 10x.

[josephsl]

Hi, I think we can probe the registry to figure out edition differences (X, S mode, etc.). As for a WinVer type, I think a 3-tuple might be the best option for now. At this point, I think it would be best to create a different pull request, as continuing with this PR branch can create headaches due to changes made unless folks agree that we should continue the work via this PR. Thanks.

[feerrenrut]

I think a 3-tuple might be the best option for now.

What data would be in this 3-tuple? How would it be constructed? Convenience of construction from either '21H2' or '2004' or 18363.1198'?

I'd prefer a type where the underlying mechanism is not exposed, even if that underlying mechanism was a 3-tuple.

[josephsl]

Hi,

The 3-tuple will consist of major, minor, build, all integers, mirroring what we get with sys.getwindowsversion().platform_version with tweaks:

• If argument not present: just return (major, minor, build) for the release one is using.
• If argument in ("7", "8", "8.1"): return the appropriate data (6, 1, 7601 for Windows 7; the only version that will go through this route as of 2020 is 8.1 (6, 3, 9600), as 7 and 8 are no longer supported by Microsoft as far as consumer-level support is concerned).
• If argument in ("10", "1507"): return (10, 0, 10240)
• Else: check feature update to build map and return the resulting build number (i.e. for "20H2", return (10, 0, 19042))

Although some folks would ask us to include UBR (update build revision) data such as 19042.630, it is most useful if one wants to compare quality update level. And it turns out all the work is already done in Resource Monitor add-on: one can check Windows Registry to obtain this value, which could be included if people insist that UBR must be included. Besides, UBR is meaningless for Insider Preview builds, especially dev channel builds where UBR can either be 1 or 1000 (the former for release branch prep builds, the latter for true dev builds from rs_prerelease branch); even then, there is a way to determine if a build is a public stable build or not (again covered in Resource Monitor add-on, which probes another Windows Registry key that is only visible if one joins Windows Insider Program).

The reason for going with a 3-tuple are:

• Because all values are integers (unless Microsoft changes this in the future, which is really unlikely), it eases comparisons.
• The only thing that matters now (and has been since the release of first Windows 10 Insider Preview build in October 2014) is build number, which is guaranteed to increase, not decrease (Microsoft even includes a code path to warn users about "downgrading" to an earlier build in Windows 10 Setup program). This is true for all released versions of Windows, including latest Insider Preview build (currently 20257 as of time of this comment) despite showing gaps as Microsoft jumps builds before the next semi-annual dev cycle begins (the next such occurrence will be this December when 21H2/cobalt development begins inside Microsoft).

Thanks.

[feerrenrut]

I'm still not convinced that exposing the tuple is a good idea:

• It's hard to document the meaning / type of its elements
• When we need to consider a new piece of information EG 10x or 10s all usages have to be looked at and potentially changed. The usages are coupled to the data representation instead of an interface.
• There is a loss of type information.

[josephsl]

Hi, in this case, I think the most sensible path would be defining a WinVersion class. In addition to creating magic methods, it then allows winVersion.isWin10 to become a method inside the new winVersion.WinVersion class, along with defining properties on Windows 10 X and S mode checks and such. Besides, there’s a much better way to tell if users are running a given feature update and/or a given version – all that’s needed is a registry key, freeing us from the burden of defining feature update build numbers every six months. In other words, I’m willing to donate part of Resource Monitor add-on code (repo: https://github.com/josephsl/resourcemonitor) to NVDA Core provided that the WinVersion class and registry-based feature update build check is acceptable. Thanks.

[DHowett]

Sorry about the quiet from my side on this. I know I've owed an update for quite some time.

From the console host side, I can give the following advice:

For now, and for the foreseeable future, the only traditional console host that people will be using will be versioned along with the rest of Windows. We expect that folks running OpenConsole are few and far between, but that it would be safe to treat OpenConsole as "the newest possible conhost".

If there's a world where NVDA can make compatibility decisions based on individual application versions, that might make things a little easier. Conhost is versioned along with Windows, and so its VERSIONINFO resource will contain the 10.0.xxx build number (in easily-consumable form) indicating the last build of Windows on which it changed. OpenConsole will bear a 1.x or 2.x (or something lower than 10) version number pretty much forever.

I know about a request to do this from Microsoft in terms of UIA Automation Id, but it breaks automated tests (Automation ID's are meant to be unique and it shines in code and tests where you need to look for a specific element).

I consider this reason enough for us not to pollute its meaning by storing a version number in it 😄

Thanks.

Speaking as somebody who is not on the app compatibility team, and who has nothing to do with version numbers, a three-tuple is going to be fine for detecting platform component needs.

Servicing may make that complicated. I can't promise that you won't be caught out by leaving out the "UBR" version, but I can promise that determining what, if any, accessibility fixes went into a servicing version is going to be nigh-unto impossible.

[codeofdusk]

Hmmm: running nav.appModule.productVersion for OpenConsole yields 10.0.19041.1, so definitely not 1.x or 2.x!

@josephsl Is this the correct way to retrieve an app version?

[DHowett]

That's interesting. I wonder if appModule resolves to cmd/powershell (or whatever application has requested a console window?)

We use a specific API that will change which process appears to own a window, which might impact tools that try to map windows to processes.

(OpenConsole will only be 1.x or 2.x in release builds. Local builds will be unversioned!)

[josephsl]

Hi,

In reply to the above comment: yes, when you run a console program, the app module will say "cmd.exe" or "powershell.exe" i.e. the name of the executable will not be conhost.

Also, I'm working on a PR that will replace this one, and I hope to complete initial implementation by later today.

Thanks.

[feerrenrut]

Joseph, if you don't mind, I think we should close this PR and start a new conversation for the new work. It can of course reference this one to refer to how decisions were made.