Skip to content

Re-Introduce Table SayAll commands #14070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Conversation

mltony
Copy link
Contributor

@mltony mltony commented Aug 26, 2022

Link to issue number:

Fixes #13469.
Fixes #13927.

Summary of the issue:

Previous PR #13956 broke sayAll functionality in BookWorm (#13927) and therefore was reverted.
This PR undoes reverting, in other words it reintroduces table sayAll commands. It also contains a minor change that fixes sayAll in BookWorm.

Description of user facing changes

Reintroduces table sayAll commands.

Description of development approach

In my original PR #13956 I did a minor refactoring of SayAll classes, in order to avoid code reduplication and have a nice class inheritance. In order to achieve this I slightly rearranged calls to textInfo. This didn't affect functionality anywhere except for page turn detector in BookWorm.
Reverting to the original order of TextInfo calls at the cost of slightly less elegant code.

Testing strategy:

Tested manually with BookWorm and confirmed that pages turn successfully.
Tested on tables in browsers.

Known issues with pull request:

No system testing was performed as runsystemtests.bat script appears to be broken. When trying to run command mentioned as example from https://github.com/nvaccess/nvda/tree/master/tests/system on clean master:

HH) runsystemtests --include chrome --test "starts" ...
Ensuring NVDA Python virtual environment
call py -m robot --argumentfile "H:\mltony\nvda\tests\system\robotArgs.robot" --include chrome --test "starts" ... "H:\m
ltony\nvda\tests\system\robot"
[ ERROR ] Suite 'Nvda & Robot' contains no tests matching tags 'fakeTagToEnforceUsageOfInclude' or 'chrome', not matchin
g tag 'excluded from build' and matching name 'starts'.

Try --help for usage information.
Deactivating NVDA Python virtual environment

Change log entries:

New features

  • Added the following table commands:
    • Say all in column: NVDA+control+alt+downArrow
    • Say all in row: NVDA+control+alt+rightArrow
    • Read entire column: NVDA+control+alt+upArrow
    • Read entire row: NVDA+control+alt+leftArrow
      Changes
      Bug fixes
      For Developers

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
  • Security precautions taken.

@mltony mltony requested review from a team as code owners August 26, 2022 00:37
@mltony
Copy link
Contributor Author

mltony commented Aug 26, 2022

cc: @lukaszgo1, @Mohamed00

@seanbudd seanbudd mentioned this pull request Aug 26, 2022
5 tasks
@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Aug 26, 2022
@seanbudd seanbudd changed the title Table SayAll commands: second attempt Re-Introduce Table SayAll commands Aug 26, 2022
@Mohamed00
Copy link

Can confirm that Bookworm's page turning works properly.

@seanbudd seanbudd requested a review from feerrenrut August 29, 2022 03:06
@feerrenrut
Copy link
Contributor

No system testing was performed as runsystemtests.bat script appears to be broken. When trying to run command mentioned as example from https://github.com/nvaccess/nvda/tree/master/tests/system on clean master:

@mltony I think there are a couple of things going wrong here. The example assumes the existence of a test named "starts", since wildcards are accepted this could also be "starts*". The ellipsis was intended to mean "and any other arguments". Happy for a PR to clarify.

@seanbudd seanbudd merged commit 61428bc into nvaccess:master Aug 30, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.4 milestone Aug 30, 2022
CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Oct 25, 2022
seanbudd pushed a commit that referenced this pull request Oct 26, 2022
Summary of the issue:
In MSHtml container, e.g. NVDA's browseable message, the table edge is not reported. The following error is found instead:

ERROR - scriptHandler.executeScript (23:32:09.385) - MainThread (6444):
error executing script: <bound method DocumentWithTableNavigation.script_nextColumn of <virtualBuffers.MSHTML.MSHTML object at 0x0953F690>> with gesture 'alt+contrôle+flèche droite'
Traceback (most recent call last):
  File "scriptHandler.pyc", line 289, in executeScript
  File "documentBase.pyc", line 480, in script_nextColumn
  File "documentBase.pyc", line 417, in _tableMovementScriptHelper
  File "documentBase.pyc", line 362, in _tableFindNewCell
  File "virtualBuffers\__init__.pyc", line 674, in _getNearestTableCell
TypeError: cannot unpack non-iterable _TableCell object
Description of user facing changes
No more error reported at edge and the edge is reported.

Description of development approach
The dataclass documentBase._TableCell was added in #14070. More specifically, DocumentWithTableNavigation._getTableCellCoords now returns _TableCell. But some usages of the return of this function have been forgotten.
This PR fixes this.
michaelDCurran pushed a commit that referenced this pull request Jan 29, 2023
A typo in #14070 prevents navigated to the first column in a table in Firefox.

Description of user facing changes
Use table navigation commands to move to the first column in firefox works again

Description of development approach
Fix typo
seanbudd pushed a commit that referenced this pull request Mar 22, 2023
Fixes #14390

Summary of the issue:
With the merging of pr #14070 which added sayall in tables, SayAll in Kindle for PC would fail to continue reading after turning the page.
Technical:
Some of the code in the nextLine method in sayAll was refacted into a nextLineImpl method.
However, if nextLineImpl returned false, finish would be called and nextLIne would return. This would be correct for handling the case where there was no more text, or table cells, but not the case where the page has just been turned. In fact, text sayAll already called finish when there was no more text, so calling finish again was useless in the base case, but for page turns caused sayAll to abort prematurely after the page was turned.

Description of user facing changes
NVDA no longer fails to keep reading with sayAll after crossing a page boundary.

Description of development approach
In sayAll's nextLine method, removed the call to self.finish when nextLineImpl returns False, but ensured that table sayAll's nextLineImpl does call finish itself if there is no more table cells.
In other words, the self.finish call has been moved into the specific nextLineImpl method where it is needed, rather than running more broadly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Say all does not turn the page properly in bookworm Table sayAll commands
5 participants