Allow the review cursor to be bounded to onscreen text by codeofdusk · Pull Request #9735 · nvaccess/nvda

codeofdusk · 2019-06-13T06:51:57Z

Link to issue number:

Builds on #9614. Supersedes #9687 and #9899. Closes #9891.

Summary of the issue:

Currently:

In the Windows Console, only onscreen text can be read using the review cursor; if text scrolls off the screen, it is impossible to review it.
In all other cases, all text in an object is presented to object review, and there is no way to know which text is visible on the screen.
In UIA consoles, jumping to the top or bottom lines of the review (shift+numpad 7/9) causes the cursor to become stuck.

Description of how this pull request fixes the issue:

This PR adds new functionality to textInfo objects: an isOffscreen property and POSITION_FIRSTVISIBLE and POSITION_LASTVISIBLE position constants. A new script (bound to NVDA+o by default) toggles review bounds. Review is bounded by default where supported. An implementation of review bounds has been provided for UIA (primarily intended for use in the console) and offsetsTextInfo.

Testing performed:

Attempted to review text before and after running git log in the UIA console using bounded and unbounded modes on Windows 10 1903. Verified that when review is bounded, reviewing the top/bottom lines (shift+numpad 7/9) works as intended.

Known issues with pull request:

In UIA consoles, we now diff the entire buffer rather than just the visible portion. This may slightly decrease performance in exchange for improved stability.

Change log entry:

== New Features ==

For certain controls (particularly the Windows Console on Windows 10 version 1803 and later), it is now possible to toggle whether the review cursor is bounded by the text currently visible onscreen by pressing NVDA+o. (Allow the review cursor to be bounded to onscreen text #9735)
This is especially useful in consoles to read text which has scrolled off the screen.

== Changes for Developers ==

Review bounds configuration can be enabled for textInfo objects by implementing the new isOffscreen property and POSITION_FIRSTVISIBLE and POSITION_LASTVISIBLE positions. Refer to the implementations in UIATextInfo and offsetsTextInfo. (Allow the review cursor to be bounded to onscreen text #9735)
Script category constants have been moved to a new scriptCategories module. These constants are also available in globalCommands and inputCore as before to avoid breaking existing code. (Allow the review cursor to be bounded to onscreen text #9735)

…nd the console by default to maintain previous behavior.

LeonarddeR · 2019-06-13T07:29:33Z

Are you aware of how first visible oand last visible offsets are implemented in textInfos.offsets? Given your initial description, I really like this new approach, but it makes sense to implement this for offsets textInfos as well. Note that you can utilize locationHelper to implement an abstract version of isOutOfBounds. It might help you to look at textINfos.offsets.OffsetsTextInfo._get_boundingRects in particular.

codeofdusk · 2019-06-13T09:29:11Z

@LeonarddeR I've added a basic implementation for offsetsTextInfo.

CC @feerrenrut @michaelDCurran

source/NVDAObjects/UIA/__init__.py

source/textInfos/__init__.py

source/NVDAObjects/__init__.py

LeonarddeR · 2019-06-13T09:44:59Z

source/NVDAObjects/__init__.py

+		category=_(u"Text review"),
+		# Translators: A gesture description.
+		description=_(u"Toggles whether review is constrained to the currently visible text"))
+	def script_toggleReviewBounds(self, gesture):


It worries me that this is is on the object instead of a global command. Is there a particular reason for that?

I too think this could be cleaner as a global command, but the reviewBounded variable is also on the object so keeping this there might be better from an organizational standpoint...

Question is whether we really believe that review Bounded should be on the object or on the textInfo, or even global. If an object gets destroyed and created again, the state of review Bounded is lost. Having review bounds on tree interceptors also makes sense, i.e. when you want to review with document review what is on screen on a particular page.

I also think that it doesn't make much sense to have review bounds doing anything when in screen review. So in that case, the command should just do nothing.

I think it may have been my idea to put this on the object. but @LeonarddeR brings up a good point in that the state will be lost if moving away from the object and back again. The problem with having it global though is that different controls work best when reviewBounded is set in a specific way by default. I.e. consoles probably want it on by default.
Would we be comfortable having bounded on by default globally? I think this could work, but it would be a bit of a change to NvDA's behaviour. Probably a good change though. Thoughts?

Yes, having this as a global default makes sense to me.

Yes, I also think a global default would be a plus.
It does not make much sense for a "screen" review to easily review what's not on screen.
It can be helpful at times, but is most often confusing when collaborating with a sighted person.

This has now been made a global default.

source/globalCommands.py

source/textInfos/offsets.py

source/NVDAObjects/__init__.py

Co-Authored-By: Leonard de Ruijter <[email protected]>

source/globalCommands.py

michaelDCurran · 2019-06-17T22:38:10Z

source/NVDAObjects/__init__.py

+		category=_(u"Text review"),
+		# Translators: A gesture description.
+		description=_(u"Toggles whether review is constrained to the currently visible text"))
+	def script_toggleReviewBounds(self, gesture):


I think it may have been my idea to put this on the object. but @LeonarddeR brings up a good point in that the state will be lost if moving away from the object and back again. The problem with having it global though is that different controls work best when reviewBounded is set in a specific way by default. I.e. consoles probably want it on by default.
Would we be comfortable having bounded on by default globally? I think this could work, but it would be a bit of a change to NvDA's behaviour. Probably a good change though. Thoughts?

michaelDCurran · 2019-06-17T22:39:40Z

source/NVDAObjects/__init__.py

+		description=_(u"Toggles whether review is constrained to the currently visible text"))
+	def script_toggleReviewBounds(self, gesture):
+		try:
+			rp = api.getReviewPosition()


This line should be moved above the try block. Only isOffscreen is expected to throw NotImplementedError.

michaelDCurran · 2019-06-17T22:50:00Z

source/NVDAObjects/__init__.py

+				)
+				if outOfBounds:
+					api.setReviewPosition(
+						self.makeTextInfo(textInfos.POSITION_CARET), isCaret=True)


Can you explain why isCaret=True is necessary here?

Because the textInfo is a caret instance? Maybe it isn't needed...

michaelDCurran · 2019-06-17T22:56:48Z

source/NVDAObjects/UIA/__init__.py

+			try:
+				visiRanges = self.obj.UIATextPattern.GetVisibleRanges()
+				element = 0 if position == textInfos.POSITION_FIRSTVISIBLE else visiRanges.length - 1
+				self._rangeObj = visiRanges.GetElement(0)


This call should use element, not 0 I think.

michaelDCurran · 2019-06-17T22:57:53Z

source/NVDAObjects/UIA/__init__.py

+				element = 0 if position == textInfos.POSITION_FIRSTVISIBLE else visiRanges.length - 1
+				self._rangeObj = visiRanges.GetElement(0)
+			except COMError:
+				# Error: FIRST_VISIBLE position not supported by the UIA text pattern.


The comment should reflect it is first or last position not supported, not just first.

michaelDCurran · 2019-06-17T22:59:42Z

source/NVDAObjects/UIA/__init__.py

+				UIAHandler.TextPatternRangeEndpoint_End) >= 0
+		else:
+			# Review bounds not available.
+			return super(UIATextInfo, self).isOutOfBounds()


I think this is supposed to be super's isOffscreen

michaelDCurran · 2019-06-17T23:00:44Z

source/NVDAObjects/UIA/__init__.py

 		return self._rangeObj.CompareEndpoints(src,other._rangeObj,target)

+	def isOffscreen(self):
+		visiRanges = self.obj.UIATextPattern.GetVisibleRanges()


Catch COMError around this call, raising NotImplementedError if so.

michaelDCurran · 2019-06-17T23:07:44Z

source/globalCommands.py

 	script_review_activate.__doc__=_("Performs the default action on the current navigator object (example: presses it if it is a button).")
 	script_review_activate.category=SCRCAT_OBJECTNAVIGATION

+	def _moveWithBoundsChecking(self, info, unit, direction, endPoint=None):


This could probably be moved into the base TextInfo class.
Also, is there a reason why this is non-distructive (I.e. returns a copy)? I don't think you actually ever make use of this. It could just do it distructively and return res like the other move... unless you have a particular plan for this not yet done?

This is a generalization of the console's old bounds checking code (i.e. we captured an oldRange and didn't update the underlying UIA text range if a move was impossible). Would destructive be okay here?

michaelDCurran · 2019-06-18T00:27:45Z

"@param isCaret: Whether the review position is changed due to caret following.". I don't think you need it. This should only be true if the setReviewPosition call is directly caused by the caret moving.

