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

Add the possibility to assign braille display model specific gestures #7517

Merged
merged 6 commits into from Oct 3, 2017

Conversation

Projects
None yet
5 participants
@leonardder
Collaborator

leonardder commented Aug 23, 2017

Link to issue number:

None. However, this is a desired feature for #7458 and #7459. See also #6063 (comment)

Summary of the issue:

Currently, it is only possible to assign gestures to keys on the braille display driver level. This means that it is not possible to assign scripts to model specific gestures. Sometimes, it is desirable to have a particular gesture bound for a specific model only, for example as mentioned in #6063 (comment)

Description of how this pull request fixes the issue:

  1. Added a new model property to braille.BrailleDisplayGesture
  2. If the model property is not None, gesture.identifiers provides an additional identifier for the model in the form br(driver.model):keys
  3. getDisplayTextForIdentifier uses a regular expression to construct a name for the new model based identifiers. This is required, since getDisplayTextForIdentifier is a class method.
  4. Added model specific gestures support to the Baum driver. Other braille display drivers may follow later. This can also be added for #7458 and #7459.

Testing performed:

Assigned a model specific gesture on several handy tech displays using the native driver (work in progress, https://github.com/bramd/nvda/tree/handytech-native). Also successfully tested this with a Baum Vario Ultra 40.

Known issues with pull request:

None I'm aware of.

@dkager, could you provide feedback on the regex pattern? We might want unit tests for this.

Credits

This also contains code by @bramd

@leonardder leonardder requested a review from dkager Aug 23, 2017

@leonardder leonardder requested a review from michaelDCurran Aug 29, 2017

@leonardder leonardder added the Braille label Aug 29, 2017

@didiercolle

In general looks ok: trivial and to the point modifications, and good comments.

My main concern is regarding the string formatting versus the regular expression.

return handler.display.description, identifier.split(":", 1)[1]
idParts = cls.ID_PARTS_REGEX.search(identifier)
if not idParts:
raise ValueError("Invalid identifier provided: %s"%identifier)

This comment has been minimized.

@didiercolle

didiercolle Aug 30, 2017

I have the impression the old version of the function did not expect malformed input (despite in case of malformed input it may also have ended up in IndexErrors).
Raising an exception in case it is needed is good, but where will it be catched and handled in an appropriate way?

@didiercolle

didiercolle Aug 30, 2017

I have the impression the old version of the function did not expect malformed input (despite in case of malformed input it may also have ended up in IndexErrors).
Raising an exception in case it is needed is good, but where will it be catched and handled in an appropriate way?

This comment has been minimized.

@leonardder

leonardder Aug 31, 2017

Collaborator

This exception is raised to give a readable clarification of what is going wrong, if there is at all something going wrong. The old version of this function indeed didn't expect mallformed input, and I think this is a safe assumption. Personally I belief we shouldn't catch this, as when this is raised, it would be a quite serious error.

@leonardder

leonardder Aug 31, 2017

Collaborator

This exception is raised to give a readable clarification of what is going wrong, if there is at all something going wrong. The old version of this function indeed didn't expect mallformed input, and I think this is a safe assumption. Personally I belief we shouldn't catch this, as when this is raised, it would be a quite serious error.

This comment has been minimized.

@didiercolle

didiercolle Sep 1, 2017

Would an assert be more appropriate in that case?
At least from an intention point of view, it may resemble more the implicit contract of the original version of the function "never ever supply malformed input, otherwise you are in deep trouble" versus "in case you supply malformed input (please don't), I will be so kind to let you know".
Of course, in the end the effect would be the same, as python turns a failing assert also into an exception, in contrast to other languages where an assert triggers a pretty abrupt emergency break.

@didiercolle

didiercolle Sep 1, 2017

Would an assert be more appropriate in that case?
At least from an intention point of view, it may resemble more the implicit contract of the original version of the function "never ever supply malformed input, otherwise you are in deep trouble" versus "in case you supply malformed input (please don't), I will be so kind to let you know".
Of course, in the end the effect would be the same, as python turns a failing assert also into an exception, in contrast to other languages where an assert triggers a pretty abrupt emergency break.

This comment has been minimized.

@leonardder

leonardder Sep 1, 2017

Collaborator

Would an assert be more appropriate in that case?

Assertions get removed in release builds, so handling this with an assertion statement will make no difference to the original assumption that no malformed input will be supplied to this function. I can live with an assertion though.

@dkager @michaelDCurran, thoughts?

@leonardder

leonardder Sep 1, 2017

Collaborator

Would an assert be more appropriate in that case?

Assertions get removed in release builds, so handling this with an assertion statement will make no difference to the original assumption that no malformed input will be supplied to this function. I can live with an assertion though.

@dkager @michaelDCurran, thoughts?

super(InputGesture, self).__init__()
# Model identifiers should not contain spaces.
self.model = model.replace(" ", "")

This comment has been minimized.

@didiercolle

didiercolle Aug 30, 2017

why removing spaces and not colons ':'?
Would that not confuse your regular expression?

A dot '.' would probably be less an issue as the model name follows after the new introduced dot? (I am not that familiar with regular expressions....). But at least in the source part before the dot, you probably need also to remove dots now.

@didiercolle

didiercolle Aug 30, 2017

why removing spaces and not colons ':'?
Would that not confuse your regular expression?

A dot '.' would probably be less an issue as the model name follows after the new introduced dot? (I am not that familiar with regular expressions....). But at least in the source part before the dot, you probably need also to remove dots now.

This comment has been minimized.

@leonardder

leonardder Aug 31, 2017

Collaborator

I don't expect dots and colon in models name here, only spaces, and as the doc string for _get_model says on BrailleDisplayGesture, this must be a name without spaces.

@leonardder

leonardder Aug 31, 2017

Collaborator

I don't expect dots and colon in models name here, only spaces, and as the doc string for _get_model says on BrailleDisplayGesture, this must be a name without spaces.

This comment has been minimized.

@didiercolle

didiercolle Sep 1, 2017

That is an expectation; where is it enforced?
I still believe that a more restrictive regular expression should go hand in hand with: i) updating that doc string with a more strict requirement, and ii) an accordding sanitasation of the input.

@didiercolle

didiercolle Sep 1, 2017

That is an expectation; where is it enforced?
I still believe that a more restrictive regular expression should go hand in hand with: i) updating that doc string with a more strict requirement, and ii) an accordding sanitasation of the input.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 1, 2017

Collaborator

That is an expectation; where is it enforced?

May be @jcsteh could give details regarding the baum protocol and device identifiers?

Nevertheless, why should people provide model names containing colons or dots? This is very unlikely.

If you think something should be changed, feel free to provide code examples which I can incorporate, but honestly I'm not convinced.

Collaborator

leonardder commented Sep 1, 2017

That is an expectation; where is it enforced?

May be @jcsteh could give details regarding the baum protocol and device identifiers?

Nevertheless, why should people provide model names containing colons or dots? This is very unlikely.

If you think something should be changed, feel free to provide code examples which I can incorporate, but honestly I'm not convinced.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 3, 2017

Contributor

@leonardder commented on 1 Sep 2017, 16:03 GMT+10:

May be @jcsteh could give details regarding the baum protocol and device identifiers?

It's difficult to cite an exact reference, since there's no single reference for the Baum protocol (each series of display is documented separately, but there just happen to be commonalities). However, from what I've seen, the device identifier can only contain letters (both upper and lower case), numbers and spaces.

Nevertheless, why should people provide model names containing colons or dots? This is very unlikely.

It is, but you just never know. I don't know that you need to enforce it elsewhere, but the docstring should probably be explicit about only allowing letters, numbers and maybe dashes/underscores.

Contributor

jcsteh commented Sep 3, 2017

@leonardder commented on 1 Sep 2017, 16:03 GMT+10:

May be @jcsteh could give details regarding the baum protocol and device identifiers?

It's difficult to cite an exact reference, since there's no single reference for the Baum protocol (each series of display is documented separately, but there just happen to be commonalities). However, from what I've seen, the device identifier can only contain letters (both upper and lower case), numbers and spaces.

Nevertheless, why should people provide model names containing colons or dots? This is very unlikely.

