Add the possibility to assign braille display model specific gestures#7517
Conversation
didiercolle
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| def __init__(self, model, keysDown): | ||
| super(InputGesture, self).__init__() | ||
| # Model identifiers should not contain spaces. | ||
| self.model = model.replace(" ", "") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
|
@leonardder commented on 1 Sep 2017, 16:03 GMT+10:
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.
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. |
|
I addressed the following:
|
michaelDCurran
left a comment
There was a problem hiding this comment.
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?
|
I'm sorry, it should be br(driver.model):keys indeed. Updated this accordingly in the pr description. |
|
@LeonarddeR: are you still waiting for more reviews from someone, or do you want me to incubate this now? |
|
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. :) |
|
Does this need a changes for developers entry in What's new? I am ready to merge it to master. |
|
I think it needs both a changes entry and a changes for devs entryr: Changes
Changes for developers
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. |
|
@LeonarddeR: after merging your other braille pr (I think), There are now conflicts with this one. |
…s, fixing merge conflicts.
|
Fixed :) |
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:
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