Skip to content
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

Fix input gesture identifier normalization. #6962

Merged
merged 3 commits into from Mar 29, 2017
Merged

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Mar 10, 2017

Previously, arbitrarily ordered items (e.g. multiple keys) were reordered using Python set order. However, different input ordering could incorrectly cause different output in some cases due to hash bucket conflicts. For example, dot4+space and space+dot4 incorrectly produced different output.
In addition, these items were reordered before conversion to lower case instead of after, which could also incorrectly result in different output.
Now, the entire identifier is converted to lower case and arbitrarily ordered items are then sorted by character.
The InputGesture.identifiers property has been changed so that it is no longer normalized. Instead, there is now an InputGesture.normalizedIdentifiers property which returns normalized identifiers.
This was done to make normalization changes easier to apply and to eliminate the potential for normalization errors in InputGesture subclasses.
The InputGesture.logIdentifier property is now deprecated. InputGesture.identifiers[0] should be used instead.
Finally, various InputGesture implementations have been changed to remove normalization code. In some cases, this may result in nicer reporting in input help mode.
Fixes #6945. Re #3157.

Previously, arbitrarily ordered items (e.g. multiple keys) were reordered using Python set order. However, different input ordering could incorrectly cause different output in some cases due to hash bucket conflicts. For example, dot4+space and space+dot4 incorrectly produced different output.
In addition, these items were reordered *before* conversion to lower case instead of after, which could also incorrectly result in different output.
Now, the entire identifier is converted to lower case and arbitrarily ordered items are then sorted by character.
The InputGesture.identifiers property has been changed so that it is no longer normalized. Instead, there is now an InputGesture.normalizedIdentifiers property which returns normalized identifiers.
This was done to make normalization changes easier to apply and to eliminate the potential for normalization errors in InputGesture subclasses.
The InputGesture.logIdentifier property is now deprecated. InputGesture.identifiers[0] should be used instead.
Finally, various InputGesture implementations have been changed to remove normalization code. In some cases, this may result in nicer reporting in input help mode.
@jcsteh jcsteh requested a review from feerrenrut March 10, 2017 13:02
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried that there may be issues caused by localisation. Other than that a few little things I noticed and a few questions.

"""
identifier = identifier.lower()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this locale dependant? Will it use the system locale or the invariant locale by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the string has any non-ASCII characters in it, it should be unicode, in which case .lower() will operate using the Unicode database:

>>> ua = u'\u0391'
>>> unicodedata.name(ua)
'GREEK CAPITAL LETTER ALPHA'
>>> la = ua.lower(); la
u'\u03b1'
>>> unicodedata.name(la)
'GREEK SMALL LETTER ALPHA'

for modVk, modExt in self.generalizedModifiers:
if isNVDAModifierKey(modVk, modExt):
modTexts.add("NVDA")
modTexts.append("NVDA")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be uppercase for some reason? It will get converted to lowercase right? Or is it for the log output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets converted to lower case when normalised. However, we want it to be displayed in upper case in log output, since that's how we write it in documentation.

@@ -160,22 +160,10 @@ def _get__rawIdentifiers(self):
if self.tracker.actionCount>1:
ID+="%s_"%self.counterNames[min(self.tracker.actionCount,4)-1]
ID+=self.tracker.action
IDs.append("TS(%s):%s"%(self.mode,ID))
IDs.append("ts(%s):%s"%(self.mode,ID))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does ts mean? could you add a comment to explain please.

jcsteh added a commit that referenced this pull request Mar 14, 2017
@jcsteh jcsteh merged commit 8f853ce into master Mar 29, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.2 milestone Mar 29, 2017
@jcsteh jcsteh deleted the i6945GestureIdOrder branch March 29, 2017 06:58
jcsteh added a commit that referenced this pull request Mar 29, 2017
jcsteh added a commit that referenced this pull request Jun 21, 2017
…rs; we got rid of that in #6962. Make it possible to get the display text from a braille keyboard gesture identifier (as used in the Input Gestures dialog).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants