Always report object states in same order, regardless whether positive or negative by LeonarddeR · Pull Request #7279 · nvaccess/nvda

LeonarddeR · 2017-06-13T08:15:46Z

Link to issue number:

Summary of the issue:

When reporting object states to the user in speech or braille, the positive object states are processed before the negative states. This creates the following problem as reported by @dkager:

Can be reproduced in many lists with checkable items in Windows 10, e.g. the disk clean-up utility.
Unchecked items are brailled as Foo sel ( ) chk , checked ones as Foo (x) sel chk .

Description of how this pull request fixes the issue:

For both speech and braille, de positive and negative states sets are merged and numerically sorted. When looping through this merged, sorted list of states, we check whether the state is positive or negative and the appropriate label will be spoken or brailled.
The negative state for controlTypes.STATE_DROPTARGET (Done dragging) is moved from speech.py to controlTypes.py

Testing performed:

In disk clean up, the selected state is now always spoken before the checked state, regardless whether positive or negative

Known issues with pull request:

As I noted in Inconsistent reporting of checkable list items #7076 (comment), This changes the order of presentation people are accustomed to in some situations. E.g. invalid entry required will change to required invalid entry. Do we care about this, or do we want to have a certain order of priority as @jcsteh suggested in Inconsistent reporting of checkable list items #7076 (comment)?
We probably want to test whether the "done dragging" case still works correctly.

…ame order, regardless whether they are positive or negative. Fixes #7076

dkager

Looks good to me. Quite a bit of code duplicaion, but I guess the states for braille and speech are not guaranteed to be identical?

dkager · 2017-06-22T14:47:10Z

+			elif state in negativeStates:
+				# Translators: Indicates that a particular state on an object is negated.
+				# Separate braille abbreviations have now been defined for commonly negated states (e.g. ( ) for not checked),
+				# 			and, if no braille abbreviations are provided, we fall back to the defined state labels in ControlTypes,


Slightly more spaces than necessary. :)

…hat in the speech and braille modules

LeonarddeR · 2017-06-23T16:50:31Z

-		negativeStates=propertyValues['negativeStates']
-	else:
-		negativeStates=None
-	if negativeStates is not None or (reason != controlTypes.REASON_CHANGE and states is not None):


@jcsteh: Could you clarify this line? Was there a particular reason why if negativeStates is not None wasn't enough?

Could you clarify this line? Was there a particular reason why if negativeStates is not None wasn't enough?

The negativeStates argument is only provided for REASON_CHANGE. It specifies states that have just changed from positive to negative. However, for other reasons (e.g. REASON_FOCUS), we still want to report certain states if they aren't present; e.g. "not checked".

Thanks. In the new code, it seems this check is no longer necessary as I provide empty sets as default

LeonarddeR · 2017-06-23T16:56:52Z

 		# Return all negative states which should be spoken, excluding the positive states.
 		return speakNegatives - states
+
+def processAndLabelStates(role, states, reason, positiveStates, negativeStates, positiveStateLabelDict={}, negativeStateLabelDict={}):


I'm happy to provide a Doc-string for this, but processPositiveStates and processNegativeStates don't have a doc string yet. May be the initial author of these functions can provide some explanation (e.g. about why we both have states and positiveStates parameters for processPositiveStates)? We can add doc strings for these in this pr as well.

why we both have states and positiveStates parameters for processPositiveStates

states contains the current states for the object. For REASON_CHANGE, positiveStates contains only those states that just got added. In other words, positiveStates contains only the states we want to possibly be reported at this time, but we might need the "unfiltered" states for the object, so we provide states as well.

LeonarddeR · 2017-06-29T13:02:48Z

@jcsteh: Apart from doc strings, I think this is ready for review.

…o test on

… this pr

… conflict

… conflicts

LeonarddeR · 2017-09-08T09:32:43Z

This now contains doc strings as well. @dkager: Would you like to review them? Your grammar is better than mine. :)

dkager

Looks pretty good.

dkager · 2017-09-20T10:39:36Z

-		textList.extend(positiveStateLabels.get(state, controlTypes.stateLabels[state]) for state in positiveStates)
-		negativeStates = controlTypes.processNegativeStates(role, states, controlTypes.REASON_FOCUS, None)
-		textList.extend(negativeStateLabels.get(state, controlTypes.negativeStateLabels.get(state, _("not %s") % controlTypes.stateLabels[state])) for state in negativeStates)
+		textList.extend(controlTypes.processAndLabelStates(role, states, controlTypes.REASON_FOCUS, states,None,positiveStateLabels,negativeStateLabels))


Inconsistent use of space after comma. Maybe pull this function out into two lines to improve readability?

dkager · 2017-09-20T10:42:45Z

 	# Translators: This is presented when a checkbox is not checked.
 	STATE_CHECKED:_("not checked"),
+	# Translators: This is presented when drag and drop is finished.
+	# This is only reported for objects which support accessible drag and drop.


I think "that support" is the correct form here.

