reduce braille.BrailleHandler.update redundant executions

[burmancomp]

Link to issue number:

closes #16456

Summary of the issue:

There are often two BrailleHandler.update executions which produce duplicate log entries when log level is debug. As an example, when navigating with up/down arrows in notepad debug level log shows this.

There should not be good reason for two executions.

Description of user facing changes

Less duplicate "Braille window dots" log lines on debug level. This should facilitate debugging a little.

Description of development approach

removed updateDisplay call from BrailleBuffer.scrollTo

Testing strategy:

daily use

Known issues with pull request:

no clear evidence

Code Review Checklist:

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

Summary by CodeRabbit

• Bug Fixes
  • Improved performance of Braille display scrolling by removing redundant display updates.

[LeonarddeR]

I think your assumption is correct this time. Can I trigger this? @coderabbitai review

[coderabbitai[bot]]

@LeonarddeR, I will review the changes.

Actions Performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai[bot]]

Walkthrough

The scrollTo method in the BrailleDisplay class has been modified to remove the self.updateDisplay() call. This change aims to eliminate redundant executions of the braille.BrailleHandler.update method, thus optimizing the performance and reducing unnecessary logging.

Changes

File	Change Summary
source/braille.py	Removed self.updateDisplay() from the scrollTo method in the BrailleDisplay class.

Assessment against linked issues

Objective (Issue #)	Addressed	Explanation
Eliminate redundant executions of braille.BrailleHandler.update (#16456)	✅
Reduce redundant log lines in debug mode (#16456)	✅

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[burmancomp]

I think your assumption is correct this time. Thanks @LeonarddeR

[seanbudd]

@burmancomp - the testing strategy section doesn't describe a testing strategy, rather a summary of the issue. Please describe how you tested this PR. I'd also encourage you to seek wider testing from other braille users using the build artifact.

[burmancomp]

@irrah68, @jykke67, @Emil-18 and @beqabeqa473 could you test if you have any braille related problem with this pr which are not present with other versions. Consider also issues which were discussed in #16501 and #16506.

Here is link to download this version:

https://ci.appveyor.com/api/buildjobs/3bjm7mgqed6q41bg/artifacts/output%2Fnvda_snapshot_pr16550-31927%2Cd1f186a7.exe

[Jykke67]

I’ve recently updated NVDA to this version, and no problems in basic usage have appeared so far.

Just to mention, in case this has any inpact, my braille display is Braille Star 80.

In order to get more evidence I probably have to test this in various situations and during a longer period of time.

[irrah68]

I tested this pr with my Braillex EL 80c display. Perhaps there is one thing that feels a little bit strange to me, but on the other hand, I don't know if this feature is meant to work this way.

In gestures.ini I have: braille_toFocus = br(papenmeier):upperrouting

When I set Braille messages to show indefinitely, press NVDA + t and read the message, then press a upper routing key, I hear the content of the line where the cursor is at the moment but the display keeps on showing the message which I got after pressing nvda +t. When or if I press a routing key of the first touch cursor line, the Display starts showing the current cursor location normally.

Otherwise I didn't notice strange behaviour and everything seemed to work normally.

[burmancomp]

As far as I understand behavior is as @irrah68 describes it. I tried it with current main branch code so this pr has not changed that.

I noticed something interesting when running from main branch source. In albatross gestures there is gestures to go to desktop, to open "this computer", to go to windows settings, and to go to notification area (I mean same as pressing windows+b). When "show messages" was "show indefinitely" those gestures worked but they did not dismissed braille message. But when pressed directly from keyboard they dismissed braille message. But this is separate issue.

[Jykke67]

Quite interesting. I have to admit that I have never been that good in replacing standard Windows gestures like Win+B with braille display specific ones even though I am a everyday user of a braille display.

I have tried to keep the standard Windows gestures in mind in case that no braille display is always available.

Of course I do use the tc buttons of the braille display in order to perform klicks, button pressings etc.

[burmancomp]

I was somewhat (am maybe less) concerned about freeze related log entries. With this pr there at least seemed to be more than with main branch code. But I do not know why. Only difference between this pr and main branch code is that line containing updateDisplay in scrollTo is removed.

I am testing by running from source. Then (some time ago) I realized that when switching between branches I have not run scons source. If I run it some building is done. So is this explanation for more log entries?

Can you @irrah68 and @Jykke67 do some testing by comparing build of this pr to some development snapshot with debug logging to see if this pr has more freeze related log entries.

I have used windows search box by activating it with windows key then typed "wordpad", and then pressed escape, and repeated same steps.

I noticed quite sure way to get freeze (from which nvda recovered). When nvda is started:

• open search box
• type "wordpad" and press enter
• close wordpad window
• goto search box and
• start type wordpad.

Can you also test if you can reproduce this? It should be case with both versions - with one cycle it likely did not occured with main branch code.

I have windows 10 and you likely have windows 11 so results may differ.

[seanbudd]

@burmancomp - I would encourage testing with AppVeyor builds just to more closely mimic release code. This isn't always necessary but if you're smoke testing a build over time it's a better approach. At the very least you should be creating launchers locally with scons launcher

[burmancomp]

What are recommended use cases for "running from source"?

[Jykke67]

I made a quick test as requested and no freezing occured, so there may be differences.

I have Win 11 updated from Win 10 by ignoring Microsoft’s hardware requirements, if that matters.

[seanbudd]

@burmancomp - there shouldn't be a difference for most situations that don't require an installed build, (e.g. portable copy is fine). I would say with targeted testing it should be fine. I would just encourage launcher builds when smoke testing, i.e. running a build over a long period of time to check for any regressions against an official build. Generally good smoke testing would involve situations that require an installed build, sometimes a signed build is needed too. If certain components have changed you may need to clean your build with scons -c or sometimes a full git clean is required.

[irrah68]

I tested this pr with --debug-logging and didn't notice any freezes. There were some error sounds and the log file contains error messages, but this has happened wit other NVDA test versions as well.

[Jykke67]

Yes. The error sound is typical for test versions of NVDA, but with logging level disabled one can get rid of it.

It took me a while to find this out, and before it I switched between test and stable versions, which was unnecessary.

BTW, the test version was updated to alpha-31971,c5954630, and audio ducking started to work again like in beta and rc versions before. Better to continue using alpha or return to 2024.2beta1 for stability purposes?