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

Implemented 'replace-char' and bind for vim mode #1595

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@erthalion

erthalion commented Aug 5, 2014

I've implemented 'replace-char' command for 'r' binding in vim mode. I hope it will be correct enough.

@jaredgrubb

This comment has been minimized.

Show comment
Hide comment
@jaredgrubb

jaredgrubb Sep 25, 2014

Contributor

Very cool .. I was just looking for this!

Contributor

jaredgrubb commented Sep 25, 2014

Very cool .. I was just looking for this!

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Sep 25, 2014

Member

This is on the right track but the handling of the mode changes is wonky. Instead of using env_set, take a look at the -m flag to bind (like in bind -m insert i force-repaint) to switch modes.

Member

ridiculousfish commented Sep 25, 2014

This is on the right track but the handling of the mode changes is wonky. Instead of using env_set, take a look at the -m flag to bind (like in bind -m insert i force-repaint) to switch modes.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Sep 25, 2014

Contributor

You can very nearly do this with just bindings:

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char force-repaint self-insert

The only difference is this will move the cursor ahead by one, which is not desired. Seems to me it might be better to come up with a way to make the '' binding able to run an action after the self-insert so it can fix the cursor (self-insert has to be last in the list now, because the typed key is then added after the command list, and that makes it the argument to self-insert).

A few possibilities:

  1. Provide a token that can be used in the command list to mean "insert the typed key here", which would prevent it from being added to the end. This would only be useful in the '' binding though, which might be a bit too specialized.
  2. If the command is a fishscript function, provide the typed key as an argument, and then provide a dummy input function that takes one argument and ignores it, which you can then put at the end of the input list to ignore the typed key that will be added to the end. With this approach the typed key can be provided to fishscript commands for all bindings, not just the '' binding, which can be useful.

I've also been meaning to add a flag to commandline that takes a sequence of characters and pushes them onto the input stack as if they were preceded by the self-insert function for each. This would work nicely with the second approach, because you could then easily have functions that conditionally interpret a sequence, or fall back to self-inserting them otherwise.

Although now that I think of it, a better approach for conditionally inserting would be to have a 'condition' flag given to the bind, that makes it fall back to interpreting the key sequence in light of other bindings if the condition fails.

Contributor

kballard commented Sep 25, 2014

You can very nearly do this with just bindings:

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char force-repaint self-insert

The only difference is this will move the cursor ahead by one, which is not desired. Seems to me it might be better to come up with a way to make the '' binding able to run an action after the self-insert so it can fix the cursor (self-insert has to be last in the list now, because the typed key is then added after the command list, and that makes it the argument to self-insert).

A few possibilities:

  1. Provide a token that can be used in the command list to mean "insert the typed key here", which would prevent it from being added to the end. This would only be useful in the '' binding though, which might be a bit too specialized.
  2. If the command is a fishscript function, provide the typed key as an argument, and then provide a dummy input function that takes one argument and ignores it, which you can then put at the end of the input list to ignore the typed key that will be added to the end. With this approach the typed key can be provided to fishscript commands for all bindings, not just the '' binding, which can be useful.

I've also been meaning to add a flag to commandline that takes a sequence of characters and pushes them onto the input stack as if they were preceded by the self-insert function for each. This would work nicely with the second approach, because you could then easily have functions that conditionally interpret a sequence, or fall back to self-inserting them otherwise.

Although now that I think of it, a better approach for conditionally inserting would be to have a 'condition' flag given to the bind, that makes it fall back to interpreting the key sequence in light of other bindings if the condition fails.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Sep 25, 2014

Member

Having a replace-one mode is a clever idea. I wonder if that's how it's implemented in vi.

Member

ridiculousfish commented Sep 25, 2014

Having a replace-one mode is a clever idea. I wonder if that's how it's implemented in vi.

@erthalion

This comment has been minimized.

Show comment
Hide comment
@erthalion

erthalion Sep 27, 2014

You can very nearly do this with just bindings

Yes, it can be done this way, but I think it will be better to use this function just for the sake of simplicity. Also, it's quite logical for me to provide the all basic methods for the edit (not only delete, or insert char). But it's only my point of view =)

the handling of the mode changes is wonky

Why? If I understand correctly, the command bind -m doing the same:

/**
   Perform the action of the specified binding
*/
static void input_mapping_execute(const input_mapping_t &m)
{
    ....
    input_set_bind_mode(m.sets_mode.c_str());
}

/**
    Set the current bind mode
*/
void input_set_bind_mode(const wcstring &bm)
{
    env_set(FISH_BIND_MODE_VAR, bm.c_str(), ENV_GLOBAL);
}

erthalion commented Sep 27, 2014

You can very nearly do this with just bindings

Yes, it can be done this way, but I think it will be better to use this function just for the sake of simplicity. Also, it's quite logical for me to provide the all basic methods for the edit (not only delete, or insert char). But it's only my point of view =)

the handling of the mode changes is wonky

Why? If I understand correctly, the command bind -m doing the same:

/**
   Perform the action of the specified binding
*/
static void input_mapping_execute(const input_mapping_t &m)
{
    ....
    input_set_bind_mode(m.sets_mode.c_str());
}

/**
    Set the current bind mode
*/
void input_set_bind_mode(const wcstring &bm)
{
    env_set(FISH_BIND_MODE_VAR, bm.c_str(), ENV_GLOBAL);
}
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Sep 27, 2014

Member

Why? If I understand correctly, the command bind -m doing the same

Yes, bind -m does the same, so please use bind -m so we don't duplicate the logic :)

Member

ridiculousfish commented Sep 27, 2014

Why? If I understand correctly, the command bind -m doing the same

Yes, bind -m does the same, so please use bind -m so we don't duplicate the logic :)

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Sep 28, 2014

Contributor

Yes, it can be done this way, but I think it will be better to use this function just for the sake of simplicity.

Adding a brand new input function is simpler? I disagree. Binds that can be done in fishscript, should be done in fishscript.

If we want to have a custom input function for this, then we should go whole-hog and bake knowledge of vi mode into the bindings, adding custom input functions for everything that's non-trivial to do with fishscript. But I'd rather not do that. Instead, I'd rather figure out how to extend the bind system to be more powerful, so that things like this bind can be written really trivially.

Contributor

kballard commented Sep 28, 2014

Yes, it can be done this way, but I think it will be better to use this function just for the sake of simplicity.

Adding a brand new input function is simpler? I disagree. Binds that can be done in fishscript, should be done in fishscript.

If we want to have a custom input function for this, then we should go whole-hog and bake knowledge of vi mode into the bindings, adding custom input functions for everything that's non-trivial to do with fishscript. But I'd rather not do that. Instead, I'd rather figure out how to extend the bind system to be more powerful, so that things like this bind can be written really trivially.

@erthalion

This comment has been minimized.

Show comment
Hide comment
@erthalion

erthalion Oct 4, 2014

@kballard , You're ignored another part of my message =)

Also, it's quite logical for me to provide the all basic methods for the edit (not only delete, or insert char)

I really think, that for such base operation like the char replacement, the introduction of a new binding mode (with the all consequences about the self-insert) is overkill.

I agree, the improvement of the bind system is a good intention. But I have one question - why the ability to run an action after the self-insert is better? I mean, can it be useful outside this issue?

erthalion commented Oct 4, 2014

@kballard , You're ignored another part of my message =)

Also, it's quite logical for me to provide the all basic methods for the edit (not only delete, or insert char)

I really think, that for such base operation like the char replacement, the introduction of a new binding mode (with the all consequences about the self-insert) is overkill.

I agree, the improvement of the bind system is a good intention. But I have one question - why the ability to run an action after the self-insert is better? I mean, can it be useful outside this issue?

@erthalion

This comment has been minimized.

Show comment
Hide comment
@erthalion

erthalion Oct 4, 2014

I'm asking only because I'm trying to understand your point. Of course, I don't know this project well enough, so I can be mistaken =)

