lib/mp-readline/readline.c: Added backward/forward/kill-word support. #5024
Conversation
|
This is a good addition in general I think as it makes the REPL more useful, problem for me personally is that I don't use standard readline shortcuts for this but the 'gui text editor-style' ones (Ctrl+Left Arrow, Ctrl+Del etc), which as far as I know this is not that uncommon (CPython's REPL also has a subset of it for instance). Now the core logic is the same, so if you refactor what is now in the switch statement into functions or macros it would be fairly easy to provide both sets of shortcuts. possibly configurable? |
|
I'll be happy to add hooks for
I'm also getting some ctypes-related build errors for various ports that I don't really understand and need to investigate. |
There was a problem hiding this comment.
Thanks for the contribution. Please note that MicroPython needs to maintain a relatively small footprint, so adding functionality like this is not always the right way to go. Nevertheless, if it's a small impact in code size for a large impact in usability then it'd be considered for inclusion.
It might be good to conditionally include the new features here with the existing MICROPY_REPL_EMACS_KEYS option.
Yes I know, quite an oversight imo. Which is likely why it gets usually reimplemented in the IDEs using it. Anyway I can understand if this is considered too non-standard for a REPL but I can always implement it in a fork. |
…d using ctypes. Fixed formatting.
|
Thank you, @dpgeorge, for this project! I've done a lot of microcontroller firmware development, some in C but mostly Assembly, and the ability to use Python on these platforms is really amazing. I understand that MicroPython needs to stay small, but these key bindings will make the REPL more functional for me, and hopefully others. I apologize in advance if my code is doing anything that's obviously ridiculous. I'll look into making this feature condition with that |
lib/mp-readline/readline.c
Outdated
| #define FORWARD true | ||
| #define BACKWARD false | ||
|
|
||
| size_t get_word_cursor_pos(bool direction) { |
There was a problem hiding this comment.
This can be made a STATIC function, since it's local to this file.
Also direction can be an int taking value 1 or -1, then there's no need to convert it to cursor_pos_step, nor have FORWARD and BACKWARD constants.
lib/mp-readline/readline.c
Outdated
| char buf_idx_offset = direction == FORWARD ? 0 : -1; | ||
| size_t cursor_pos = rl.cursor_pos; | ||
| bool in_leading_nonalphanum = true; | ||
| char c; |
There was a problem hiding this comment.
char c should go below in the while loop.
lib/mp-readline/readline.c
Outdated
| if (!in_leading_nonalphanum) { | ||
| break; | ||
| } | ||
| } else if (in_leading_nonalphanum) { |
There was a problem hiding this comment.
This check is not needed, can just be an else (saves code size).
lib/mp-readline/readline.c
Outdated
| } else if (in_leading_nonalphanum) { | ||
| in_leading_nonalphanum = false; | ||
| } | ||
| if (direction == FORWARD) { |
There was a problem hiding this comment.
No need for this check, just unconditionally test if cursor_pos >= rl.line->len
lib/mp-readline/readline.c
Outdated
| } | ||
| cursor_pos += cursor_pos_step; | ||
| } | ||
| return cursor_pos; |
There was a problem hiding this comment.
Can probably return cursor_pos - rl.cursor_pos here, since all callers just want the number of chars skipped.
|
Eventually this would also need tests, see eg |
|
I know |
|
That makes already for 3 different shortcuts people want for this. So whether or not they all get implemented in the main port, I really think it's for the best if all functionality is in seperate functions and/or macros so people who want a shortcut not available in the main port can easily add it in their fork by referring to said functions/macros simply by adding switch cases and populating them with clls like |
|
Since alt- shortcuts are not currently used anywhere in this readline implementation it would be important to check -- for multiple terminal programs -- that this PR uses the correct escape sequences for it. For me (Linux + urxvt) it works correctly.
The simplest option is not to add any :) Adding a way to customise the shortcuts via a config file would add more code than just supporting all the different keys by default. So I would suggest that by default there just be one shortcut and it be consistent with some existing application, eg emacs. |
|
Doesn't work on windows port but that's because the mapping in windows_mphal.c doesn't implement |
Per: micropython#5024 (comment) Made the function STATIC, changed direction to an int, moved 'char c' into the while loop, removed unnecessary check, renamed function to get_next_word_cursor_offset() and made it return the signed cursor position offset to the next word in the specified direction. Also changed the shortcut comments to reflect the Emacs key notation.
|
@derekenos do you plan on finishing this? I wrote a quick I can pick it up / continue with my impl. I've also made it work for "Ctrl+Right" / "Ctrl+Left" since I usually use them. |
|
@Jongy please feel free to pick up this up or continue with your own. I was at the point of adding tests but became busy with other things. I considered closing this PR but hoped to come back to it, and didn't want the feature-relevant conversation contained herein to disappear from sight. |
|
#5420 was merged. |
Don't blink blue on non-BLE workflow boards
This PR adds forward-word (Alt-f), backward-word (Alt-b), kill-word (Alt-d), and backward-kill-word (Alt-Backspace) support to readline.