-
-
Notifications
You must be signed in to change notification settings - Fork 49
Added unit tests for handling key events in processing:core
#966
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
Conversation
Just a follow up anything else I need to change here? |
Hey @Rishab87, thanks for your contribution! We're busy right now preparing the release of Processing 4.4 but we'll review your PR as soon as we can. |
Thanks for letting me know @SableRaf, please take your time. |
Also apart from this should I start with addition of more tests by then? |
If you'd like, but I'd suggest rather focusing on your GSoC application in the meantime :) |
Sure @SableRaf , I'll focus on my application then in the meantime. |
@SableRaf I've created first draft of my proposal, so should I start with writing other unit tests, like for math related functionalities their is no unit test currently present for it so should I continue with it? |
I've pushed some changes which fixes #779 but I don't think it's the most optimised approach. It unregisters all the hashes with that key code The issue seems to be when we press Ctrl and then A, Ctrl gets registered and ctrl+A gets registered, when we release ctrl we release 2 keys, ctrl and ctrl+A at the same time I'm open to hearing your thoughts on this, will make changes accordingly. |
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.
Hi @Rishab87 Looks good, just a small readability improvement and some cleanup
case KeyEvent.RELEASE -> { | ||
pressedKeys.remove(((long) keyCode << Character.SIZE) | key); | ||
case KeyEvent.RELEASE -> { | ||
List<Long> hashesToRemove = new ArrayList<>(); |
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.
Instead we can do
pressedKeys.removeIf(hash -> {
int pressedKeyCode = (int)(hash >> Character.SIZE);
return pressedKeyCode == keyCode;
});
applet.handleKeyEvent(releaseShift); | ||
|
||
Assert.assertFalse("keyPressed should be false after all keys released", applet.keyPressed); | ||
Assert.assertEquals("pressedKeys should be empty", true, applet.pressedKeys.isEmpty()); |
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.
Just use assertTrue
applet.handleKeyEvent(releaseAlt); | ||
|
||
Assert.assertFalse("keyPressed should be false after all keys released", applet.keyPressed); | ||
Assert.assertEquals("pressedKeys should be empty", true, applet.pressedKeys.isEmpty()); |
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.
Same here
Thanks for the review @Stefterv , I've made those changes |
@all-contributors please add @Rishab87 for code |
I've put up a pull request to add @Rishab87! 🎉 |
I've added unit tests to test key events. Here are the tests that I added:
testSingleKeyPressAndRelease
testShiftAndLetterSequence
testControlAndLetterSequence
testAltAndLetterSequence
testKeyRepeat
testKeyRepeatEnabled
testKeyFocusLost
As expected
testControlAndLetterSequence
andtestShiftAndLetterSequence
fail (see #779)Fixes #965
Update this PR also adds a fix to #779 , now all tests pass
Update, this also fixes #824