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

Allow control-E to complete a command #932

Closed
waterhouse opened this Issue Jul 27, 2013 · 12 comments

Comments

Projects
None yet
4 participants
@waterhouse
Contributor

waterhouse commented Jul 27, 2013

At the moment, fish's command completion appears to work like this: If, e.g., I've recently typed:
sudo port selfupdate; port echo outdated
then, if I type sudo port se, the letters lfupdate; port echo outdated appear highlighted in yellow. At this point, I can use either control-F or the right arrow to accept the completion text, making it as though I had typed it myself.

I'd like to be able to use control-E to do this too. I find it easier to type control-E than control-F in this circumstance (I use the Dvorak keyboard), and it is logically permissible because control-E has rightward-cursor-moving semantics.

To be precise: if control-E is used when the cursor is at the end of a (partially) typed command, and if there is completion text, then control-E should accept the completion text and move the cursor to the end of the (now larger) command. control-E should not be otherwise changed. (Consequently, if you type a partial command, then move the cursor left, it would take two control-E's to accept the completion.)

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 27, 2013

Member

fish allows you to change your keyboard bindings: bind \ce forward-word will do what you want. Note that you need to add it to the fish_user_keybindings function to have it loaded on startup.

Member

zanchey commented Jul 27, 2013

fish allows you to change your keyboard bindings: bind \ce forward-word will do what you want. Note that you need to add it to the fish_user_keybindings function to have it loaded on startup.

@waterhouse

This comment has been minimized.

Show comment
Hide comment
@waterhouse

waterhouse Jul 27, 2013

Contributor

I'm afraid that doesn't do what I want. First of all, going forward by a word of completed text is not the same as going to the end of the completion text if that text is more than one word. Second, and more seriously, control-E normally moves the cursor to the end of the line, and rebinding it destroys that behavior.

Currently, \ce is bound to end-of-line. The solution would be to modify the end-of-line procedure so that, if it is invoked when already at the end of a (typed) line and there is pending completion text, it will accept the completion text and move to the end of that.

Contributor

waterhouse commented Jul 27, 2013

I'm afraid that doesn't do what I want. First of all, going forward by a word of completed text is not the same as going to the end of the completion text if that text is more than one word. Second, and more seriously, control-E normally moves the cursor to the end of the line, and rebinding it destroys that behavior.

Currently, \ce is bound to end-of-line. The solution would be to modify the end-of-line procedure so that, if it is invoked when already at the end of a (typed) line and there is pending completion text, it will accept the completion text and move to the end of that.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 27, 2013

Member

Sorry, I meant forward-char. I see what you mean now though, I think #344 covers the same thing.

Member

zanchey commented Jul 27, 2013

Sorry, I meant forward-char. I see what you mean now though, I think #344 covers the same thing.

@waterhouse

This comment has been minimized.

Show comment
Hide comment
@waterhouse

waterhouse Jul 27, 2013

Contributor

I observe that, in reader.cpp, the code corresponding to the forward-char function uses accept_autosuggestion(true) to, well, accept, and uses a simple length comparison to detect end-of-command.

            case R_FORWARD_CHAR:
            {
                if (data->buff_pos < data->command_length())
                {
                    data->buff_pos++;
                    reader_repaint();
                }
                else
                {
                    accept_autosuggestion(true);
                }
                break;
            }

I also observe the code corresponding to R_FORWARD_WORD and R_END_OF_LINE. The fix appears straightforward. This is what I did to the R_END_OF_LINE case in reader.cpp:

            case R_END_OF_LINE:
            {
              if (data->buff_pos < data->command_length())
              {
                  while (buff[data->buff_pos] &&
                          buff[data->buff_pos] != L'\n')
                  {
                      data->buff_pos++;
                  }
              }
              else
              {
                accept_autosuggestion(true);
              }

              reader_repaint();
              break;
            }

Doing this and recompiling fish does exactly what I want.

Will this be accepted? Should I submit a diff or something to make it easier for project administrators?

Contributor

waterhouse commented Jul 27, 2013

I observe that, in reader.cpp, the code corresponding to the forward-char function uses accept_autosuggestion(true) to, well, accept, and uses a simple length comparison to detect end-of-command.

            case R_FORWARD_CHAR:
            {
                if (data->buff_pos < data->command_length())
                {
                    data->buff_pos++;
                    reader_repaint();
                }
                else
                {
                    accept_autosuggestion(true);
                }
                break;
            }

I also observe the code corresponding to R_FORWARD_WORD and R_END_OF_LINE. The fix appears straightforward. This is what I did to the R_END_OF_LINE case in reader.cpp:

            case R_END_OF_LINE:
            {
              if (data->buff_pos < data->command_length())
              {
                  while (buff[data->buff_pos] &&
                          buff[data->buff_pos] != L'\n')
                  {
                      data->buff_pos++;
                  }
              }
              else
              {
                accept_autosuggestion(true);
              }

              reader_repaint();
              break;
            }

Doing this and recompiling fish does exactly what I want.

Will this be accepted? Should I submit a diff or something to make it easier for project administrators?

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 27, 2013

Member

#91 even.

Member

zanchey commented Jul 27, 2013

#91 even.

@waterhouse

This comment has been minimized.

Show comment
Hide comment
@waterhouse

waterhouse Jul 27, 2013

Contributor

Yes, I see a trail of previous issues. #91 seems to be on essentially the same issue. Should I comment there rather than here?

Contributor

waterhouse commented Jul 27, 2013

Yes, I see a trail of previous issues. #91 seems to be on essentially the same issue. Should I comment there rather than here?

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 27, 2013

Member

Either is fine. A diff would be good, or even a pull request. Change looks good though.

Member

zanchey commented Jul 27, 2013

Either is fine. A diff would be good, or even a pull request. Change looks good though.

@waterhouse

This comment has been minimized.

Show comment
Hide comment
@waterhouse

waterhouse Jul 27, 2013

Contributor

Diff provided (see other thread).

Contributor

waterhouse commented Jul 27, 2013

Diff provided (see other thread).

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jul 27, 2013

Member

No objection from this fish. Merge or not as you see fit, zanchey.

Member

ridiculousfish commented Jul 27, 2013

No objection from this fish. Merge or not as you see fit, zanchey.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Jul 27, 2013

Member

I'm fine with this change too. I don't use CTRL+E, but it doesn't look like this change will break anything (except for using CTRL+E when it would do nothing anyway).

Member

xfix commented Jul 27, 2013

I'm fine with this change too. I don't use CTRL+E, but it doesn't look like this change will break anything (except for using CTRL+E when it would do nothing anyway).

zanchey added a commit that referenced this issue Jul 27, 2013

Ctrl+E should insert suggested completion and then go to end of line
(Closes #91, #932)

Currently, control-E is bound to `end-of-line`.

This patch modifes the `end-of-line` procedure so that, if it is invoked when
the cursor is at the end of a command and there is pending completion text,
it will accept the completion text and move to the end. The behavior of
`end-of-line` will not otherwise be altered.
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 27, 2013

Member

fixed on master with e3ea953

Thanks!

Member

zanchey commented Jul 27, 2013

fixed on master with e3ea953

Thanks!

@zanchey zanchey closed this Jul 27, 2013

@waterhouse

This comment has been minimized.

Show comment
Hide comment
@waterhouse

waterhouse Jul 27, 2013

Contributor

Excellent! Glad to have helped.

Contributor

waterhouse commented Jul 27, 2013

Excellent! Glad to have helped.

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