App modules/product name and version: use kernel32::GetPackageFullName to obtain product info for immersive (hosted) apps#10114
Merged
michaelDCurran merged 4 commits intoSep 10, 2019
Conversation
Contributor
Author
|
Hi, If he is willing, CC @michaelDCurran |
Contributor
Author
|
Hi, A bit about platform_version attribute from winVersion.winVersion tuple: new in Python 3.7. This returns major, minor, and build and is intended to find out the actual platform version rather than the emulated one Python gets. In terms of using that attribute everywhere, not this year, but perhaps for 2020.1. Thanks. |
…ve (hosted) apps via kernel32::GetPackageFullName function. Re nvaccess#10108. On Windows 8 and later, some apps can run inside a container. This is the case for WinRT/UWP apps, some web apps, and converted desktop apps (such as Microsoft Office 365 downloaded from Microsoft Store). For these apps, kernel32.dll::GetPackageFullName returns the 'real' product info such as name and version. Because it'll be returned in a serialized string representation, parse the first two values (name and version). To accomodate this change, the former method of obtaining product name and version via file version info has been moved to an internal function inside product info setter method. This function will be invoked if: * This is Windows 7/Server 2008 R2 (no support for containers yet). * An immersive app that is really a native pap (such as File Explorer). * Converted desktop apps that will not expose version info via file version info structure (such as Notepad in 20H1 Preview build 18963 and later). For consistency with immersive app info structure, the modified function will return a 2-tuple that records product name and version in that order.
3632189 to
9c536bd
Compare
Contributor
Author
|
Hi, Another note on platform version tuple: in Version 1909, it'll say (10, 0, 18362). Note that this minor deviation does not affect this PR. Also, the PR is ready for review. Thanks. |
This was referenced Sep 8, 2019
michaelDCurran
requested changes
Sep 8, 2019
| # Some apps such as File Explorer says it is an immersive process but error 15700 is shown. | ||
| # Therefore resort to file version info behavior becuase it is not a hosted app. | ||
| # Others such as Store version of Office are not truly hosted apps, | ||
| # yet reutnrs an internal version anyway. |
| # which returns major, minor, build. | ||
| if winVersion.winVersion.platform_version >= (6, 2, 9200): | ||
| # Some apps such as File Explorer says it is an immersive process but error 15700 is shown. | ||
| # Therefore resort to file version info behavior becuase it is not a hosted app. |
| # This is needed in case immersive app package returns an error, | ||
| # dealing with a native app, or a converted desktop app. | ||
|
|
||
| def _executableFileInfo(): |
Member
There was a problem hiding this comment.
Please rename this to _getExecutableFileInfo()
Contributor
Author
|
Hi, coming right up, thanks for this prompt review.
From: Michael Curran <notifications@github.com>
Sent: Sunday, September 8, 2019 3:18 PM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] App modules/product name and version: use kernel32::GetPackageFullName to obtain product info for immersive (hosted) apps (#10114)
@michaelDCurran requested changes on this pull request.
_____
In source/appModuleHandler.py <#10114 (comment)> :
+ if not ctypes.windll.Kernel32.QueryFullProcessImageNameW(
+ self.processHandle, 0, exeFileName, ctypes.byref(length)
+ ):
+ raise ctypes.WinError()
+ fileName = exeFileName.value
+ fileinfo = getFileVersionInfo(fileName, "ProductName", "ProductVersion")
+ return (fileinfo["ProductName"], fileinfo["ProductVersion"])
+
+ # No need to worry about immersive (hosted) apps and friends until Windows 8.
+ # Python 3.7 introduces platform_version to sys.getwindowsversion tuple,
+ # which returns major, minor, build.
+ if winVersion.winVersion.platform_version >= (6, 2, 9200):
+ # Some apps such as File Explorer says it is an immersive process but error 15700 is shown.
+ # Therefore resort to file version info behavior becuase it is not a hosted app.
+ # Others such as Store version of Office are not truly hosted apps,
+ # yet reutnrs an internal version anyway.
typo
_____
In source/appModuleHandler.py <#10114 (comment)> :
+ exeFileName = ctypes.create_unicode_buffer(ctypes.wintypes.MAX_PATH)
+ length = ctypes.wintypes.DWORD(ctypes.wintypes.MAX_PATH)
+ if not ctypes.windll.Kernel32.QueryFullProcessImageNameW(
+ self.processHandle, 0, exeFileName, ctypes.byref(length)
+ ):
+ raise ctypes.WinError()
+ fileName = exeFileName.value
+ fileinfo = getFileVersionInfo(fileName, "ProductName", "ProductVersion")
+ return (fileinfo["ProductName"], fileinfo["ProductVersion"])
+
+ # No need to worry about immersive (hosted) apps and friends until Windows 8.
+ # Python 3.7 introduces platform_version to sys.getwindowsversion tuple,
+ # which returns major, minor, build.
+ if winVersion.winVersion.platform_version >= (6, 2, 9200):
+ # Some apps such as File Explorer says it is an immersive process but error 15700 is shown.
+ # Therefore resort to file version info behavior becuase it is not a hosted app.
typo
_____
In source/appModuleHandler.py <#10114 (comment)> :
"""
# Sometimes (I.E. when NVDA starts) handle is 0, so stop if it is the case
if not self.processHandle:
raise RuntimeError("processHandle is 0")
- # Create the buffer to get the executable name
- exeFileName = ctypes.create_unicode_buffer(ctypes.wintypes.MAX_PATH)
- length = ctypes.wintypes.DWORD(ctypes.wintypes.MAX_PATH)
- if not ctypes.windll.Kernel32.QueryFullProcessImageNameW(self.processHandle, 0, exeFileName, ctypes.byref(length)):
- raise ctypes.WinError()
- fileName = exeFileName.value
- fileinfo = getFileVersionInfo(fileName, "ProductName", "ProductVersion")
- self.productName = fileinfo["ProductName"]
- self.productVersion = fileinfo["ProductVersion"]
+ # Use an internal function for obtaining file name and version for the executable.
+ # This is needed in case immersive app package returns an error,
+ # dealing with a native app, or a converted desktop app.
+
+ def _executableFileInfo():
Please rename this to _getExecutableFileInfo()
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#10114?email_source=notifications&email_token=AB4AXEHBBNTBVFJ4SVGGR2TQIV2Y5A5CNFSM4IMTTBLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEAGDFY#pullrequestreview-285237655> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEFRIKUAD5NXDLDOEKTQIV2Y5ANCNFSM4IMTTBLA> .
|
…getExecutableFileInfo. Re nvaccess#10108. Reviewed by Mick Curran (NV Access): because of what the internal function does, it is better to give it a more descriptive name than just 'executable file info'.
LeonarddeR
reviewed
Sep 9, 2019
| # This is needed in case immersive app package returns an error, | ||
| # dealing with a native app, or a converted desktop app. | ||
|
|
||
| def _getExecutableFileInfo(): |
Collaborator
There was a problem hiding this comment.
I think I would prefer this to be a private appModule method instead of an internal function to _setProductInfo.
Contributor
Author
|
Hi, I once thought about doing that, but figured it’ll be used by this function alone. Let me do some thinking and make my decision soon. Thanks.
From: Leonard de Ruijter <notifications@github.com>
Sent: Monday, September 9, 2019 8:14 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] App modules/product name and version: use kernel32::GetPackageFullName to obtain product info for immersive (hosted) apps (#10114)
@LeonarddeR commented on this pull request.
_____
In source/appModuleHandler.py <#10114 (comment)> :
"""
# Sometimes (I.E. when NVDA starts) handle is 0, so stop if it is the case
if not self.processHandle:
raise RuntimeError("processHandle is 0")
- # Create the buffer to get the executable name
- exeFileName = ctypes.create_unicode_buffer(ctypes.wintypes.MAX_PATH)
- length = ctypes.wintypes.DWORD(ctypes.wintypes.MAX_PATH)
- if not ctypes.windll.Kernel32.QueryFullProcessImageNameW(self.processHandle, 0, exeFileName, ctypes.byref(length)):
- raise ctypes.WinError()
- fileName = exeFileName.value
- fileinfo = getFileVersionInfo(fileName, "ProductName", "ProductVersion")
- self.productName = fileinfo["ProductName"]
- self.productVersion = fileinfo["ProductVersion"]
+ # Use an internal function for obtaining file name and version for the executable.
+ # This is needed in case immersive app package returns an error,
+ # dealing with a native app, or a converted desktop app.
+
+ def _getExecutableFileInfo():
I think I would prefer this to be a private appModule method instead of an internal function to _setProductInfo.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#10114?email_source=notifications&email_token=AB4AXEGZ5BJLIF2ZUE7HXGLQIZR4PA5CNFSM4IMTTBLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEC2NLY#pullrequestreview-285583023> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEEHTMTKVDDNWD44V2TQIZR4PANCNFSM4IMTTBLA> .
|
Collaborator
|
The problem with making it private like this, is that noone else can use
the logic in that function. Making it a private method on the class at
least gives the most freedom to devs.
|
Contributor
Author
|
Hi, right, and also allows easy tuning later when the need arises. I’ll make necessary alterations today. Thanks for bringing that up.
From: Leonard de Ruijter <notifications@github.com>
Sent: Monday, September 9, 2019 10:12 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] App modules/product name and version: use kernel32::GetPackageFullName to obtain product info for immersive (hosted) apps (#10114)
The problem with making it private like this, is that noone else can use
the logic in that function. Making it a private method on the class at
least gives the most freedom to devs.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#10114?email_source=notifications&email_token=AB4AXEDI264BNLRLWTLSRDTQIZ7W3A5CNFSM4IMTTBLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6ILNEI#issuecomment-529577617> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEA27VHDSDM6K7YELI3QIZ7W3ANCNFSM4IMTTBLA> .
|
…ts own method in base pap module. Re nvaccess#10108. Suggested by Leonard de Ruijter (Babbage): transform executable file info function from an internal one to a private method in base app module for ease of future maintenance.
michaelDCurran
approved these changes
Sep 10, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fixes #4259
Fixes #10108
Summary of the issue:
Product information for immersive (hosted) apps (including those hosted by WWAHost) are incorrect or seen by NVDA as nonexistent. In some cases, converted desktop apps do not expose correct app info.
Description of how this pull request fixes the issue:
On Windows 8 and later, some apps can run inside a container. This is the case for WinRT/UWP apps, some web apps hosted by WWAHost, and converted desktop apps (such as Microsoft Office 365 downloaded from Microsoft Store). For these apps, kernel32.dll::GetPackageFullName returns the 'real' product info such as name and version. Because it'll be returned in a serialized string representation, parse the first two values (name and version).
To accomodate this change, the former method of obtaining product name and version via file version info has been moved to an internal function inside product info setter method. This function will be invoked if:
For consistency with immersive app info structure, the modified function will return a 2-tuple that records product name and version in that order.
Testing performed:
Tested with native and hosted apps, including NVDA from source, Outlook 365, Windows 10 Calculator and Store, Twitter web app and many others (also tested via Windows 10 App Essentials add-on). On Windows 8.1, WWAHost apps were used.
Known issues with pull request:
For some converted desktop apps, there will be product info conflict: file version info says one thing and package name says something else. This is most prominent in Office 365 apps (package version starts with 16051 but file info says 16.0), but can be worked around by modifying app modules to favor one approach over another.
Change log entry:
Changes:
On Windows 8 and later, NVDA will now report product name and version information for hosted apps such as apps downloaded from Microsoft Store using information provided by the app. (#4259, #10108)
Thanks.