It is, but you just never know. I don't know that you need to enforce it elsewhere, but the docstring should probably be explicit about only allowing letters, numbers and maybe dashes/underscores.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 8, 2017

Collaborator

I addressed the following:

  1. Updated doc strings to state that only alphanumeric model names are supported.
  2. Added an assertion to the Baum driver to check whether the model name is alphanumeric. Note thatI tested a Baum Pronto! which explicitly has a bang in its model name,. The device name for pronto is 'Baum Pronto'. This means that the gesture identifier contains baum twice in this case, but I'm reluctant to strip Baum from the name.
Collaborator

leonardder commented Sep 8, 2017

I addressed the following:

  1. Updated doc strings to state that only alphanumeric model names are supported.
  2. Added an assertion to the Baum driver to check whether the model name is alphanumeric. Note thatI tested a Baum Pronto! which explicitly has a bang in its model name,. The device name for pronto is 'Baum Pronto'. This means that the gesture identifier contains baum twice in this case, but I'm reluctant to strip Baum from the name.
@michaelDCurran

Looks good to me. However, in the actual pull request description, you say that it matches br[driver.model]:keys it should be br(driver.model):keys?

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 11, 2017

Collaborator

I'm sorry, it should be br(driver.model):keys indeed. Updated this accordingly in the pr description.

Collaborator

leonardder commented Sep 11, 2017

I'm sorry, it should be br(driver.model):keys indeed. Updated this accordingly in the pr description.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 13, 2017

Contributor

@leonardder: are you still waiting for more reviews from someone, or do you want me to incubate this now?

Contributor

michaelDCurran commented Sep 13, 2017

@leonardder: are you still waiting for more reviews from someone, or do you want me to incubate this now?

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 14, 2017

Collaborator

Yes, sure. I initially requested review from @dkager as well, so I'd be able to fix some issues beforehand if he would review earlier than you, but that didn't seem the case. :)

Collaborator

leonardder commented Sep 14, 2017

Yes, sure. I initially requested review from @dkager as well, so I'd be able to fix some issues beforehand if he would review earlier than you, but that didn't seem the case. :)

michaelDCurran added a commit that referenced this pull request Sep 19, 2017

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 3, 2017

Contributor

Does this need a changes for developers entry in What's new? I am ready to merge it to master.

Contributor

michaelDCurran commented Oct 3, 2017

Does this need a changes for developers entry in What's new? I am ready to merge it to master.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Oct 3, 2017

Collaborator

I think it needs both a changes entry and a changes for devs entryr:

Changes

  • When assigning gestures to keys on a Baum display, you can limit them to the model of the braille display in use (e.g. VarioUltra or Pronto). (#7517)

Changes for developers

  • The braille.BrailleDisplayGesture class now has an extra model property. If provided, pressing a key will generate an additional, model specific gesture identifier. This allows a user to bind gestures limited to a specific braille display model.
    • See the baum driver as an example for this new functionality.

Note that this functionality is also used in the Handy Tech driver, so everywhere where this states Baum, Handy Tech can be added to these entries as soon as #7458 is in master.

Collaborator

leonardder commented Oct 3, 2017

I think it needs both a changes entry and a changes for devs entryr:

Changes

  • When assigning gestures to keys on a Baum display, you can limit them to the model of the braille display in use (e.g. VarioUltra or Pronto). (#7517)

Changes for developers

  • The braille.BrailleDisplayGesture class now has an extra model property. If provided, pressing a key will generate an additional, model specific gesture identifier. This allows a user to bind gestures limited to a specific braille display model.
    • See the baum driver as an example for this new functionality.

Note that this functionality is also used in the Handy Tech driver, so everywhere where this states Baum, Handy Tech can be added to these entries as soon as #7458 is in master.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 3, 2017

Contributor

@leonardder: after merging your other braille pr (I think), There are now conflicts with this one.

Contributor

michaelDCurran commented Oct 3, 2017

@leonardder: after merging your other braille pr (I think), There are now conflicts with this one.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Oct 3, 2017

Collaborator

Fixed :)

Collaborator

leonardder commented Oct 3, 2017

Fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment