For single column SysListViews, ignore column order array as it is not required by seanbudd · Pull Request #13761 · nvaccess/nvda

seanbudd · 2022-06-02T02:51:39Z

Link to issue number:

Summary of the issue:

Certain applications don't provide a column order array. For the case of single column list views, a column order array is unnecessary, as a default mapping can be assumed.

Description of how this pull request fixes the issue:

For single column list views, map the first column in the UX, to the first column index: i.e. [0] is the column order array. When fetching from a multi column list view, log an error if the column order array is unknown.

Testing strategy:

Manual testing

Test reading start menu items with StartAllBack/StartIsBack (STR in StartisBack application fails to speak menu items #13659)
Test STR in NVDA2022.1 fails to report list items in some ListViews #13735 (thanks @cary-rowen)

Known issues with pull request:

None

Change log entries:

Bug fixes:

- Fix reading single column list view items. (#13659, #13735)

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

CyrilleB79 · 2022-06-02T06:45:52Z

@seanbudd, if I understand correctly, there has been a regression for some applications (StartisBack and the one in 13735) between 2021.3.5 and 2022.1.
From a user point of view, it would be worth to have a change log item to indicate that this issue has been fixed (or worked around).

cary-rowen · 2022-06-02T07:30:36Z

Just curious, why isn't CI generating the build?

LeonarddeR · 2022-06-02T10:39:11Z

Just curious, why isn't CI generating the build?

Because this pr is made against another branch (not beta, rc, master)

seanbudd · 2022-06-03T03:57:58Z

@cary-rowen - you can test this build: https://ci.appveyor.com/api/buildjobs/tinlr9qek2w5cd8b/artifacts/output%2Fnvda_snapshot_try-fixSingleColumnSysListViews-25504%2Ccff35796.exe

Sorry for the delay - we had to fix the build system

cary-rowen · 2022-06-03T05:01:10Z

Hi, @seanbudd

I tested #13735 with the build you provided and it has been fixed.
thanks for your efforts.

Best

feerrenrut · 2022-06-03T06:52:32Z

This is generally because they are a single column list view, and as such, want developers to assume that the column order array is [0] in this case.

I think this is overstating our confidence. To ensure we don't mislead future dev work, can you change this to "we don't know for sure, but this might not be implemented specifically for single column list controls."

feerrenrut

Overall looks good.

feerrenrut · 2022-06-03T07:01:24Z

-
-	def _get__columnOrderArray(self) -> Optional[ctypes.Array]:
-		return self._getColumnOrderArrayRaw(self.columnCount)
+	def _getColumn(self, column: int) -> Optional[int]:


I think the name of this could be more descriptive, and probably needs some docs.
I think the column order is to allow for a presentation based indexing, that maps to a logical/internal indexing of columns. Probably because columns can be reordered.

I've changed the name to getMappedColumn and added a docstring.

cary-rowen · 2022-06-06T05:01:22Z

Will NV Access consider creating an NVDA2022.1.1?

…r array is not known

feerrenrut

Looks good.

feerrenrut · 2022-06-08T09:46:48Z

-		return self._getColumnOrderArrayRaw(self.columnCount)
+		If the column order array cannot be fetched from a multi-column SysListView ,
+		returns None as a mapped column cannot be determined.
+		"""


Suggested change

"""

@param presentationIndex: Zero based index for the column as presented to the user.

@return: The internal / logical column zero based index for the column.

"""

Fixed this: note that presentationIndex is actually a one based index, not zero based.

seanbudd requested a review from a team as a code owner June 2, 2022 02:51

seanbudd requested review from michaelDCurran and removed request for a team June 2, 2022 02:51

seanbudd changed the title ~~For SysListViews, map first column to first column when no known column order array~~ For single column SysListViews, ignore column order array as it is not required Jun 3, 2022

seanbudd requested review from feerrenrut and removed request for michaelDCurran June 3, 2022 05:11

feerrenrut suggested changes Jun 3, 2022

View reviewed changes

cary-rowen mentioned this pull request Jun 5, 2022

NVDA doesn't recognise list item in StartAllBack application #13771

Closed

seanbudd requested a review from feerrenrut June 6, 2022 00:38

seanbudd changed the base branch from cherryPickSysListView32Fix to master June 8, 2022 05:45

seanbudd added 3 commits June 8, 2022 15:45

For SysListViews, map first column to first column when a column orde…

33f9ecc

…r array is not known

ensure column count is 1 for mapping

222dbfc

improve mapped column docs

e167c76

seanbudd force-pushed the fixSingleColumnSysListViews branch from 05c0133 to e167c76 Compare June 8, 2022 05:46

feerrenrut approved these changes Jun 8, 2022

View reviewed changes

seanbudd added 2 commits June 9, 2022 10:48

improve func docstring

5c117bd

update changes

ccd31f6

seanbudd linked an issue Jun 9, 2022 that may be closed by this pull request

NVDA2022.1 fails to report list items in some ListViews #13735

Closed

seanbudd merged commit 47af401 into master Jun 9, 2022

seanbudd deleted the fixSingleColumnSysListViews branch June 9, 2022 02:52

nvaccessAuto added this to the 2022.2 milestone Jun 9, 2022

-		"""
+		@param presentationIndex: Zero based index for the column as presented to the user.
+		@return: The internal / logical column zero based index for the column.
+		"""

Uh oh!

Conversation

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

Uh oh!

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!

CyrilleB79 commented Jun 2, 2022

Uh oh!

cary-rowen commented Jun 2, 2022

Uh oh!

LeonarddeR commented Jun 2, 2022

Uh oh!

seanbudd commented Jun 3, 2022

Uh oh!

cary-rowen commented Jun 3, 2022

Uh oh!

feerrenrut commented Jun 3, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

cary-rowen commented Jun 6, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

seanbudd Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented Jun 2, 2022 •

edited

Loading