michaelDCurran · 2019-06-18T00:31:11Z

Unless I've missed a call when reviewing, It looks like _moveWithBoundsChecking only ever directly replaces a call to info.move, and always overrides info.

codeofdusk · 2019-06-18T00:34:38Z

Unless I've missed a call when reviewing, It looks like _moveWithBoundsChecking only ever directly replaces a call to info.move, and always overrides info.

Yes, we can do this, because the original info (rather than a new one) is returned if a move is impossible.

codeofdusk · 2019-06-20T01:57:12Z

@michaelDCurran @LeonarddeR This PR now uses unique IDs for NVDA objects to persist review bounds states when objects are regenerated.

I'd like to move the logic that sets reviewBounded to True for consoles to some sort of initialization method. However, when I try to override the __init__ method on the console, my version never gets called. Is there another way to achieve this?

I'd also like to move script_toggleReviewBounds from NVDAObject to TextContainerObject, but when I try the new script doesn't seem to be bound. I can provide the changes I'm trying to make as a commit or diff. Any ideas of things to look into?

michaelDCurran · 2019-06-21T01:07:02Z

NVDAObject overlay classes don't support the standard __init__ constructor. Rather, if you implement an "initOverlayClass" method, this will be called on the instance once the object has been constructed and all overlay classes have been chosen and inserted.

michaelDCurran · 2019-06-21T01:20:42Z

Currently TextContainerObject does not inherit from ScriptableObject and therefore scripts are not picked up if placed on this class.
However, it seems that currently all classes that inherit from TextContainerObject also inherit from ScriptableObject directly or indirectly.
Therefore, I suggest making TextContainerObject inherit from ScriptableObject instead of AutoPropertyObject. (Note that ScriptableObject inherits from autoPropertyObject itself).
In documentBase.py: change

class TextContainerObject(AutoPropertyObject):

To

class TextContainerObject(ScriptableObject):

codeofdusk · 2019-06-21T01:33:07Z

Currently TextContainerObject does not inherit from ScriptableObject and therefore scripts are not picked up if placed on this class.
However, it seems that currently all classes that inherit from TextContainerObject also inherit from ScriptableObject directly or indirectly.
Therefore, I suggest making TextContainerObject inherit from ScriptableObject instead of AutoPropertyObject. (Note that ScriptableObject inherits from autoPropertyObject itself).
In documentBase.py: change
class TextContainerObject(AutoPropertyObject):
To
class TextContainerObject(ScriptableObject):

@michaelDCurran Working now, thanks!

feerrenrut · 2019-07-16T11:50:22Z

source/textInfos/__init__.py


+	def _get_isOffscreen(self):
+		"""
+		Returns True if this textInfo is positioned outside its object's


If the text is partially visible, what is returned?

Depends on the underlying implementation.

This reverts commit 4cc443f.

This reverts commit 291b00e.

This reverts commit 2c24358.

…ported.

… stuck out of bounds.

…anges. This is likely a little slower, but is more stable.

…ls and disable the console bounds checking code when oldRange is also offscreen.

codeofdusk · 2019-07-22T06:51:40Z

Superseded for now by #9957, but I'm willing to look into this again if there's a use case.

…e checks (PR #9957) Builds on #9614 Supersedes #9735 and #9899 Closes #9891 Previously, after the console window was maximized (or the review cursor is otherwise placed outside the visible text), text review is no longer functional. The review top line and review bottom line scripts do not function as intended. This commit changes: - The isOffscreen property has been implemented as UIAUtils.isTextRangeOffscreen. - When checking if the text range is out of bounds, we now also check that oldRange is in bounds before stopping movement. - Re-implemented POSITION_FIRST and POSITION_LAST in terms of visible ranges.

…ls and disable the console bounds checking code when oldRange is also offscreen.

This reverts commit 9fbd6b4.

codeofdusk added 6 commits June 12, 2019 20:04

Remove bounds checking code from the console.

7a123c3

Implement isOutOfBounds.

a61dab0

Implement review bounds configuration.

a8b93bd

Fix isOutOfBounds for UIA, implement initial bounds checking, and bou…

6bb21b2

…nd the console by default to maintain previous behavior.

Implement review of top and bottom lines when review is bounded.

deffcea

Add new gesture to the input gestures dialog, update user guide.

51fa56a

Implement review bounds configuration for offsetsTextInfo.

a1ad305

LeonarddeR reviewed Jun 13, 2019

View reviewed changes

codeofdusk and others added 4 commits June 13, 2019 06:38

Apply suggestions from code review

d7a00e8

Co-Authored-By: Leonard de Ruijter <[email protected]>

Review actions.

1a937d2

isOutOfBounds -> isOffscreen

536ccce

Move SCRCAT constants to a new scriptCategories module.

d84570a

michaelDCurran requested changes Jun 17, 2019

View reviewed changes

Review actions.

4540972

Merge branch 'master' into reviewbounds

b7126db

Merge branch 'master' into reviewbounds

0f41ef3

codeofdusk added 3 commits June 17, 2019 20:36

Clarify doc comment.

768cf6b

Add unique IDs for NVDA objects.

4cc443f

Persist review bounds state when an object is regenerated.

291b00e

Make isOffscreen a property.

c94229b

Fixes.

b2924fa

codeofdusk added 4 commits June 20, 2019 21:39

Better default review bounds configuration for objects.

2c24358

Remove unneeded import.

959a6c7

Merge branch 'master' into reviewbounds

360ce53

Merge branch 'master' into reviewbounds

7146fae

This was referenced Jul 7, 2019

UI Automation in Windows Console: text review behaves inconsistently when the window is maximized #9891

Closed

UI Automation in Windows Console: disable some GetVisibleRanges dependent logic when consoles are maximized #9899

Closed

feerrenrut reviewed Jul 16, 2019

View reviewed changes

codeofdusk added 8 commits July 17, 2019 01:54

Merge branch 'master' into reviewbounds

2f85736

Revert "Add unique IDs for NVDA objects."

87d67e7

This reverts commit 4cc443f.

Revert "Persist review bounds state when an object is regenerated."

d41476b

This reverts commit 291b00e.

Revert "Better default review bounds configuration for objects."

8c50383

This reverts commit 2c24358.

Make review bounds state global and bound review by default where sup…

cf11cdd

…ported.

Disable bounds checking if we're currently offscreen to avoid getting…

11f0d59

… stuck out of bounds.

In UIA consoles, diff the entire buffer instead of just the visible r…

bd33c92

…anges. This is likely a little slower, but is more stable.

Fix top/bottom review scripts.

f3ff4a2

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jul 22, 2019

Backport _getTextLines from nvaccess#9735.

9fbd6b4

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jul 22, 2019

Move the isOffscreen logic from nvaccess#9735 to a function in UIAUti…

90fe976

…ls and disable the console bounds checking code when oldRange is also offscreen.

codeofdusk mentioned this pull request Jul 22, 2019

UI Automation in Windows Console: improve reliability of visible range checks #9957

Merged

codeofdusk closed this Jul 22, 2019

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Sep 25, 2019

Move the isOffscreen logic from nvaccess#9735 to a function in UIAUti…

8964260

…ls and disable the console bounds checking code when oldRange is also offscreen.

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Sep 25, 2019

Backport _getTextLines from nvaccess#9735.

cdf4479

codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Nov 20, 2019

Revert "Backport _getTextLines from nvaccess#9735."

26a5e57

This reverts commit 9fbd6b4.

michaelDCurran pushed a commit that referenced this pull request Nov 20, 2019

Revert "Backport _getTextLines from #9735." (#10513)

833bc4e

This reverts commit 9fbd6b4.

codeofdusk mentioned this pull request May 17, 2020

Efficiently reviewing the output buffer in terminals #11172

Closed

codeofdusk mentioned this pull request Dec 19, 2021

windows terminal: NVDA reads invisible lines when using pagers such as less and more #13157

Open

Uh oh!

Conversation

codeofdusk commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR commented Jun 13, 2019

Uh oh!

codeofdusk commented Jun 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Jun 18, 2019 via email

Uh oh!

michaelDCurran commented Jun 18, 2019 via email

Uh oh!

codeofdusk commented Jun 18, 2019

Uh oh!

codeofdusk commented Jun 20, 2019

Uh oh!

michaelDCurran commented Jun 21, 2019

Uh oh!

michaelDCurran commented Jun 21, 2019

Uh oh!

codeofdusk commented Jun 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeofdusk commented Jul 22, 2019

Uh oh!

Reviewers

codeofdusk commented Jun 13, 2019 •

edited

Loading