-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fixes #1145 #1272
fixes #1145 #1272
Conversation
return true; | ||
} else { | ||
// Check to see if a remapping could potentially be applied when more keys are received | ||
for (let remap of this._remappings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this a return value rather than returning it implicitly as a class property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want me to just make a getter for it? It is already used on lines 77 and 83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or do you mean break off the function into a isRemappingPotential() function that takes the current keystrokes as an input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean could we return this value from sendKey
somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am missing something obvious that you can see pretty clearly...
It is definitely possible, the problem I see is that we call handleKeyEvent, which calls sendKey, then it calls handleKeyEventHelper which uses the property
I could make a data structure that returned a few properties but I don't think it will end up being cleaner...
sendkey only returning a boolean seems to make sense to me, I would really only change this to return a true/false/maybe with the data structure anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, it would be annoying to thread through. OK, a getter is fine.
this also fixes #1222 |
@johnfn should be good to go now, let me know if there is anything else |
should work...tested the few cases that were in the issue reported