reduce braille.BrailleHandler.update redundant executions #16550
Conversation
|
I think your assumption is correct this time. |
|
@LeonarddeR, I will review the changes. Actions PerformedReview triggered.
|
WalkthroughThe Changes
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (19)
source/braille.py (19)
Line range hint
1427-1427: Remove unused importos.- import os
Line range hint
431-431: Avoid using bareexceptstatements. Specify the exception type to catch.- except: + except Exception:
Line range hint
1109-1109: Avoid using bareexceptstatements. Specify the exception type to catch.- except: + except Exception:
Line range hint
2397-2397: Avoid using bareexceptstatements. Specify the exception type to catch.- except: + except Exception:
Line range hint
2808-2808: Avoid using bareexceptstatements. Specify the exception type to catch.- except: + except Exception:
Line range hint
8-8: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
291-301: Thepgettextfunction used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import pgettext
Line range hint
558-558: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
894-894: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
1045-1045: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
1061-1061: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
1064-1064: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
1072-1072: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3158-3158: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3171-3171: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3181-3181: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3333-3333: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3371-3371: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
Line range hint
3378-3378: The_function used for translations is not defined in this file. Ensure that it is imported or defined.+ from gettext import gettext as _
There was a problem hiding this comment.
updateDisplayupdates the braille handler, soscrollTono longer does this.- The only place where
scrollTois called on the buffer from within the braille module itself, is inBrailleHandler.scrollToCursorOrSelection - All functions that call
scrollToCursorOrSelectionexplicitly update the braille handler after thescrollToCursorOrSelectioncall. - There are two cases where
globalCommandscallscrollTo, both inscript_braille_toFocus. That script explicitly callsupdateDisplayon the buffer as well.
|
|
@burmancomp - the testing strategy section doesn't describe a testing strategy, rather a summary of the issue. |
|
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. |
|
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. |
|
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. |
|
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. |
|
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 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:
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. |
|
@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. |
|
What are recommended use cases for "running from source"? |
|
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. |
|
@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 |
|
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. |
|
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? |
seanbudd
added
the
conceptApproved
Link to issue number:
closes #16456
Summary of the issue:
There are often two
BrailleHandler.updateexecutions 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
updateDisplaycall fromBrailleBuffer.scrollToTesting strategy:
daily use
Known issues with pull request:
no clear evidence
Code Review Checklist:
Summary by CodeRabbit