SysListView32 fully in process for cases where allocating memory in a process would otherwise be required#13271
Conversation
See test results for failed build of commit 6ac854916e |
|
Note that the failing system test is unrelated. |
seanbudd
left a comment
There was a problem hiding this comment.
The approach generally looks good to me, requesting @feerrenrut as a second reviewer due to C++ changes
| def _get__columnOrderArray(self): | ||
| coa=(c_int *self.columnCount)() | ||
| def _getColumnOrderArrayRaw(self, columnCount: int): | ||
| coa = (ctypes.c_int * columnCount)() |
There was a problem hiding this comment.
Could this variable be renamed to be clearer?
| coa = (ctypes.c_int * columnCount)() | |
| columnOrderArray = (ctypes.c_int * columnCount)() |
|
|
||
| def _get__columnOrderArray(self): | ||
| coa=(c_int *self.columnCount)() | ||
| def _getColumnOrderArrayRaw(self, columnCount: int): |
There was a problem hiding this comment.
can this (and other getColumnOrderArray functions) get a return type added?
There was a problem hiding this comment.
I'm not sure how since we're using a dynamic python class (ctypes array) as the return type.
There was a problem hiding this comment.
I think ctypes.Array works, however I could stand corrected.
Note that the following evaluates to True
import ctypes
columnOrderArray = (ctypes.c_int * 7)()
isinstance(columnOrderArray, ctypes.Array)
This comment was marked as outdated.
This comment was marked as outdated.
|
@feerrenrut I think I've addressed all your comments, not sure though. |
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
Link to issue number: Fix-up of PR #13271 Summary of the issue: PR #13271 moved most of the logic for retrieving content in syslistview32 controls in process. As part of this work _getColumnLocationRaw was changed to return an instance of ctypes.wintypes.RECT rather than one of locationHelper classes, additionally the returned coordinates were no longer normalized. Some places in NVDA expects _getColumnLocationRaw to return normalized coordinates - in particular this affects the list control in Tween and my add-on for Becky! Internet Mail. The error when trying to navigate the list was as follows: ERROR - scriptHandler.executeScript (21:29:59.073) - MainThread (1152): error executing script: <bound method GlobalCommands.script_reportCurrentFocus of <globalCommands.GlobalCommands object at 0x064ECAD0>> with gesture 'NVDA+tab' Traceback (most recent call last): File "scriptHandler.pyc", line 212, in executeScript File "globalCommands.pyc", line 2032, in script_reportCurrentFocus File "speech\speech.pyc", line 561, in speakObject File "speech\speech.pyc", line 604, in getObjectSpeech File "speech\speech.pyc", line 453, in getObjectPropertiesSpeech File "baseObject.pyc", line 42, in __get__ File "baseObject.pyc", line 146, in _getPropertyViaCache File "appModules\tween.pyc", line 26, in _get_name File "baseObject.pyc", line 42, in __get__ File "baseObject.pyc", line 146, in _getPropertyViaCache File "NVDAObjects\IAccessible\sysListView32.pyc", line 531, in _get_name File "NVDAObjects\IAccessible\sysListView32.pyc", line 459, in _getColumnContent File "appModules\tween.pyc", line 42, in _getColumnContentRaw TypeError: cannot unpack non-iterable RECT object Note that for these controls we cannot just use location returned from _getColumnLocation since indexes provided to it are not constant for a given column - they can change for example when user reorders columns. Description of how this pull request fixes the issue: Renames raw getters to rawInProc to clarify that these methods are used in-process Add docstrings to clarify when and why these methods are used The location normalization is once again performed by _getColumnLocationRaw In the Tween app Module annoying decorational Unicode symbol is removed from the columns header - this is not strictly related to the main issue.
Fix up of #13560, #13271 Summary of the issue: When out-of-process failed, an empty array was returned instead of None. This could have led to incorrect reporting of list items. For example, trying to get the contents of the second column of the list/table/report, the array is initialized with all values of zero, if used without being filled with valid data all columns will report the same thing. #13271 notes: >How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store. - If injection doesn't work there will be no `helperLocalBindingHandle`, out of process approach will be tried. - If an application like StartIsBack/StartAllBack doesn't handle the Windows Message properly then it doesn't matter if the message is sent in-process or out-of-process. Description of how this pull request fixes the issue: - When possible, use in process approach to get the column order array. It should be possible if there is a `helperLocalBindingHandle` - Don't return an array filled with zeros, return None when out-of-process fails.
Fix up of #13560, #13271 Summary of the issue: When out-of-process failed, an empty array was returned instead of None. This could have led to incorrect reporting of list items. For example, trying to get the contents of the second column of the list/table/report, the array is initialized with all values of zero, if used without being filled with valid data all columns will report the same thing. #13271 notes: >How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store. - If injection doesn't work there will be no `helperLocalBindingHandle`, out of process approach will be tried. - If an application like StartIsBack/StartAllBack doesn't handle the Windows Message properly then it doesn't matter if the message is sent in-process or out-of-process. Description of how this pull request fixes the issue: - When possible, use in process approach to get the column order array. It should be possible if there is a `helperLocalBindingHandle` - Don't return an array filled with zeros, return None when out-of-process fails.
Link to issue number:
Fixes #8175
Follow up of #11745
Fixes regression from #11469, #9873
Summary of the issue:
Getting list item content on 64 bit systems is sometimes problematic.
Description of how this pull request fixes the issue:
Moving text content fetching to in process in #11745 already fixed some but not all issues. The current pr adds:
In short, everything that required allocating memory within a process using VirtualAlloc
Testing strategy:
Known issues with pull request:
How likely are cases where injection doesn't work and the outproc variants succeed? I guess this is pretty unlikely, unless we're running under Windows Store.
Change log entries:
Bug fixes
Code Review Checklist: