-   Notifications  You must be signed in to change notification settings 
- Fork 750
Multi-line insertion #403
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
Multi-line insertion #403
Conversation
…e beginning of each line of the block
| Wonderful! Thank you for this! I just tested it locally, and it works beautifully. I'll have a second look later today or tomorrow and then I'll merge it. | 
| Thanks! | 
| Yes, I noticed. But that's a following step. I'd like to merge that separately, first I'd like to be sure that this is absolutely correct. | 
| I just committed a fix for the backspace/delete handling. | 
…e end of each line of the block
| I just committed the same thing for Shift-A (insert at the end of the selection block). | 
| insert = False | ||
| if event.data == '\x7f': # Backspace | ||
| j = [-1, -1, -1] | ||
| elif event.data == '\x1b[3~': # Delete | 
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.
Checking event.data is not reliable. (This won't work on Windows or when people configure a terminal to send different escape sequences. Better is to have separate key bindings, using for instance Keys.Delete.
| class InputMode(object): | ||
| INSERT = 'vi-insert' | ||
| INSERT_SELECTION_LEFT = 'vi-insert-selection-left' | ||
| INSERT_SELECTION_RIGHT = 'vi-insert-selection-right' | 
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'm not sure that we really need both INSERT_SELECTION_LEFT and INSERT_SELECTION_RIGHT.
|  | ||
| @handle('A', filter=selection_mode & ~IsReadOnly()) | ||
| def _(event): | ||
| event.current_buffer.selection_right_cursor_positions = [] | 
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.
We should never dynamically add fields to a class. A Buffer class does not define selection_right_cursor_positions, so we should not assign to this attribute without defining it first in the class itself.
| event.current_buffer.cursor_position = from_to[1] + 1 | ||
| event.cli.vi_state.input_mode = InputMode.INSERT_SELECTION_RIGHT | ||
| buffer = event.current_buffer | ||
| if bool(buffer.selection_state): | 
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.
Do we need the cast to bool?
| FYI: I left a few comments, but I'm about to start merging, with some changes. | 
| @handle(Keys.Backspace, filter=insert_selection_right_mode) | ||
| @handle(Keys.Delete, filter=insert_selection_right_mode) | ||
| @handle(Keys.Any, filter=insert_selection_right_mode) | ||
| def _(event): | 
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.
The code between inserting left and right is identical. I'm going to replace this by a feature I'd call "multiple cursors". Whether the insert positions are coming from the left or right side of the selection doesn't matter. We just have multiple positions where we can insert text.
| My code was probably not following good practices, so thanks a lot for your comments. Looking forward to seeing this feature in next release! | 
| Don't worry! The pull request saved me some work, so it's was valuable anyway. edit: TODO: unit tests. | 
| @davidbrochart, Thanks again for this! I merged your first commit, and made some changes for the rest. Normally, everything should be in prompt_toolkit. | 
| Thank you for merging and for the enhancements! | 
| This last commit is (not exactly) an implementation of the Vi substitute command (search and replace). I just took advantage of the multi-line insertion commit to go a bit further. In selection mode, it is now possible to enter the search mode (with  | 
| Or, instead of the second  | 
| Hi @davidbrochart, About searching in selection mode, this has been implemented, but slightly different. In commits: b217160 About the search/replace mode. I'm not a big fan of using enter to go into replace mode. It doesn't work like that in Vi or other editors, right? Just one question, if you'd like to propose more improvements (which are always very welcome), could you start from a new pull request? That makes it easier to review changes (and I could close this one). Thanks! | 
| Sure, I will create a new pull request, so that you can close this one. | 
Added Shift-I Vi command in block selection mode to insert text at the beginning of each line of the block.