Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introducing table sayall commands #13670

Merged
merged 36 commits into from Jul 13, 2022
Merged

Introducing table sayall commands #13670

merged 36 commits into from Jul 13, 2022

Conversation

mltony
Copy link
Contributor

@mltony mltony commented May 7, 2022 •
edited

Link to issue number:

Fixes #13469

Summary of the issue:

Feature request: add table sayAll commands to read rows and columns.

Description of how this pull request fixes the issue:

Adds 4 commands:

  • SayAll from current cell horizontally to the right.
  • SayAll from current cell vertically down.
  • Speak current column from top to bottom without moving caret.
  • speak current row from left to right without moving caret.

Additionally:

  • Refactored class _TextReader in sayAll.py:
    • Refactored out a few methods for different types of cursor and made them abstract.
    • Added implementations for review and caret cursor types and also for new table cursor type.
  • Refactored class DocumentWithTableNavigation:
    • Refactored cell moving logic as method _tableFindNewCell, which is used in both simple table commands and sayAll commands.
    • Added _TableCell class to replace 5-element tuples that contain information about table cells.

Testing strategy:

  • Tested sayall commands in Chrome, Firefox, MSWord (both browse and focus mode); tested with merged cels to check that they are handled correctly during sayAll.
  • Tested normal table navigation commands to avoid regression.
  • Tested sayAll commands in Chrome, Firefox, MSWord to avoid regressions.

Known issues with pull request:

  • Doesn't support Excel tables.
  • Doesn't support table-like list views.

Change log entries:

New features

  • Added table sayAll commands.
    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

@AppVeyorBot
Copy link

See test results for failed build of commit 519d8c9288

@mltony
Copy link
Contributor Author

mltony commented May 7, 2022

I see FAIL: Translation comments check. Not quite sure how to investigate that. It suggests to find more information in the build log, however among 5k lines of log all unit tests seem to succeed and no obvious translation-related errors can be found.

@josephsl
Copy link
Collaborator

josephsl commented May 7, 2022 via email

@mltony mltony marked this pull request as ready for review May 7, 2022 21:27
@mltony mltony requested review from a team as code owners May 7, 2022 21:27
@mltony
Copy link
Contributor Author

mltony commented May 7, 2022

Fixed translation check.

@cary-rowen
Copy link
Contributor

Great job, I tested it and it seems to be performing well so far.

@seanbudd seanbudd requested a review from feerrenrut May 11, 2022 04:34
@CyrilleB79
Copy link
Collaborator

I have just tested the snapshot. It's very nice and works well! Thanks.

One additional point to clarify:
You have not implemented these commands in the list views where table commands can operate. Is it on purpose?
Since this PR already provides fully functional new features, the request can be handled in a subsequent PR if you prefer.
In any case, please indicate if and what you want to do something for list view commands.

@feerrenrut
Copy link
Contributor

@mltony Please consider adding unit and/or system tests for this behavior.

@seanbudd
Copy link
Member

seanbudd commented Jun 2, 2022

We won't take this on until there is system tests for this behaviour and unit tests where possible.
We will schedule this work if nobody else picks this up by that time.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jun 2, 2022
@seanbudd seanbudd marked this pull request as draft June 2, 2022 06:58
@mltony
Copy link
Contributor Author

mltony commented Jun 2, 2022

Yes, I will work on that. Sorry for delay. I got overwhelmed by projects at work, but hopefully next month I'll have some time to address all the comments.

@seanbudd
Copy link
Member

Is this ready for review - i.e. no longer a draft?

@mltony mltony marked this pull request as ready for review July 12, 2022 19:05
@mltony
Copy link
Contributor Author

mltony commented Jul 12, 2022

Yes, ready for review.

@seanbudd seanbudd self-requested a review July 12, 2022 23:57
@seanbudd
Copy link
Member

Thanks @mltony - I've pushed some minor changes to fix up the review comments missed in source/documentBase.py.

Qchristensen
Qchristensen approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User guide changes read well, good work!

@AppVeyorBot
Copy link

See test results for failed build of commit f967268ee3

@seanbudd seanbudd merged commit 82a4f86 into nvaccess:master Jul 13, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jul 13, 2022
This was referenced Jul 13, 2022
Open
Closed
seanbudd added a commit that referenced this pull request Jul 13, 2022
@seanbudd
Fix up #13670
d7da020
@seanbudd seanbudd mentioned this pull request Jul 13, 2022
Merged
5 tasks
seanbudd added a commit that referenced this pull request Jul 14, 2022
@seanbudd
Fix up #13670 (#13901)
432579d 
There was a small typo in #13670

Description of development approach
Fixes _TableCell.collSpan usage to _TableCell.colSpan
@lukaszgo1 lukaszgo1 mentioned this pull request Jul 20, 2022
Closed
seanbudd added a commit that referenced this pull request Jul 25, 2022
@seanbudd
Revert "Fix up #13670 (#13901)"
023c256 
This reverts commit 432579d.
seanbudd added a commit that referenced this pull request Jul 25, 2022
@seanbudd
Revert "Introducing table sayall commands (#13670)"
7e742d1 
This reverts commit 82a4f86.
@lukaszgo1 lukaszgo1 mentioned this pull request Jul 26, 2022
Closed
5 tasks
@seanbudd seanbudd mentioned this pull request Jul 27, 2022
Merged
5 tasks
seanbudd added a commit that referenced this pull request Jul 27, 2022
@seanbudd
Revert "Introducing table sayall commands (#13670)" (#13954)
9d25bac 
Reverts: #13670, #13901
Fixes #13927
Re-introduces: #13469

Summary of the issue:
The fix for #13927 is complex, and will need weeks of testing on alpha.
This means that #13469 is blocked until #13670 can be implemented with #13927 fixed.

Description of user facing changes
Removes table say all commands from 2022.3

Description of development approach
Revert PRs in the right order, fix up the release blurb

Testing strategy:
#13670 has been confirmed as the commit which broke #13927 using a git bisect.
The build from this code has been tested with bookworm.
seanbudd added a commit that referenced this pull request Jul 27, 2022
@seanbudd
Revert "Revert "Introducing table sayall commands (#13670)" (#13954)"
19c0f8f 
This reverts commit 9d25bac.
mltony added a commit to mltony/nvda that referenced this pull request Aug 25, 2022
@mltony
Revert "Revert "Introducing table sayall commands (nvaccess#13670)" (n…
a174193 
…vaccess#13954)"

This reverts commit 9d25bac.
mltony added a commit to mltony/nvda that referenced this pull request Aug 26, 2022
@mltony
Revert "Revert "Introducing table sayall commands (nvaccess#13670)" (n…
7529794 
…vaccess#13954)"

This reverts commit 9d25bac.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feerrenrut feerrenrut feerrenrut left review comments

@XLTechie XLTechie XLTechie left review comments

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

Assignees

@seanbudd seanbudd

Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Milestone
2022.3
Development

Successfully merging this pull request may close these issues.

Table sayAll commands