dkager · 2017-09-20T10:44:40Z


 def processPositiveStates(role, states, reason, positiveStates):
+	"""Processes the states for an object and returns the positive states to output for a specified reason.
+	For example, if C{STATE_CHECKED} is among the returned states, it means that the processed object is checked.


Trivial I guess, but "in the returned states" is a bit closer to what you'd actually write in a program to check this.

dkager · 2017-09-20T10:45:30Z

+	For example, if C{STATE_CHECKED} is among the returned states, it means that the processed object is checked.
+	@param role: The role of the object to process states for (e.g. C{ROLE_CHECKBOX}.
+	@type role: int
+	@param states: Usually the raw states for an object to process.


This implies there are exceptions. What are they?

dkager · 2017-09-20T10:46:08Z

+	@type role: int
+	@param states: Usually the raw states for an object to process.
+	@type states: set
+	@param reason: The reason to process the states for (e.g. C{REASON_FOCUS}.


I think "for" should be removed.

dkager · 2017-09-20T10:47:00Z


 def processNegativeStates(role, states, reason, negativeStates):
+	"""Processes the states for an object and returns the negative states to output for a specified reason.
+	For example, if C{STATE_CHECKED} is among the returned states, it means that the processed object is not checked.


Same stuff as above.

dkager · 2017-09-20T10:49:25Z

 		# Return all negative states which should be spoken, excluding the positive states.
 		return speakNegatives - states
+
+def processAndLabelStates(role, states, reason, positiveStates, negativeStates, positiveStateLabelDict={}, negativeStateLabelDict={}):


In another PR @jcsteh cautioned against using empty dicts as default function parameters. I wonder if that applies here.

dkager · 2017-09-20T10:49:46Z

+	"""Processes the states for an object and returns the appropriate state labels for both positive and negative states.
+	@param role: The role of the object to process states for (e.g. C{ROLE_CHECKBOX}.
+	@type role: int
+	@param states: Usually the raw states for an object to process.


Same stuff as above.

dkager · 2017-09-20T10:51:22Z

+		if state in positiveStates:
+			mergedStateLabels.append(positiveStateLabelDict.get(state, stateLabels[state]))
+		elif state in negativeStates:
+			# Translators: Indicates that a particular state on an object is negated.


"of an object"

LeonarddeR · 2017-10-12T19:05:00Z

@dkager: Could you review my most recent commits?

dkager · 2017-12-03T11:13:02Z

As far as small changes bringing big joy go, this is still one of my favorites.

In speech and braille output, states will now always be reported in s…

09f4fe6

…ame order, regardless whether they are positive or negative. Fixes #7076

dkager reviewed Jun 22, 2017

View reviewed changes

Leonard de Ruijter added 2 commits June 22, 2017 18:09

Adressed review comment

75dfbdb

Create a new processAndLabelStates function in controlTypes and use t…

de0d6cf

…hat in the speech and braille modules

LeonarddeR commented Jun 23, 2017

View reviewed changes

LeonarddeR mentioned this pull request Jun 24, 2017

Inconsistent reporting of checkable list items #7076

Closed

Added unit tests for states

b869827

Leonard de Ruijter added 6 commits June 29, 2017 15:28

Small fix for negative state unit test

1de30dc

Make use of assertSetEqual and use de setUp method to set an object t…

8027e51

…o test on

Added additional controlTypes unit tests, not particularly related to…

a47eeb3

… this pr

Merge remote-tracking branch 'origin/master' into i7076, fixing merge…

7ba1cd2

… conflict

Merge remote-tracking branch 'origin/master' into i7076, fixing merge…

357c75c

… conflicts

Added doc strings

e6f178e

dkager suggested changes Sep 20, 2017

View reviewed changes

Leonard de Ruijter added 3 commits September 20, 2017 13:00

Merge remote-tracking branch 'origin/master' into i7076

e439860

Review actions

6ade488

Switch to processAndLabelStates for browseMode

c43baae

dkager approved these changes Oct 13, 2017

View reviewed changes

michaelDCurran approved these changes Dec 12, 2017

View reviewed changes

michaelDCurran added a commit that referenced this pull request Dec 12, 2017

Incubates #7279

ebbd6f4

nvaccessAuto added the incubating label Dec 12, 2017

nvaccessAuto assigned michaelDCurran Dec 12, 2017

michaelDCurran merged commit 3bd729e into nvaccess:master Jan 1, 2018

nvaccessAuto removed the incubating label Jan 1, 2018

nvaccessAuto added this to the 2018.1 milestone Jan 1, 2018

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

jcsteh mentioned this pull request Feb 12, 2020

Fix reporting of "not checked" for check boxes with no other states in browse mode. #10781

Merged

Uh oh!

Conversation

LeonarddeR commented Jun 13, 2017

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Uh oh!

dkager 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!

LeonarddeR commented Jun 29, 2017

Uh oh!

LeonarddeR commented Sep 8, 2017

Uh oh!

dkager 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!

LeonarddeR commented Oct 12, 2017

Uh oh!

dkager commented Dec 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants