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.

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

Remove redundant braille.BrailleHandler.update executions #16463

Merged
merged 2 commits into from
May 7, 2024

Conversation

burmancomp
Copy link
Contributor

Link to issue number:

fixes #16456

Summary of the issue:

In braille.BrailleHandler._handlePendingUpdate and braille.BrailleHandler._doNewObject functions scrollTocursororSelection function is called. scrollToCursorOrSelection calls scrollTo which in turns calls braille.BrailleHandler.update. Both _handlePendingUpdate and _doNewObject call braille.BrailleHandler.update after calling scrollToCursorOrSelection.

Description of user facing changes

There should be less "Braille window dots" log lines when log level is debug. Maybe some minor effect on performance.

Description of development approach

Minor modifications of _handlePendingUpdate and _doNewObject functions.

Testing strategy:

Local testing in daily use

Known issues with pull request:

none at this moment

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@burmancomp burmancomp marked this pull request as ready for review April 30, 2024 14:10
@burmancomp burmancomp requested a review from a team as a code owner April 30, 2024 14:10
@LeonarddeR
Copy link
Collaborator

This looks reasonable at first sight, though I"m a bit worried about unexpected side effects. This definitely needs a merge early in a release cycle, therefore I'll add the appropriate label for that.

@LeonarddeR LeonarddeR added the merge-early Merge Early in a developer cycle label Apr 30, 2024
@seanbudd seanbudd added this to the 2024.3 milestone May 1, 2024
user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@seanbudd seanbudd added queued for merge conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. labels May 6, 2024
@seanbudd seanbudd merged commit 29f4c5c into nvaccess:master May 7, 2024
1 check passed
@burmancomp burmancomp deleted the brailleRedundantUpdates branch May 7, 2024 06:10
@LeonarddeR
Copy link
Collaborator

This pr causes an issue where braille messages aren't properly dismissed when braille changes while viewing them.
For example, on github, try to move to the next heading until the "No next heading" message is shown, then change browse mode cursor. Observe that the "no next heading" message is still shown.

LeonarddeR added a commit to LeonarddeR/nvda that referenced this pull request May 7, 2024
seanbudd pushed a commit that referenced this pull request May 8, 2024
…illeHandler.update executions #16501

fixes #16463

Summary of the issue:
Fix for issue of #16463
where braille message was not dismissed.

Description of user facing changes
Braille message should be dismissed when moving cursor.

Description of development approach
if buffer is message buffer BrailleHandler._dismissMessage is executed before BrailleHandler.scrollToCursorOrSelection.
seanbudd pushed a commit that referenced this pull request May 10, 2024
Reverts #16463 and #16501

Issues fixed
Issues reopened
Reopens #16456

Reason for revert
complexity

Can this PR be reimplemented? If so, what is required for the next attempt
at least execution of scrollToCursorOrSelection in _doNewObject
XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request May 10, 2024
…6463)

fixes nvaccess#16456

Summary of the issue:
In braille.BrailleHandler._handlePendingUpdate and braille.BrailleHandler._doNewObject functions scrollTocursororSelection function is called. scrollToCursorOrSelection calls scrollTo which in turns calls braille.BrailleHandler.update. Both _handlePendingUpdate and _doNewObject call braille.BrailleHandler.update after calling scrollToCursorOrSelection.

Description of user facing changes
There should be less "Braille window dots" log lines when log level is debug. Maybe some minor effect on performance.

Description of development approach
Minor modifications of _handlePendingUpdate and _doNewObject functions.
XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request May 10, 2024
…illeHandler.update executions nvaccess#16501

fixes nvaccess#16463

Summary of the issue:
Fix for issue of nvaccess#16463
where braille message was not dismissed.

Description of user facing changes
Braille message should be dismissed when moving cursor.

Description of development approach
if buffer is message buffer BrailleHandler._dismissMessage is executed before BrailleHandler.scrollToCursorOrSelection.
XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request May 10, 2024
Reverts nvaccess#16463 and nvaccess#16501

Issues fixed
Issues reopened
Reopens nvaccess#16456

Reason for revert
complexity

Can this PR be reimplemented? If so, what is required for the next attempt
at least execution of scrollToCursorOrSelection in _doNewObject
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle queued for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant braille.BrailleHandler.update executions
3 participants