erthalion commented Oct 4, 2014

I'm asking only because I'm trying to understand your point. Of course, I don't know this project well enough, so I can be mistaken =)

@tannhuber

This comment has been minimized.

Show comment
Hide comment
@tannhuber

tannhuber Oct 7, 2014

Contributor

I'm also waiting for this impatiently. My PC is more than 13 years old. This means,

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char force-repaint self-insert

is slow-motion here. @erthalion's replace-char is fast and positions the cursor correctly.

Contributor

tannhuber commented Oct 7, 2014

I'm also waiting for this impatiently. My PC is more than 13 years old. This means,

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char force-repaint self-insert

is slow-motion here. @erthalion's replace-char is fast and positions the cursor correctly.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Oct 10, 2014

Contributor

It's only fast because it's not repainting. But not repainting is a bug. It's shifting the mode to "insert", but because it's not repainting, the prompt doesn't get a chance to adjust in order to display the fact that it's in insert mode. If you set up an insert mode binding that repaints, and you hit that after pressing r, then you'll see it repaint.

Besides, being in insert mode is actually wrong. Insert mode bindings are not supposed to be active when replacing a character in Vim.

If you want a solution written in C++, then the solution has to leave the binding mode alone and instead subvert the character-reading process earlier on, before it processes any bindings.

Contributor

kballard commented Oct 10, 2014

It's only fast because it's not repainting. But not repainting is a bug. It's shifting the mode to "insert", but because it's not repainting, the prompt doesn't get a chance to adjust in order to display the fact that it's in insert mode. If you set up an insert mode binding that repaints, and you hit that after pressing r, then you'll see it repaint.

Besides, being in insert mode is actually wrong. Insert mode bindings are not supposed to be active when replacing a character in Vim.

If you want a solution written in C++, then the solution has to leave the binding mode alone and instead subvert the character-reading process earlier on, before it processes any bindings.

@tannhuber

This comment has been minimized.

Show comment
Hide comment
@tannhuber

tannhuber Oct 12, 2014

Contributor

@kballard: Thanks for the explanation. I understand that. Hopefully, somebody will find a proper solution soon.

Contributor

tannhuber commented Oct 12, 2014

@kballard: Thanks for the explanation. I understand that. Hopefully, somebody will find a proper solution soon.

@erthalion

This comment has been minimized.

Show comment
Hide comment
@erthalion

erthalion Oct 23, 2014

Insert mode bindings are not supposed to be active when replacing a character in Vim

Yes, I looked through the vim and readline source code and saw nothing about the insert mode when replacing a character. Sorry for the incorrect approach =)
Is that solution better?

erthalion commented Oct 23, 2014

Insert mode bindings are not supposed to be active when replacing a character in Vim

Yes, I looked through the vim and readline source code and saw nothing about the insert mode when replacing a character. Sorry for the incorrect approach =)
Is that solution better?

@tannhuber

This comment has been minimized.

Show comment
Hide comment
@tannhuber

tannhuber May 9, 2015

Contributor

@kballard: I've tested @erthalion's PR successfully for a long time. Is there a chance to merge?

I apologize for being impatient. But the lack of replace confuses me almost every day. :-)

Contributor

tannhuber commented May 9, 2015

@kballard: I've tested @erthalion's PR successfully for a long time. Is there a chance to merge?

I apologize for being impatient. But the lack of replace confuses me almost every day. :-)

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard May 9, 2015

Contributor

Offhand, the current version looks ok. But I actually haven't relatively little familiarity with the insert code, and I'd feel more comfortable if someone else (such as @ridiculousfish) chimed in.

Contributor

kballard commented May 9, 2015

Offhand, the current version looks ok. But I actually haven't relatively little familiarity with the insert code, and I'd feel more comfortable if someone else (such as @ridiculousfish) chimed in.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish May 10, 2015

Member

I am sorry that I let this slide for so long. Let's tackle after 2.2.

Member

ridiculousfish commented May 10, 2015

