Options for focus context presentation in braille by LeonarddeR · Pull Request #7236 · nvaccess/nvda

LeonarddeR · 2017-05-31T13:35:31Z

Link to issue number:

Fixes #217

Summary of the issue:

When you arrow through a list of items and are reading the braille display, the start of each list item may begin in a different place for one list item than it is for another. This makes it more difficult to easily arrow down through a list in search of something, because you have to stop and read the display to be able to find the list item.

Description of how this pull request fixes the issue:

This implements two types of focus context/ancestry presentation brought up by @jcsteh:

To have braille insure that lists and such consistently present the start of items in the same place on the display #217 (comment): an option that allows you to always show the object's content at the very left of the braille display. This means that you will always have to scroll back if you want to see the ancestry, but this is no problem for speech oriented braille users and might even be their preference.
To have braille insure that lists and such consistently present the start of items in the same place on the display #217 (comment) : An option that only presents new focus ancestry information to the user when the ancestry has changed.

Testing performed:

Go to braille settings
Set "Focus context presentation" to "Only when scrolling back", and press ok
Arrow around the desktop. Desktop items will show up at the left of the braille display
Set "Focus context presentation" to "Fill display for context changes", and press ok
When the desktop list is focused, the ancestry will be shown as earlier. When you start arrowing around, the selected desktop icon will be shown hard left, as there is no context change
Set "Focus context presentation" back to "Always fill display", and press ok
Arrowing through the desktop reveals default behaviour on the braille display.

Known issues with pull request:

I'd like feedback from people about the wording of the new focus context presentation options and its options. It is quite hard to give a short description of the current, default behaviour actually. Also, I'm not that good in writing good summaries for the changes file.
This doesn't have a global command yet

Open question

I'm using an integer in the config spec, is this preferred or should we go for a string instead?

…rd left * Added focus presentation to the gui * Updated copyright headers

dkager · 2017-06-05T14:51:31Z

You'll also want to update the user guide once the final wording has been decided.

…k, not yet ready for review

…de, also did some rewording in the gui

…he spec accordingly

LeonarddeR · 2017-06-21T08:33:39Z

@jcsteh: Not trying to push, but just wanted to let you know that this is ready for review.

One thing I'd like to point your attention at, is the use of a focusToHardLeftSet variable. I'm not particularly happy with this approach, but I can't come up with alternatives here.

dkager · 2017-06-21T19:14:43Z

 		if region.focusToHardLeft:
 			# Only scroll to the start of this region.
 			restrictPos = endPos - regionPos - 1
+		elif config.conf["braille"]["focusContextPresentation"]==2:


While NVDA certainly isn't too strict about this, I think having constants for this option would improve readability a lot.

If we'd go for constants, I'd say we should go for strings, or for
option to be super modern. Items like tethering in the config spec
should be option type as well, actually. Unless you can't set a default
for those.

LeonarddeR · 2017-06-22T04:06:20Z

@@ -1211,6 +1213,20 @@ def _set_windowEndPos(self, endPos):
 		if region.focusToHardLeft:
 			# Only scroll to the start of this region.


This function has some double looping due to my recent changes, so I'm planning to change the order a bit later today. I want it to loop through the regions only once.

…ions only once

…onfig spec

LeonarddeR · 2017-06-30T11:10:03Z

I just added unit tests for this.

jcsteh

This looks pretty good for the most part. Very nice work. Thanks.

jcsteh · 2017-07-11T10:49:57Z

 		self.hidePreviousRegions = (start.compareEndPoints(readingInfo, "startToStart") < 0)
-		# If this is a multiline control, position it at the absolute left of the display when focused.
-		self.focusToHardLeft = self._isMultiline()
+		# Don't touch focusToHardLeft if it is already true


I think some explanation/example of how this might already be True would be helpful; e.g. that this can be set by getFocusContextRegions or BrailleHandler._doNewObject.

jcsteh · 2017-07-11T10:58:55Z

-		if region.focusToHardLeft:
-			# Only scroll to the start of this region.
-			restrictPos = endPos - regionPos - 1
+		# Loop through the currently displayed regions in reverse order


Now that this code is getting more complex, I think a comment before this line would be useful explaining what this block of code does; e.g. "If there's a region that should be placed at the left of the display, make sure we only scroll back that far."

jcsteh · 2017-07-11T11:10:44Z

+					restrictPos = regionStart
+					break
+				elif config.conf["braille"]["focusContextPresentation"]!='hybrid':
+					# Do not process the other regions


Why? It'd be good to clarify the comment. I think this is because if we're not doing context change presentation, we only need to consider the last region. We could perhaps say "If this isn't context change presentation, we only need to consider the last region."

jcsteh · 2017-07-11T11:14:56Z

 			commonRegionsEnd = 0
 			newAncestorsStart = 1
-		# Yield the common regions.
+		# Yield the common regions, thereby setting focusToHardLeft back to False.


I don't quite follow this comment. I think you mean "Yield the common regions and also set their focusToHardLeft to False" or similar. Still, I can't easily glean an understanding of why this is done. I assume this is because it was set based on a previous focus change, but it'd be good to clarify this.

Ah, this was part of my initial implementation of this and I wanted to make sure that all cached regions with focusToHardLeft set to True had it set back to False. But, this isn't necessary.

Hmm, no, the current implementation still relies on setting focusTohardLeft to False. Let me investigate this in more detail and come up with a descriptive comment. Cheers for unit testing.

jcsteh · 2017-07-11T11:41:24Z

 		region = NVDAObjectRegion(parent, appendText=TEXT_SEPARATOR)
 		region._focusAncestorIndex = index
+		if config.conf["braille"]["focusContextPresentation"]=='hybrid' and not focusToHardLeftSet:
+			region.focusToHardLeft = True


Would this be more readable if it just did index == newAncestorsStart? That would avoid the need for the focusToHardLeftSet flag and also make it clear that we do this only for the first new ancestor.

A comment might be nice stating why we do this; i.e. we're doing context change presentation and this is the first new ancestor, so only scroll back this far.

Would this be more readable if it just did index == newAncestorsStart ? That would avoid the need for the focusToHardLeftSet flag and also make it clear that we do this only for the first new ancestor.

This is not possible due to the "if not parent.isPresentableFocusAncestor:" line.

jcsteh · 2017-07-11T12:15:10Z

+		self.assertEqual(len(self.regionsWithPositions),3)
+
+	def test_fillDisplay(self):
+		"""Test for the case where both the focus object and its ancestors should be visible on a 40 cell display."""


Perhaps "all its ancestors" to be super clear?

jcsteh · 2017-07-11T12:18:32Z

+		# This is because getting windowEndPos can update windowStartPos
+		# Both the focus object and its parent should fit on the display
+		# Thus, the window end position is equal to the end position of the 3rd region
+		self.assertEqual(braille.handler.buffer.windowEndPos,self.regionsWithPositions[2][2])


If it's not too much trouble, consider making the regions with positions code in braille return a namedtuple. That would make these tests more readable; e.g. self.regionsWithPositions[2].end.

jcsteh · 2017-07-11T12:23:03Z

+		# Only the focus object should be visible now, equivalent to scroll only
+		self.assertEqual(braille.handler.buffer.windowEndPos,self.regionsWithPositions[2][2])
+		self.assertEqual(braille.handler.buffer.windowStartPos,self.regionsWithPositions[2][1])
+		# Clean up the cached focus ancestors


You're only cleaning up ancestors to a certain point, so I think this comment needs some clarification. You could use the roles to help explain.

jcsteh · 2017-07-11T12:28:38Z

+		# Forcefully create a fake focus ancestry
+		globalVars.focusAncestors=[api.getDesktopObject(),NVDAObjectWithRole(role=controlTypes.ROLE_DIALOG),NVDAObjectWithRole(role=controlTypes.ROLE_LIST)]
+		braille.handler.handleGainFocus(self.obj)
+		# Make sure that we are testing with three regions


It'd be super helpful if this comment could be expanded to list the roles (dialog, list, list item) and note that braille excludes the desktop object. I forgot about this last point myself and was thus very confused.

