Skip to content

Remove redundant braille.BrailleHandler.update executions#16463

Merged
seanbudd merged 2 commits into
nvaccess:masterfrom
burmancomp:brailleRedundantUpdates
May 7, 2024
Merged

Remove redundant braille.BrailleHandler.update executions#16463
seanbudd merged 2 commits into
nvaccess:masterfrom
burmancomp:brailleRedundantUpdates

Conversation

@burmancomp

@burmancomp burmancomp commented Apr 30, 2024

Copy link
Copy Markdown
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

LeonarddeR commented Apr 30, 2024

Copy link
Copy Markdown
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
seanbudd
seanbudd reviewed May 6, 2024
View reviewed changes
Comment thread user_docs/en/changes.t2t Outdated
@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
@burmancomp burmancomp deleted the brailleRedundantUpdates branch May 7, 2024 06:10
@LeonarddeR

LeonarddeR commented May 7, 2024

Copy link
Copy Markdown
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
@LeonarddeR
Revert "Remove redundant braille.BrailleHandler.update executions (nv…
ac0e758 
…access#16463)"

This reverts commit 29f4c5c.
@LeonarddeR LeonarddeR mentioned this pull request May 7, 2024
Closed
@burmancomp burmancomp mentioned this pull request May 7, 2024
Merged
5 tasks
seanbudd pushed a commit that referenced this pull request May 8, 2024
@burmancomp
Fix for dismiss braille message issue of remove redundant braille.Bra…
c30ef08 
…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.
@burmancomp burmancomp mentioned this pull request May 9, 2024
Closed
5 tasks
@seanbudd seanbudd mentioned this pull request May 10, 2024
Closed
burmancomp added a commit to burmancomp/nvda that referenced this pull request May 10, 2024
@burmancomp
Revert "Remove redundant braille.BrailleHandler.update executions (nv…
bc27880 
…access#16463)"
@burmancomp burmancomp mentioned this pull request May 10, 2024
Merged
seanbudd pushed a commit that referenced this pull request May 10, 2024
@burmancomp
revert 16463 and 16501 (#16519)
2d86502 
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
@burmancomp burmancomp mentioned this pull request May 14, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran michaelDCurran is a code owner automatically assigned from nvaccess/developers

Assignees

No one assigned

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

Milestone

2024.3

Development

Successfully merging this pull request may close these issues.

Redundant braille.BrailleHandler.update executions

3 participants

@burmancomp @LeonarddeR @seanbudd