I am sorry that I let this slide for so long. Let's tackle after 2.2.

@tannhuber

This comment has been minimized.

Show comment
Hide comment
@tannhuber

tannhuber May 11, 2015

Contributor

Great!

Contributor

tannhuber commented May 11, 2015

Great!

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 20, 2015

Member

Revisiting this. kballard's post about implementing this using key bindings is very compelling. An example benefit is that the fish_mode_prompt can be trivially taught about replace mode - something that would be harder if we baked it into the core read loop.

Still the cursor positioning is a problem, and so is the behavior of self-insert. For example, typing r followed by right arrow will attempt to self-insert the escape sequence for right arrow, which ends very badly.

I propose fixing this by splitting the input queue into a "command list" and a "character list." Most functions will prefer to dequeue from the command list first, and then the character list if the command list is empty. But crucially, self-insert will only dequeue from the character list. This lets us implement replace as:

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char self-insert backward-char force-repaint

which seems very elegant.

Member

ridiculousfish commented Jul 20, 2015

Revisiting this. kballard's post about implementing this using key bindings is very compelling. An example benefit is that the fish_mode_prompt can be trivially taught about replace mode - something that would be harder if we baked it into the core read loop.

Still the cursor positioning is a problem, and so is the behavior of self-insert. For example, typing r followed by right arrow will attempt to self-insert the escape sequence for right arrow, which ends very badly.

I propose fixing this by splitting the input queue into a "command list" and a "character list." Most functions will prefer to dequeue from the command list first, and then the character list if the command list is empty. But crucially, self-insert will only dequeue from the character list. This lets us implement replace as:

bind -m replace-one r force-repaint
bind -M replace-one -m default '' delete-char self-insert backward-char force-repaint

which seems very elegant.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 20, 2015

Member

Instead of that, I made R_SELF_INSERT simply dequeue until it finds a real character, then put all of the readline functions back. This avoids messing with the input queue, for the time being. Things may get stickier with capital-R replace.

Member

ridiculousfish commented Jul 20, 2015

Instead of that, I made R_SELF_INSERT simply dequeue until it finds a real character, then put all of the readline functions back. This avoids messing with the input queue, for the time being. Things may get stickier with capital-R replace.

ridiculousfish added a commit that referenced this pull request Jul 20, 2015

Make R_SELF_INSERT only insert characters, not readline functions
This enables readline functions to appear after the self-insert
when defining bindings. This is preparation for fixing #1595.
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 20, 2015

Member

lowercase r-replace went in as 2109af0. Speak up if it doesn't work and thanks to everyone in this thread.

Member

ridiculousfish commented Jul 20, 2015

lowercase r-replace went in as 2109af0. Speak up if it doesn't work and thanks to everyone in this thread.

@ridiculousfish ridiculousfish added this to the next-2.x milestone Jul 20, 2015

@tannhuber

This comment has been minimized.

Show comment
Hide comment
@tannhuber

tannhuber Jul 20, 2015

Contributor

Awesome! Thank you very very much!

Contributor

tannhuber commented Jul 20, 2015

Awesome! Thank you very very much!

@martinklepsch

This comment has been minimized.

Show comment
Hide comment
@martinklepsch

martinklepsch Jul 29, 2015

I just installed from master via brew unlink fish; brew install --HEAD fish but pressing r does not let me replace the character under the cursor. Instead it seems to be ignored and whatever character is pressed next is evaluated. E.g. re moves cursor to end of current word and stays in normal mode.

-> fish -v
fish, version 2.2.0-127-g299a383

299a383 is the latest rev. Any suggestions welcome!

martinklepsch commented Jul 29, 2015

I just installed from master via brew unlink fish; brew install --HEAD fish but pressing r does not let me replace the character under the cursor. Instead it seems to be ignored and whatever character is pressed next is evaluated. E.g. re moves cursor to end of current word and stays in normal mode.

-> fish -v
fish, version 2.2.0-127-g299a383

299a383 is the latest rev. Any suggestions welcome!

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