-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Emacs keybindings #40
Conversation
Hey @redzic, this is great. I really like the I'm good to merge this, but I'm a little concerned about the different results with tests. This does not happen for me... does it happen consistently for you with this branch? Does it happen on main too? Mind if we do a quick check on this before merging just to be sure we're not breaking anything? |
Also, in order to fully close #36, we'll also need to change the controls line as mentioned in the issue. Would you like to address this as part of this PR, or would you rather this be done in a separate one (by you or someone else)? |
Yeah, maybe I'm running the tests incorrectly or something but even on the Here's a terminal screen capture to demonstrate this: And here's the raw output captured by Regarding the controls line/legend, I've just added a commit to change them according to what was said in #36, so let me know if there's anything I should change with that. |
It looks like the tests are failing because of the change in the UI difference, and it's still expecting the old UI to be rendered. |
Hey @redzic: thanks for the details about the tests failing. I'm really sorry for this experience. This is quite odd. I'd like to get to the bottom of this, but let's discuss this in a bit in another issue (if you're willing to help debug this, ofc). About the legend: Sorry for being a little unclear, I mean to also delete the hjkl part. Since now that we support emacs, we want to just have arrow that will mean "any arrow motions" (keyboard-arrows, vim or emacs). You'll also have to update the snapshots (because the legend changed, so it would be changed there too), and this might be a little difficult in your situation, I'm afraid.
This will walk you through the changes in the snapshots (you will be able to see the difference and approve/reject it). If you can't find a way to do this (due to the testing issues), then let's revert the legend changes, merge this as is and I'll do the legend in a different PR. Sounds good? |
Thank you for the very helpful reply! I've just followed the steps in the process, and it seems like I was still able to review the changes in the snapshots even though the tests are inconsistent for me. And what you said about the "any arrow motions working" makes sense to me. Hopefully whatever is causing the testing weirdness can be resolved. |
It also sounds like a good idea to open another issue about the testing to discuss it there. I think I'll do that right now. |
Hey, thanks for opening the other issue! I'll reply there a little later. What do you think of changing it to |
I agree that the brackets wrapping the arrow characters looks a little strange/confusing. I think it's a good idea to change it to just |
Awesome. Thanks! |
Looks like I forgot to change the UI test changes lol. Also messed up the commit message a little bit--oops! I think the tests should pass on this one. This is my first pull request on github actually, so thank you for this great project and the time you took to help me! |
Looks great. Thanks for your great work on this!
My pleasure and privilege. I hope to see you around this repo more. :) |
Fixes #36 ... again
I've added a key! macro to make it easier to define patterns like
Event::Key(Key::Char(..))
In my first PR I think I replaced one of the other keybindings with
\n
when it was supposed to ben
, so this should fix that, since the tests are working locally. Although for some reason if I run the same tests multiple times without changing the code I get different results.