Skip to content

Review cursor: don't report "top"/"bottom" message when moving on the first/last line of the current object#9553

Merged
feerrenrut merged 4 commits into
nvaccess:masterfrom
AAClause:i9551
Jun 11, 2020
Merged

Review cursor: don't report "top"/"bottom" message when moving on the first/last line of the current object#9553
feerrenrut merged 4 commits into
nvaccess:masterfrom
AAClause:i9551

Conversation

@AAClause

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #9551

Summary of the issue:

Flash messages ("top" and "bottom") from review_top and review_bottom scripts are unnecessary.

Description of how this pull request fixes the issue:

Removes these flash messages.

Testing performed:

Tested with a try build.

Known issues with pull request:

None

Change log entry:

Section: Changes

@feerrenrut

Copy link
Copy Markdown
Contributor

Marking this as blocked until we get some more information from the community on the new UX of this command.

@feerrenrut

Copy link
Copy Markdown
Contributor

As per #9551 (comment) you can go ahead with this PR, please make the appropriate changes to provide symmetry with the start and end versions of these commands.

Also, please update the description of this PR to provide further detail. This is particularly necessary with the testing section. The testing section should provide detailed instructions on how to test this, and examples of the expected output. Consider different cases that might be encountered. The testing section is your opportunity to convince us that you have considered your testing strategy, performed the tests and that it works as expected. It is also useful to demonstrate to those who can not read the code what the results of the change are.

@LeonarddeR

This comment has been minimized.

@AAClause

Copy link
Copy Markdown
Contributor Author

@LeonarddeR with current changes, left/right are still announced when you move character by character. Same for top/bottom when you move line by line. Indeed, I think that it's useful in these cases.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Ah, wait. I just hadn't looked at the code and wrote my reply based on wrong assumptions. Just forget what I said.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Just a thought,

Instead of speaking messages in all these cases, how about playing the windows ding sound?

winsound.MessageBeep(winsound.MB_OK)

@lukaszgo1

Copy link
Copy Markdown
Contributor

@Andre9642 Have you considered converting modified scripts to the script decorator?

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Accepting these changes "as-is" while updating the scripts to use decorators would be nice, this is currently a fairly minor change and as it stands is low risk to go into 2020.2.

@LeonarddeR Introducing sounds might be worth considering, but that should be done independently to this PR.

@feerrenrut feerrenrut merged commit 670d16b into nvaccess:master Jun 11, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.2 milestone Jun 11, 2020
JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review cursor: don't report "top"/"bottom" message when moving on the first/last line of the current object

5 participants