jcsteh · 2017-07-11T12:28:59Z

+		braille.handler.handleGainFocus(self.obj)
+		# WindowEndPos should be retrieved before we attempt to get the start position
+		# This is because getting windowEndPos can update windowStartPos
+		# Both the focus object and its parent should fit on the display


"its parent" -> "its ancestors"

jcsteh · 2017-07-11T12:31:54Z

Oh, and this now has merge conflicts. :(

LeonarddeR · 2017-07-11T16:37:42Z

note that this not yet ready for another round of review, there are some open comments. As for the review regarding the unit tests, I decided to create a new module called objectProvider which we can use to add more object related stuff to if we so desire later.

… presentation option. Still a bit rough though.

jcsteh

Very nice comments and documentation. Thank you. Almost there. :)

jcsteh · 2017-07-11T23:53:19Z

 	def _set_windowEndPos(self, endPos):
+		"""Sets the end position for the braille window and recalculates the window start position based on several variables.
+		1. Braille display size.
+		2. Whether one of the regions should be shown hard left on the braille display.


nit: Semi-colon instead of full stop at end of this line.

jcsteh · 2017-07-11T23:54:04Z

+		"""Sets the end position for the braille window and recalculates the window start position based on several variables.
+		1. Braille display size.
+		2. Whether one of the regions should be shown hard left on the braille display.
+			(i.e. because of The configuration setting for focus context representation 


nit: No parenthesis here, since we just continue on from semi-colon on previous line. (Also, there was no close parenthesis originally.)

jcsteh · 2017-07-11T23:54:44Z

-			# Only scroll to the start of this region.
-			restrictPos = endPos - regionPos - 1
+		# Loop through the currently displayed regions in reverse order
+		# If focusTohardLeft is set for one of the regions, the display shouldn't scroll further back than the start of that region


nit: focusTohardLeft has a lower case h; should be upper.

jcsteh · 2017-07-11T23:59:12Z

 	tetherTo = string(default="focus")
 	readByParagraph = boolean(default=false)
 	wordWrap = boolean(default=true)
+	focusContextPresentation = option("changedContext","fill","scroll",default="changedContext")


nit: Spaces after commas here please.

jcsteh · 2017-07-12T00:01:38Z

 api.setFocusObject(phObj)
 api.setNavigatorObject(phObj)
-
+api.setDesktopObject(phObj)


nit: Reinstate blank line after this.

jcsteh · 2017-07-12T00:14:15Z

+When set to fill display for context changes, NVDA will try to display as much as possible context information on the braille display, but only for the parts of the context that have changed.
+For the example above, this means that when changing focus to the list, NVDA will show the list item on the braille display.
+Furthermore, if there is enough space left on the braille display, NVDA will try to show that the list item is part of a list.
+Now, if you start moving through the list with your arrow keys, it is assumed that you are aware of the fact that you are moving through a list.


Perhaps this:

If you then start moving through the list with your arrow keys, it is assumed that you are aware that you are still in the list.

jcsteh · 2017-07-12T00:16:05Z

+Thus, for the remaining list items you focus, NVDA will only show the focused list item on the display.
+In order for you to read the context again (i.e. that you are in a list and that the list is part of a dialog), you will have to scroll your braille display back.
+
+When this option is set to always fill the display, NVDA will try to show as much as possible context information on the braille display, regardless of whether you have seen the same context information before.


"as much as possible context information" -> "as much context information as possible"

jcsteh · 2017-07-12T00:20:28Z

+In order for you to read the context again (i.e. that you are in a list and that the list is part of a dialog), you will have to scroll your braille display back.
+
+When this option is set to always fill the display, NVDA will try to show as much as possible context information on the braille display, regardless of whether you have seen the same context information before.
+This has the advantage that NVDA will make use of the braille display in an effective way, filling up as much as possible empty display space.


I know what you mean here, but some users (you and Davy, for example, heh) might argue this isn't "effective". Perhaps this:

This has the advantage that NVDA will fit as much information as possible on the display.

jcsteh · 2017-07-12T00:23:09Z

+
+When this option is set to always fill the display, NVDA will try to show as much as possible context information on the braille display, regardless of whether you have seen the same context information before.
+This has the advantage that NVDA will make use of the braille display in an effective way, filling up as much as possible empty display space.
+however, this option has the downside that there is always a difference in the position where the representation of the object with focus starts on the braille display.


Need capital at start of sentence.

I think we can simplify/shorten this:

However, the downside is that there is always a difference in the position where the focus starts on the braille display.

Would it also be useful to mention that this makes skimming difficult? For example:

This can make it difficult to skim a long list of items, for example, as you will need to continually move your finger to find the start of the item.

jcsteh · 2017-07-12T00:24:13Z

+This was the default behavior for NVDA 2017.2 and before.
+
+When you set the focus context presentation option to only show the context information when scrolling back, NVDA never shows context information on your braille display by default.
+Thus, in the example above, it is presented on the braille display that you focused a list item. However, in order for you to read the context (i.e. that you are in a list and that the list is part of a dialog), you will have to scroll your braille display back.


"it is presented on the braille display that" -> "NVDA will display that"

Two sentences on one line. Please split.

LeonarddeR · 2017-07-12T07:02:57Z

@leonardder commented on 11 jul. 2017 18:37 CEST:

note that this not yet ready for another round of review

@jcsteh: It seems you decided otherwise, many thanks. I've addressed your comments.

I also added a script to globalCommands, which I left unbound by default. I'd argue that users don't change this every single day. If you think this should be bound by default, I propose control+NVDA+F for focus.

jcsteh · 2017-07-14T01:23:27Z

+			index = values.index(config.conf["braille"]["focusContextPresentation"])
+		except:
+			index=0
+		newIndex = index+1 if index+1<len(values) else 0


FYI, this can be shortened to:

newIndex = index % len(values)

I'll leave it up to you as to whether you want to change this. Whether it makes things more readable or not is certainly debatable. :)

jcsteh · 2017-07-14T01:27:53Z

+		config.conf["braille"]["focusContextPresentation"] = values[newIndex]
+		braille.invalidateCachedFocusAncestors(0)
+		braille.handler.handleGainFocus(api.getFocusObject())
+		# Translators: Reports the new state of braille focus context presentation.


It might be helpful to note that %s gets replaced with the context presentation setting and give an example:

# %s will be replaced with the context presentation setting. # For example, the full message might be "Braille focus context presentation Fill display for context changes"

Actually, given the length of that example, I wonder whether we need a colon here. Normally, we prefer to avoid this for brevity (linguistic correctness is less important for these kinds of things), but this is long enough that it might be confusing without it. Alternatively, we could just cut the prefix altogether and just use the setting name. Arguably, the setting names are long/unique enough that the context will be fairly clear.

jcsteh · 2017-07-14T01:30:08Z


+==== Focus context presentation ====
+This option allows you to choose what context information NVDA will show on the braille display when an object gets focus.
+Context information (or focus ancestry) refers to the hierarchy of objects containing the focus.


I still wonder whether we should drop the term "focus ancestry" here. I'm concerned it will just confuse most users and it's certainly more technical than "context information".

jcsteh · 2017-07-14T01:30:57Z

+This list might be contained by a dialog, etc.
+Please consult the section about [object navigation #ObjectNavigation] for more information about the hierarchy that applies to objects in NVDA.
+
+When set to fill display for context changes, NVDA will try to display as much as possible context information on the braille display, but only for the parts of the context that have changed.


"as much as possible context information" -> "as much context information as possible"

LeonarddeR · 2017-07-14T05:25:03Z

1. I decided to go the "Braille focus context presentation: always fill display" way for the global command. I agree that the message is getting a bit long, but: * This is left unbound by default anyway * I find "Only when scrolling back" a bit too little verbose when changing this option that way. * But, if you have alternative ideas or still think that we should remove the prefix, let me know. 2. That (newIndex+1) % len(values) is actually very cool, this really shows my improper computer science background.

…ssue #217).

Leonard de Ruijter added 2 commits May 31, 2017 15:16

Added a focuspresentation option to the configSpec

cf47e2b

* When focusPresentation is set to 1, always show the focus object ha…

b8539ad

…rd left * Added focus presentation to the gui * Updated copyright headers

Leonard de Ruijter added 4 commits June 19, 2017 19:06

First work on hybrid mode for braille focus presentation

d9182da

First attempt on getting the hybrid braille focus presentation to wor…

6ec1fea

…k, not yet ready for review

Finished under de hood implementation of focus presentation hybrid mo…

7d9a16b

…de, also did some rewording in the gui

Reworded focus presentation setting in braille settings and changed t…

3fc14f7

…he spec accordingly

LeonarddeR changed the title ~~Option to always show objects hard left on a braille display~~ Options to show objects hard left on a braille display Jun 21, 2017

Reverted an unintentional line removal and fixed a translator comment

c6cd906

dkager reviewed Jun 21, 2017

View reviewed changes

LeonarddeR commented Jun 22, 2017

View reviewed changes

For _set_windowEndPos, loop through the current currently visible reg…

c480943

…ions only once

LeonarddeR mentioned this pull request Jun 22, 2017

ConfigObj: use option flag for certain items #6090

Closed

Leonard de Ruijter added 2 commits June 30, 2017 10:43

Changed focus context presentation to an option based option in the c…

5bce298

…onfig spec

Provided a unit test for #217

304d9e5

dkager mentioned this pull request Jul 5, 2017

Create an option to reduce braille output while tethered to focus #1884

Closed

jcsteh requested changes Jul 11, 2017

View reviewed changes

Leonard de Ruijter added 2 commits July 11, 2017 14:53

Merge branch 'master' into i217, resolving merge conflict

a2ac45f

Review actions part 1

83b698e

Leonard de Ruijter added 2 commits July 11, 2017 18:57

Reoder focus context presentation options to show the new default first

0256c42

First attempt to come up with documentation for the new focus context…

263143b

… presentation option. Still a bit rough though.

jcsteh requested changes Jul 12, 2017

View reviewed changes

Leonard de Ruijter added 2 commits July 12, 2017 08:23

Review actions part 2

c4505d3

Add a script to globalCommands

d7dafdb

Fix accidental override of miscDeps

ab52ce7

jcsteh requested changes Jul 14, 2017

View reviewed changes

Review actions part 3

7aa3631

jcsteh changed the title ~~Options to show objects hard left on a braille display~~ Options for focus context presentation in braille Jul 19, 2017

jcsteh approved these changes Jul 19, 2017

View reviewed changes

jcsteh added a commit that referenced this pull request Jul 19, 2017

Options for focus context presentation in braille. Incubates #7236 (i…

6c68b5f

…ssue #217).

nvaccessAuto added the incubating label Jul 19, 2017

nvaccessAuto assigned jcsteh Jul 19, 2017

jcsteh merged commit 1785d5f into nvaccess:master Aug 1, 2017

nvaccessAuto removed the incubating label Aug 1, 2017

nvaccessAuto added this to the 2017.3 milestone Aug 1, 2017

dkager mentioned this pull request Aug 2, 2017

New braille context presentation modes sometimes break panning backwards #7468

Closed

LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019

		@@ -1211,6 +1213,20 @@ def _set_windowEndPos(self, endPos):
		if region.focusToHardLeft:
		# Only scroll to the start of this region.

Uh oh!

Conversation

LeonarddeR commented May 31, 2017 • edited by jcsteh 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:

Open question

Uh oh!

dkager commented Jun 5, 2017

Uh oh!

LeonarddeR commented Jun 21, 2017

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!

LeonarddeR commented Jun 30, 2017

Uh oh!

jcsteh left a comment

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!

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!

jcsteh commented Jul 11, 2017

Uh oh!

LeonarddeR commented Jul 11, 2017 via email

Uh oh!

jcsteh left a comment

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!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Jul 12, 2017

LeonarddeR commented May 31, 2017 •

edited by jcsteh

Loading