Skip to content

Conversation

@davidbrochart
Copy link
Contributor

Added Shift-I Vi command in block selection mode to insert text at the beginning of each line of the block.

@jonathanslenders
Copy link
Member

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.

@davidbrochart
Copy link
Contributor Author

Thanks!
I just realized backspace and delete keys don't work though. Any idea?

@jonathanslenders
Copy link
Member

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.
(There are a few other keys in this mode that don't work, like ControlW for instance.)

@davidbrochart
Copy link
Contributor Author

I just committed a fix for the backspace/delete handling.

@davidbrochart
Copy link
Contributor Author

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
Copy link
Member

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'
Copy link
Member

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 = []
Copy link
Member

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):
Copy link
Member

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?

@jonathanslenders
Copy link
Member

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):
Copy link
Member

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.

@davidbrochart
Copy link
Contributor Author

My code was probably not following good practices, so thanks a lot for your comments. Looking forward to seeing this feature in next release!

@jonathanslenders
Copy link
Member

jonathanslenders commented Oct 9, 2016

Don't worry! The pull request saved me some work, so it's was valuable anyway.
I thing I probably also have to do is create a unit test for this. (Not much work.)

edit: TODO: unit tests.

@jonathanslenders
Copy link
Member

@davidbrochart, Thanks again for this! I merged your first commit, and made some changes for the rest. Normally, everything should be in prompt_toolkit.

@davidbrochart
Copy link
Contributor Author

Thank you for merging and for the enhancements!

@davidbrochart
Copy link
Contributor Author

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 /) and then to enter the replace mode (with a second /). Once the replace mode is entered, the matching text (in the selection) is deleted, and the user can enter the replacing text. The replace mode is exited by pressing Esc.
So it doesn't follow the Vi :s/foo/bar/g syntax, but at the same time it still seems quite logical: you start with a search and you go on with a replace if you want. One limitation is that now the / character cannot be used in the searched text. It is just a proof of concept, let me know if you find it interesting/acceptable (and sorry for using global variables!).

@davidbrochart
Copy link
Contributor Author

Or, instead of the second /, we could just hit Enter, just like in pure search mode. This way, there wouldn't be any limitation of not having a / character in the searched text. And because replacing is active only in selection mode, this behavior doesn't interfere with the pure search mode. So, the sequence would be:
/ -> enter searched text -> Enter -> enter replacing text -> Esc

@jonathanslenders
Copy link
Member

Hi @davidbrochart,

About searching in selection mode, this has been implemented, but slightly different. In commits: b217160
and e84abde

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!

@davidbrochart
Copy link
Contributor Author

Sure, I will create a new pull request, so that you can close this one.
I know it is not the Vi syntax, I will try to implement the real substitute command then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants