soffice: Make selected cell announcement more efficient by michaelweghorn · Pull Request #13233 · nvaccess/nvda

michaelweghorn · 2022-01-12T15:09:18Z

Link to issue number:

Summary of the issue:

Since PR #12849, information about selected cells in LibreOffice Calc is queried using the 'IAccessibleTable2' interface which is
supported from LibreOffice 7.3 on.
The call to the 'selectedCells' method on that interface requests a list of a11y objects for all currently selected cells, of which only the first and the last one are actually needed for the announcement of selected cells.
Since Calc spreadsheets have more than a billion cells, this is inefficient when many cells are selected and resulted in Calc becoming unresponsive.

Description of how this pull request fixes the issue:

Instead of using the 'selectedCells' method from the 'IAccessibleTable2' interface, the first and last selected cell are now retrieved using the 'accSelection' on the 'IAccessible' object of the table, which avoids that a11y objects for all other selected
cells have to be generated as well.

Testing strategy:

use LibreOffice Calc 7.3 or above
select all cells in LO Calc and check that NVDA announces coordinate + content of the first cell (A1) and the last cell (AMJ1048576) in the spreadsheet
test a few other selections in the spreadsheet (use shift + arrow keys to increase/decrease selection)

Known issues with pull request:

none

Change log entries:

Changes

- Announcement of selected cells for LibreOffice Calc is more efficient and no longer results in a Calc freeze when many cells are selected. (#13232)

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

michaelweghorn · 2022-01-12T15:10:38Z

@LeonarddeR : Any thoughts on this?
Unfortunately, I couldn't see any way to retrieve coordinates of the first and last selected cell without using IAccessibleTable2's selectedCells method.

AppVeyorBot · 2022-01-12T15:32:31Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9m7ogk89otl53v49/artifacts/output/nvda_snapshot_pr13233-24442,7615c8ee.exe

See test results for failed build of commit 7615c8eeb3

michaelweghorn · 2022-01-20T16:57:16Z

@LeonarddeR : Any thoughts on this? Unfortunately, I couldn't see any way to retrieve coordinates of the first and last selected cell without using IAccessibleTable2's selectedCells method.

Had another idea: While I don't see any way to do this via the IAccessibleTable2 interface, using IAccessible::get_accSelection and then the IEnumVARIANT to retrieve the first and last selected cell appears to work just fine. I plan to submit an updated PR using that approach.

LeonarddeR · 2022-01-20T18:40:24Z

I think your latter idea is great indeed, as long as get_accSelection is stable. I recall it had some buggery in the past though that might have been fixed lately.
I guess it makes sense to only annonce the first and last cell in a selection regardless of count, unless there's only one selected cell of course.

Link to issue number: Fixes nvaccess#13232 Summary of the issue: Since commit 32bf54e ("Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above (nvaccess#12849)"), information about selected cells in LibreOffice Calc is queried using the 'IAccessibleTable2' interface which is supported from LibreOffice 7.3 on. The call to the 'selectedCells' method on that interface requests a list of a11y objects for all currently selected cells, of which only the first and the last one are actually needed for the announcement of selected cells. Since Calc spreadsheets have more than a billion cells, this is inefficient when many cells are selected and resulted in Calc becoming unresponsive. Description of how this pull request fixes the issue: Instead of using the 'selectedCells' method from the 'IAccessibleTable2' interface, the first and last selected cell are now retrieved using the 'accSelection' on the 'IAccessible' object of the table, which avoids that a11y objects for all other selected cells have to be generated as well.

michaelweghorn · 2022-01-21T16:03:26Z

I think your latter idea is great indeed, as long as get_accSelection is stable. I recall it had some buggery in the past though that might have been fixed lately.

I have updated the PR accordingly, get_accSelection was working fine for me. Possibly, this commit on LibreOffice side may have fixed the issues encountered previously:
https://git.libreoffice.org/core/commit/1b94deea2c7648255e3efac08fd352f80c2bda06

I guess it makes sense to only annonce the first and last cell in a selection regardless of count, unless there's only one selected cell of course.

I completely agree. Announcing only the cell count for more than 2000 cells was just a poor alternative to not announcing anything when many cells are selected, to work around Calc becoming unresponsive when trying to generate a11y objects for all selected cells.

Side note: Unrelated to this change, there's an issue that NVDA runs into an exception when only a single cell is selected (e.g. when reducing a previously larger selection to just one cell). I plan to follow up on that next week.

michaelweghorn · 2022-01-25T15:11:12Z

Side note: Unrelated to this change, there's an issue that NVDA runs into an exception when only a single cell is selected (e.g. when reducing a previously larger selection to just one cell). I plan to follow up on that next week.

I have now created issue #13276 and PR #13277 for that.

From LibreOffice change https://gerrit.libreoffice.org/c/core/+/129201 on, the IEnumVARIANT returns an IDispatch rather than a child ID, s. a. discussion in nvaccess#13277 . Add a type check and handle both cases.

michaelweghorn · 2022-01-31T12:28:56Z

I have added another commit on top that makes this work for the case where the IEnumVARIANT returns an IDispatch instead of a child ID, so this will work with LibreOffice versions including change https://gerrit.libreoffice.org/c/core/+/129201 as well, s. discussion in PR #13277.

seanbudd

Apologies for the late review @michaelweghorn.
The approach generally looks good, with some suggestion on handling the types.
I've also fixed up the PR description.

source/appModules/soffice.py

meant to comment, not approve.

seanbudd · 2022-06-29T08:20:25Z

I'll look into taking these actions on soon if @michaelweghorn doesn't get the chance.

s. nvaccess#13233 (comment) > I think it would be best to capture the type specifically here, and raise an exception if an unexpected type is used.

michaelweghorn · 2022-06-29T14:18:42Z

@seanbudd: Thanks for the review and fixup of the PR description!

I couldn't apply all suggestions as is, s. inline comments. If anything is wrong with my setup, I'd be happy about any help to fix that.

seanbudd

Thanks @michaelweghorn

michaelweghorn · 2022-06-30T06:23:01Z

Thanks @seanbudd - also for helping out with my limited Python skills.

michaelweghorn requested a review from a team as a code owner January 12, 2022 15:09

michaelweghorn requested a review from michaelDCurran January 12, 2022 15:09

michaelweghorn marked this pull request as draft January 12, 2022 15:34

michaelweghorn force-pushed the michaelweghorn/issue13232_limit_queried_cells branch from e3f6f8b to f18ec84 Compare January 12, 2022 15:44

michaelweghorn marked this pull request as ready for review January 18, 2022 08:04

michaelweghorn marked this pull request as draft January 20, 2022 16:53

michaelweghorn force-pushed the michaelweghorn/issue13232_limit_queried_cells branch from f18ec84 to cfb0578 Compare January 21, 2022 15:44

michaelweghorn changed the title ~~soffice: Query at most 2000 Calc cells for selection~~ soffice: Make selected cell announcement more efficient Jan 21, 2022

michaelweghorn marked this pull request as ready for review January 21, 2022 16:07

michaelweghorn mentioned this pull request Jan 31, 2022

Add handling for accSelection returning IDispatch or negative child ID #13277

Merged

5 tasks

soffice: Handle IDispatch returned by IEnumVARIANT

84dd446

From LibreOffice change https://gerrit.libreoffice.org/c/core/+/129201 on, the IEnumVARIANT returns an IDispatch rather than a child ID, s. a. discussion in nvaccess#13277 . Add a type check and handle both cases.

feerrenrut added the app/open-office label Feb 16, 2022

feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022

feerrenrut requested review from feerrenrut and removed request for michaelDCurran June 9, 2022 05:44

seanbudd added the blocked/needs-code-review label Jun 20, 2022

seanbudd requested review from seanbudd and removed request for feerrenrut June 28, 2022 05:24

seanbudd self-assigned this Jun 28, 2022

seanbudd added the merge-early Merge Early in a developer cycle label Jun 28, 2022

seanbudd previously approved these changes Jun 29, 2022

View reviewed changes

source/appModules/soffice.py Outdated Show resolved Hide resolved

source/appModules/soffice.py Show resolved Hide resolved

source/appModules/soffice.py Show resolved Hide resolved

seanbudd removed the blocked/needs-code-review label Jun 29, 2022

Apply suggestion wrt type from review

7b44ac9

s. nvaccess#13233 (comment) > I think it would be best to capture the type specifically here, and raise an exception if an unexpected type is used.

add type hints to children

84536a8

seanbudd approved these changes Jun 29, 2022

View reviewed changes

seanbudd added 3 commits June 30, 2022 09:36

Merge branch 'master' into michaelweghorn/issue13232_limit_queried_cells

5ef060a

update changes

4ed42a9

import union

cb35d4a

seanbudd merged commit 0170323 into nvaccess:master Jun 30, 2022

nvaccessAuto added this to the 2022.3 milestone Jun 30, 2022

michaelweghorn deleted the michaelweghorn/issue13232_limit_queried_cells branch June 30, 2022 06:24

seanbudd mentioned this pull request Aug 4, 2022

bug: no announcements in cell selection mode in libreoffice #13961

Open

Uh oh!

Conversation

michaelweghorn commented Jan 12, 2022 • edited by seanbudd 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!

michaelweghorn commented Jan 12, 2022

Uh oh!

AppVeyorBot commented Jan 12, 2022

Uh oh!

michaelweghorn commented Jan 20, 2022

Uh oh!

LeonarddeR commented Jan 20, 2022

Uh oh!

michaelweghorn commented Jan 21, 2022

Uh oh!

michaelweghorn commented Jan 25, 2022

Uh oh!

michaelweghorn commented Jan 31, 2022

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd commented Jun 29, 2022

Uh oh!

michaelweghorn commented Jun 29, 2022

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

michaelweghorn commented Jun 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaelweghorn commented Jan 12, 2022 •

edited by seanbudd

Loading