No longer fail to read list of tweets in Tween by lukaszgo1 · Pull Request #13560 · nvaccess/nvda

lukaszgo1 · 2022-03-29T20:32:26Z

Opened against beta since this PR fixes regression introduced during 2022.1 development cycle.

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.

Testing strategy:

Made sure that lists in Tween are once again readable and that for the date column the displayed content is used.
Repeated these tests with reordered columns - made sure there is no change.
Smoke tested various other list controls.
Inspected usages of other methods modified in PR SysListView32 fully in process for cases where allocating memory in a process would otherwise be required #13271 no more issues were noticed.

Known issues with pull request:

None known

Change log entries:

None needed if this is included in 2022.1

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

* Improve clarity of in-process and out of porcess methods * normalize location in the raw method to fix reading in Tween * Remove unnecesary Unicode arrow from headers in Tween

lukaszgo1 · 2022-03-29T20:32:57Z

@LeonarddeR Requesting your review as well.

seanbudd · 2022-03-30T05:28:53Z

This regression ideally should be fixed before rc. Are we aware of any other appModules or core functionality affected?
The PR looks good to be merged into alpha as-is, but I have some concerns with merging this into beta.

As this is quite late in the beta cycle, changing code in sysListView32 is quite risky and could introduce new, similar issues.

Is it possible to fix this by changing appModules.tween? Alternatively, instead of changing existing functions in sysListView32, create new ones. A follow up PR to master can clean this up.

LeonarddeR

Thanks for taking this

lukaszgo1 · 2022-03-30T11:37:25Z

This regression ideally should be fixed before rc.

Agreed.

Are we aware of any other appModules or core functionality affected?

As mentioned in the PR description this affects my add-on for Becky Internet Mail.

The PR looks good to be merged into alpha as-is, but I have some concerns with merging this into beta.

As this is quite late in the beta cycle, changing code in sysListView32 is quite risky and could introduce new, similar issues.

Most of the changes in sysListView32 is just docstrings and straight method renames. If some specific change there concerns you in particular I'm happy to discuss. Also note that cleaning this up for 2022.2 may break backwards compatibility, so it seems saner to try to get this right for 2022.1 than merge something half-baked and then fix it up afterwards.

Is it possible to fix this by changing appModules.tween? Alternatively, instead of changing existing functions in sysListView32, create new ones. A follow up PR to master can clean this up.

It should be possible to separate the code which normalizes instances of ctypes.wintypes.RECT into a separate method, use it in app Module for Tween and document that _getColumnLocationRaw returns raw coordinates, but this seems pretty ugly to me.

lukaszgo1 · 2022-03-30T12:52:35Z

Additionally after PR #13271 if the app module developer wants to overwrite one of the list methods which operates on raw indexes they have to shadow both in and out of process variants which should not be necessary. With this PR they can once again overwrite just a raw getter similar to what was possible in 2021.3.

seanbudd · 2022-03-30T22:49:09Z

Also note that cleaning this up for 2022.2 may break backwards compatibility, so it seems saner to try to get this right for 2022.1 than merge something half-baked and then fix it up afterwards.

All the functions changed here are underscored functions, not considered part of our stable, public API.
Even the getter _get__columnOrderArray maps to _columnOrderArray.

If add-on authors such as yourself need to rely on any of these methods, we should consider making them part of the public API. It's too late to do this in 2022.1, but public functions which mirror required functions could be introduced in 2022.2.

Most of the changes in sysListView32 is just docstrings and straight method renames. If some specific change there concerns you in particular I'm happy to discuss.

There's no specific concerns, just that this is a risky core change late in the beta period.
After merging this, I would want to add at least another week to the beta period.
This is fine, but should be avoided where possible.
I'll follow up later today on whether or not we can go ahead with this as-is.

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 PR 13271.

22ab86c

* Improve clarity of in-process and out of porcess methods * normalize location in the raw method to fix reading in Tween * Remove unnecesary Unicode arrow from headers in Tween

lukaszgo1 requested a review from a team as a code owner March 29, 2022 20:32

lukaszgo1 requested review from seanbudd and removed request for a team March 29, 2022 20:32

seanbudd modified the milestone: 2022.1 Mar 30, 2022

LeonarddeR approved these changes Mar 30, 2022

View reviewed changes

seanbudd approved these changes Mar 31, 2022

View reviewed changes

seanbudd changed the title ~~Fix-up of PR 13271 - no longer fail to read list of tweets in Tween~~ No longer fail to read list of tweets in Tween Mar 31, 2022

seanbudd merged commit 08797d3 into nvaccess:beta Mar 31, 2022

lukaszgo1 deleted the tweenAppModSysListView32InProcFixUp branch March 31, 2022 09:37

seanbudd mentioned this pull request May 3, 2022

Improve logging and typing for sysListView32 #13657

Merged

7 tasks

seanbudd mentioned this pull request May 26, 2022

Cherry-pick f90caa92: Improve logging and typing for sysListView32 (#13657) #13741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

No longer fail to read list of tweets in Tween#13560

No longer fail to read list of tweets in Tween#13560
seanbudd merged 1 commit into
nvaccess:betafrom
lukaszgo1:tweenAppModSysListView32InProcFixUp

lukaszgo1 commented Mar 29, 2022

Uh oh!

lukaszgo1 commented Mar 29, 2022

Uh oh!

seanbudd commented Mar 30, 2022

Uh oh!

LeonarddeR left a comment

Uh oh!

lukaszgo1 commented Mar 30, 2022

Uh oh!

lukaszgo1 commented Mar 30, 2022

Uh oh!

seanbudd commented Mar 30, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lukaszgo1 commented Mar 29, 2022

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

lukaszgo1 commented Mar 29, 2022

Uh oh!

seanbudd commented Mar 30, 2022

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Mar 30, 2022

Uh oh!

lukaszgo1 commented Mar 30, 2022

Uh oh!

seanbudd commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanbudd commented Mar 30, 2022 •

edited

Loading