-
-
Notifications
You must be signed in to change notification settings - Fork 743
UI Automation in Windows Console: limit blank lines in review and initial word movement support #9647
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
UI Automation in Windows Console: limit blank lines in review and initial word movement support #9647
Changes from 7 commits
41d4816
53ecca9
a9575ba
1cf0563
6083ded
768fe0f
9a8835c
bc55777
6f52524
c2386b9
c016ab1
6557f04
4e9bbe0
b12cbc9
e4e96ef
d00a15b
1df93fa
9fef531
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |||||||||||
| # See the file COPYING for more details. | ||||||||||||
| # Copyright (C) 2019 Bill Dengler | ||||||||||||
|
|
||||||||||||
| import ctypes | ||||||||||||
| import NVDAHelper | ||||||||||||
| import time | ||||||||||||
| import textInfos | ||||||||||||
| import UIAHandler | ||||||||||||
|
|
@@ -16,10 +18,13 @@ | |||||||||||
|
|
||||||||||||
| class consoleUIATextInfo(UIATextInfo): | ||||||||||||
| _expandCollapseBeforeReview = False | ||||||||||||
| _isCaret = False | ||||||||||||
| _expandedToWord = False | ||||||||||||
|
|
||||||||||||
| def __init__(self, obj, position, _rangeObj=None): | ||||||||||||
| super(consoleUIATextInfo, self).__init__(obj, position, _rangeObj) | ||||||||||||
| if position == textInfos.POSITION_CARET: | ||||||||||||
| self._isCaret = True | ||||||||||||
| if isAtLeastWin10(1903): | ||||||||||||
| # The UIA implementation in 1903 causes the caret to be | ||||||||||||
| # off-by-one, so move it one position to the right | ||||||||||||
|
|
@@ -30,6 +35,115 @@ def __init__(self, obj, position, _rangeObj=None): | |||||||||||
| 1 | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| def move(self, unit, direction, endPoint=None): | ||||||||||||
| if not self._isCaret: | ||||||||||||
|
||||||||||||
| if not self._isCaret: | |
| if self.basePosition != textInfos.POSITION_CARET: |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be possible that visiRanges has a length of 0. It would probably be a bug, but best to check anyway. Only do the next lot of code if length>0.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about also checking that we are not before the first visible range? I.e. we should not allow someone to move off the top of the screen up into previous content.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer to do this check after the call to super, and then restore the textRange if we have moved too far. If my logic is correct, I'm pretty sure that this current code will still allow moving to the first line after the last visible range? Because when on the last line, compareEndPoints textInfo's start with lastVisiRange's end would not yet be >=0.
So before super, I'd save off a copy of _rangeObj with something like:
oldRange=self._rangeObj.clone()
Then after super, do these checks, and if we have moved out of range, then replace self._rangeObj with oldRange, and return 0.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see this method better named to reflect that this is talking about within the current line. Something like _getCurrentOffsetInCurrentLine.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again: perhaps call it _getWordOffsetsInCurrentLine
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what this does exactly and why it is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand should use similar code to move. E.g.:
If unit is word, then use _getCurrentOffset and _getWordOffsets and manually move the start and end of the range to have it correctly bound the current word.
Then, there is no need to change what text is exposed as the textRange will be correct.
The advantage of this is there is less to track, it is easier to read, and most importantly, later comparisons or movements of this textInfo won't behave like it is expanded to line even though the text returned is word.
There may be a question around performance: but I'd like to see the code changed to my proposed way before we then consider if optimisations are needed at all.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent experiences with Notepad++ made me a bit worried about endless while loops. Could you limit this somehow?
Would something like this work?
| while True: | |
| while charInfo.compareEndPoints( | |
| lineInfo, | |
| "startToEnd" | |
| ) <= 0: |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think _getCurrentWord can be removed now as it is no longer used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this is just fixing up code formatting, I don't think this is directly related to this pr, so I'd prefer to see it in one of the other ones that deals directly with speakTypedWords etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary, see below.