Another fix for dismiss braille message issue of remove redundant braille.BrailleHandler.update executions#16506
Conversation
|
This is why I was for a revert initially. Now, it is no longer possible to see from the diff were we came from. |
LeonarddeR
left a comment
There was a problem hiding this comment.
I think this looks good, but if a third fixup would be necessary, I'd rather see a revert of the former prs and have a broader test with try builds first.
|
I agree with @LeonarddeR. This is becoming a bit absurd.
|
|
I try to revert if new issues are found. |
|
Here is a diff against the original code: diff --git a/source/braille.py b/source/braille.py
index 450594c8b..3e5c1595d 100644
--- a/source/braille.py
+++ b/source/braille.py
@@ -2560,9 +2560,11 @@ class BrailleHandler(baseObject.AutoPropertyObject):
self.mainBuffer.update()
# Last region should receive focus.
self.mainBuffer.focus(region)
- self.scrollToCursorOrSelection(region)
if self.buffer is self.mainBuffer:
- self.update()
+ if isinstance(region, TextInfoRegion):
+ self.scrollToCursorOrSelection(region)
+ else:
+ self.update()
elif self.buffer is self.messageBuffer and keyboardHandler.keyCounter>self._keyCountForLastMessage:
self._dismissMessage()
@@ -2614,10 +2616,11 @@ class BrailleHandler(baseObject.AutoPropertyObject):
region.pendingCaretUpdate = False
self.mainBuffer.update()
self.mainBuffer.restoreWindow()
- if scrollTo is not None:
- self.scrollToCursorOrSelection(scrollTo)
if self.buffer is self.mainBuffer:
- self.update()
+ if scrollTo is not None:
+ self.scrollToCursorOrSelection(scrollTo)
+ else:
+ self.update()
elif (
self.buffer is self.messageBuffer
and keyboardHandler.keyCounter > self._keyCountForLastMessage
|
| elif self.buffer is self.mainBuffer: | ||
| self.update() | ||
| if self.buffer is self.mainBuffer: | ||
| if isinstance(region, TextInfoRegion): |
There was a problem hiding this comment.
Why do we only call scrollToCursorOrSelection for TextInfoRegion?
There was a problem hiding this comment.
I am not aware about other regions with cursor or selection. I suppose that scrollToCursorOrSelection does nothing if there is no cursor nor selection.
If there is cursor or selection scrollTo executes BrailleHandler.update.
Regions without cursor or selection needs that BrailleHandler.update is executed but if there is cursor or selection scrollTo executes it.
There was a problem hiding this comment.
scrollToCursorOrSelection has an explicit check where region is not a TextInfoRegion. I guess that code is there for a reason.
There was a problem hiding this comment.
Thank you for mentioning this. I am sorry. I ignored it because I did not understand it, and things seemed to work. I should have considered it more.
It seems that not isinstance(region, TextInfoRegion) might be (is) there for situation that sometimes some other type of regions which are not class of TextInfoRegion or its descendants would have selection feature.
Simple solution which does not have issues of #16499 or #16501 would be to go back to code before #16463 except removing line
self.updateDisplay()
from BrailleBuffer.scrollTo function.
Do you have considerations against this?
|
I think I'd like to go back to before #16463 and just leave this code alone. |
|
@dkager could you kindly explain me this code line in
When region is not |
|
Could you please open a PR to revert #16463 and #16501? |
Link to issue number:
fixes #16501
Summary of the issue:
Braille message was not dismissed when moving to new object where region was
TextInfoRegion.I noticed that when messages were shown infinitely. I typed in start menu search box "wordpad" and pressed enter. Although Wordpad edit window was activated, braille was not updated.
Also, what triggered message dismiss, changed although this was not intention.
Description of user facing changes
Hopefully above issues are fixed.
Description of development approach
Testing strategy:
Tested locally
Known issues with pull request:
none at this moment
Code Review Checklist: