builtin commandline: use readline #9398

phillipwood · 2022-11-30T15:01:42Z

Description

Currently in a key-binding commandline --insert and commandline --cursor are always be executed before commandline -f. To work around this commandline -f special cases begin-undo-group and end-undo-group so they work when combined with text insertion but that means they do not work with delete-char or kill-word etc. This PR uses the suggestion in src/builtin/commandline.cpp to try and fix that. It converts the builtin commandline function to use readline functions for moving the cursor and inserting text. This has the advantage that commandline --insert and commandline --cursor can be freely mixed with commandline -f and they are executed in the order that they are written.

Some notes on the commits:

The second commit is larger than I'd like, it would be smaller if we could avoid renaming enum readline_cmd_t by choosing a different name for the class but I failed to come up with a good name.
The fourth commit should perhaps be squashed into the fifth to avoid having to comment out the temporary test failures
The fifth commit would benefit from using std::variant but that is not an option in C++11. I did look at some C++11 implementations with a view to importing one but they were all huge.

Fixes #3031

TODOs:

Changes to fish usage are reflected in user documentation/manpages.
Tests have been added for regressions fixed
User-visible changes noted in CHANGELOG.rst

Add some tests in preparation for changing the commandline builtin to use readline functions when moving the cursor and inserting text.

In preparation for adding readline commands that have data associated with them convert readline_cmd_t from an enum to a class. The enum is renamed to readline_cmd_t::id_t. There are a lot of changes in this commit but they all fall into one of three categories - rename readline_cmd_t to readline_cmd_t::id_t. - use the .id() instance method to access the enum member. - convert functions that take or return readline_cmd_t by value to use const references instead.

In preparation for using readline functions to implement `commandline --cursor` and `commandline --insert` factor out the code that finds the boundaries of the current token/job/process in the command line buffer so that it can be used from the reader as well as in the commandline builtin.

This means that the cursor movement is no-longer executed out of order when `commandline --cursor <pos>` is mixed with `commandline -f <command>` Before this change commandline -f begin-selection commandline --cursor 10 commandline -f kill-selection end-selection does not work as expected because the cursor is moved before any of the readline functions are executed. This change breaks a number of the commandline test cases that combine `commandline --cursor` with `commandline --append/insert/replace`. They are temporarily commented out, the next commit will make them pass again.

Instead of manipulating the command line buffer directly use a readline command with associated data to insert text. This means that text insertion by `commandline --insert` no-longer happens before any `commandline -f` readline functions are executed. The data for the readline function is stored in a discriminated union. Ideally we'd use std::variant but that is not available in C++11 and the C++11 implementations I looked at were all huge and complicated. Using a union requires constructors and assignment operators that initialize the correct union member. Before this change commandline -f backward-word commandline --insert "new text" would insert "new text" at the current cursor position rather than at the beginning of the current word because the text was inserted before the `backward-word` command was executed. This fixes and reinstates the test cases that were broken by the last commit.

Now that `commandline --insert` is just another readline command it is no-longer necessary to special case `begin-undo-group` and `end-undo-group` when they are passed to `commandline -f`. Before this change the `undo` command in commandline -f begin-undo-group commandline -f backward-delete-char backward-delete-char commandline -f end-undo-group undo only undoes one deletion because the `begin-undo-group` and `end-undo-group` commands are executed before the `backwards-delete-char` and `undo` commands. Fixes fish-shell#3031

krobelus · 2022-12-01T09:29:32Z

This is a great step in the right direction, thanks! I left some minor nitpicks.
Nice to see Git folk stop by :)

krobelus · 2022-12-01T09:30:55Z

tests/pexpects/commandline.py

@@ -1,5 +1,6 @@
 #!/usr/bin/env python3
 from pexpect_helper import SpawnedProc, control
+from re import escape


I think we should generally call this re.escape instead of escape

Ok, will do. Thanks for all your comments, sorry it has taken me so long to reply. I'll push a new version in the New Year.

krobelus · 2022-12-01T09:30:56Z

tests/pexpects/commandline.py

+t("abc def", "commandline --current-token --cursor -- -100", "0 abc def")
+t("abc def",
+  "commandline --current-token --cursor 0; commandline --insert x",
+  "5 abc xdef")


the tests are great. One alternative syntax is "abc x%def" instead of "5 abc xdef"

I'm afraid I'm not quite sure what you mean

I mean that instead of passing a separate parameter for the cursor position, we could designate some character to represent the cursor. Of course it's fine as-is

krobelus · 2022-12-01T09:30:56Z

tests/pexpects/commandline.py

+# commandline --insert
+t("abc", "commandline --cursor 2; commandline --insert x", "3 abxc")
+t("abc defghi jkl",
+  "commandline --cursor 8; commandline --current-token --insert x",


Okay, this test shows that --current-token is ignored when --insert is given

krobelus · 2022-12-01T09:30:56Z

tests/pexpects/commandline.py

+t("abc def; ghi", "commandline --append x", "12 abc def; ghix")
+t("abc def ghi",
+  "commandline --cursor 5; commandline --current-token --append x",
+  "5 abc defx ghi")


OTOH --current-token does make a difference with --append. Makes sense.

krobelus · 2022-12-01T09:30:56Z

src/reader.cpp

@@ -138,6 +138,34 @@ static acquired_lock<commandline_state_t> commandline_state_snapshot() {

 commandline_state_t commandline_get_state() { return *commandline_state_snapshot(); }

+void commandline_get_part(const wchar_t *current_buffer, size_t current_cursor_pos,


we should change current_cursor_pos to cursor_pos here.
Similarly, current_buffer should probably be buff like in the functions from parse_util.cpp.

krobelus · 2022-12-01T09:43:15Z

src/input_common.cpp

@@ -270,3 +270,125 @@ void input_event_queue_t::prepare_to_select() {}
 void input_event_queue_t::select_interrupted() {}
 void input_event_queue_t::uvar_change_notified() {}
 input_event_queue_t::~input_event_queue_t() = default;
+
+readline_cmd_t::readline_cmd_t(const readline_cmd_t& rl) : id_(rl.id_) {


we still use old-school pointer alignment (readline_cmd_t &code), clang-format should do the trick

Will do. I think clang-format chooses whatever is most common in the file unless one sets

DerivePointerAlignment: false PointerAlignment: Right ReferenceAlignment: Pointer

in .clang-format

krobelus · 2022-12-01T09:45:06Z

src/input.cpp

@@ -212,12 +216,12 @@ static void input_set_bind_mode(parser_t &parser, const wcstring &bm) {
 }

 /// Returns the arity of a given input function.
-static int input_function_arity(readline_cmd_t function) {
-    switch (function) {


I think this one should take the enum

krobelus · 2022-12-01T09:45:52Z

src/input_common.cpp

+readline_cmd_t::~readline_cmd_t() {
+    // Only destroy arg_.insertion if it is active.
+    if (id_ == id_t::insert_chars) arg_.insertion.~commandline_insertion_t();
+}


this is tricky code but it looks correct.

The fourth commit should perhaps be squashed into the fifth to avoid having to comment out the temporary test failures

I think so too, although it was nice to read this way

I'll squash them together.

krobelus · 2022-12-01T09:46:37Z

src/input_common.h

+
+    union arg_t {
+        /// Set if id_ is set_cursor
+        commandline_cursor_pos_t cursor_pos;


Yeah, this is probably the best solution for now.
Just out of curiosity, here are some possible alternatives to the union:

class readline_cmd_arg_t { virtual ~readline_cmd_arg_t(); }; class readline_cmd_cursor_pos_arg_t : readline_cmd_arg_t { commandline_part_t part; long pos; } struct readline_cmd_t { id_t id_; std::unique_ptr<readline_cmd_arg_t> arg_; }

class readline_cmd_t { virtual ~readline_cmd_arg_t(); }; class readline_cmd_beginning_of_line_t : readline_cmd_t {}; class readline_cmd_cmd_cursor_pos_t : readline_cmd_t { commandline_part_t part; long pos; };

The second is closest to Rust enums but it probably does not make for a good for developer experience in C++.

krobelus · 2022-12-01T09:47:42Z

src/builtins/commandline.cpp

-                    reader_queue_ch(*mc);
-                }
+                // Inserts the readline function at the back of the queue.
+                reader_queue_ch(readline_cmd_t(*mc));


BTW we currently don't use undo groups.

Our plan is to make it so that every key press either
0. odes not modify undo history

adds to an existing undo entry (when adding to the current word)

or creates exactly one new undo entry (which needs the undo group if there are multiple changes)

This way undo groups need not be user visible. That's #7885

faho · 2022-12-01T17:13:38Z

I have a question: Is this the right way around?

Here's how I understand this: Currently, when you do commandline -f something, we enqueue that something in the input queue, but when you do commandline -i foo, we insert that "foo" immediately.

That means, effectively, when you have a script that does commandline -i foo, you can then rely on foo being in the commandline immediately, but when you do commandline -f backward-kill-word, you can't rely on the word being killed until fish gets around to processing the queue that far.

Now this makes it so we put the input action into the queue as well, so now you can't rely on foo being in there immediately.

This should be reproducible with something like

function foo
    commandline -r foo
    commandline >&2
    commandline -r bar
    commandline >&2
end

bind \cg foo

That seems like

It has some potential to break scripts that rely on commandline inputs applying immediately
Commandline changes not applying immediately is kind of annoying to begin with

So I'm not really sure this is the right way around - maybe, instead of adding input actions to the queue as well we should be doing functions immediately? Or possibly drain the queue for every action?

krobelus · 2022-12-03T21:16:51Z

you're right, that should probably be fixed before merging.
There are two things that can enqueue readline events: keyboard input and the commandline builtin.
Both run on the main thread so there is no race.
To fix the coherence I see two options:

always drain the input queue after running commandline (or equivalently, skip the queue)
always drain the input queue before running commandline (if it were to read commandline state)

faho · 2022-12-22T11:31:36Z

Tagging this for after 3.6.0 as we'd prefer to get that out sooner and this would need some testing.

phillipwood · 2022-12-23T15:54:00Z

Tagging this for after 3.6.0 as we'd prefer to get that out sooner and this would need some testing.

When is 3.6.0 due for release? @faho thanks for you comment, I need to think about it before replying properly

faho · 2023-01-14T09:27:09Z

When is 3.6.0 due for release

It's out now.

faho · 2023-03-28T15:25:03Z

Alright, this is conflicting already and the approach needs to be reworked a bit.

We can, of course, revisit it later.

A long standing issue is that bindings cannot mix special input functions and shell commands. For example, bind x end-of-line "commandline -i x" silently does nothing. Instead we have to do lift everything to shell commands bind x "commandline -f end-of-line; commandline -i x" for no good reason. Additionally, there is a weird ordering difference between special input functions and shell commands. Special input functions are pushed into the the queue whereas shell commands are executed immediately. This weird ordering means that the above "bind x" still doesn't work as expected, because "commandline -i" is processed before "end-of-line". Finally, this is all implemented via weird hack to allow recursive use of a mutable reference to the reader state. Fix all of this by processing shell commands the same as both special input functions and regular chars. Hopefully this doesn't break anything. Fixes fish-shell#8186 Fixes fish-shell#10360 Closes fish-shell#9398

Today, bind foo "commandline -f expand-abbr; commandline -i \n" does not work because this 1. enqueues an expand-abbr readline event 2. "commandline -i" inserts \n 3. processes the expand-abbr readline event Since there is no abbreviation on the new line, this doesn't do anything. PR fish-shell#9398 would fix this particular instance however it does not fix the issue that "commandline -i" is run before the expand-abbr is processed by the reader. This is harmless here but there would be a problem if "commandline" tried to read commandline state that was created by a preceding command. It's not super clear to me whether the above binding should work as one would naively expect. That would imply that "commandline" would need to drain all input events (at least all synthetic ones) from the input queue, to ensure it sees the current state. Fortunately the parent commit makes it so if we separate them bind foo "commandline -f expand-abbr" "commandline -i \n" both will be separate events and the commandline state will be synced after each of them. This fixes abbreviation expansion here. Also, we can now mix readline cmds and shell commands, which makes it shorter.

A long standing issue is that bindings cannot mix special input functions and shell commands. For example, bind x end-of-line "commandline -i x" silently does nothing. Instead we have to do lift everything to shell commands bind x "commandline -f end-of-line; commandline -i x" for no good reason. Additionally, there is a weird ordering difference between special input functions and shell commands. Special input functions are pushed into the the queue whereas shell commands are executed immediately. This weird ordering means that the above "bind x" still doesn't work as expected, because "commandline -i" is processed before "end-of-line". Finally, this is all implemented via weird hack to allow recursive use of a mutable reference to the reader state. Fix all of this by processing shell commands the same as both special input functions and regular chars. Hopefully this doesn't break anything. Fixes fish-shell#8186 Fixes fish-shell#10360 Closes fish-shell#9398

Today, bind foo "commandline -f expand-abbr; commandline -i \n" does not work because this 1. enqueues an expand-abbr readline event 2. "commandline -i" inserts \n 3. processes the expand-abbr readline event Since there is no abbreviation on the new line, this doesn't do anything. PR fish-shell#9398 would fix this particular instance however it does not fix the issue that "commandline -i" is run before the expand-abbr is processed by the reader. This is harmless here but there would be a problem if "commandline" tried to read commandline state that was created by a preceding command. It's not super clear to me whether the above binding should work as one would naively expect. That would imply that "commandline" would need to drain all input events (at least all synthetic ones) from the input queue, to ensure it sees the current state. Fortunately the parent commit makes it so if we separate them bind foo "commandline -f expand-abbr" "commandline -i \n" both will be separate events and the commandline state will be synced after each of them. This fixes abbreviation expansion here. Also, we can now mix readline cmds and shell commands, which makes it shorter.

A long standing issue is that bindings cannot mix special input functions and shell commands. For example, bind x end-of-line "commandline -i x" silently does nothing. Instead we have to do lift everything to shell commands bind x "commandline -f end-of-line; commandline -i x" for no good reason. Additionally, there is a weird ordering difference between special input functions and shell commands. Special input functions are pushed into the the queue whereas shell commands are executed immediately. This weird ordering means that the above "bind x" still doesn't work as expected, because "commandline -i" is processed before "end-of-line". Finally, this is all implemented via weird hack to allow recursive use of a mutable reference to the reader state. Fix all of this by processing shell commands the same as both special input functions and regular chars. Hopefully this doesn't break anything. Fixes #8186 Fixes #10360 Closes #9398

Today, bind foo "commandline -f expand-abbr; commandline -i \n" does not work because this 1. enqueues an expand-abbr readline event 2. "commandline -i" inserts \n 3. processes the expand-abbr readline event Since there is no abbreviation on the new line, this doesn't do anything. PR #9398 would fix this particular instance however it does not fix the issue that "commandline -i" is run before the expand-abbr is processed by the reader. This is harmless here but there would be a problem if "commandline" tried to read commandline state that was created by a preceding command. It's not super clear to me whether the above binding should work as one would naively expect. That would imply that "commandline" would need to drain all input events (at least all synthetic ones) from the input queue, to ensure it sees the current state. Fortunately the parent commit makes it so if we separate them bind foo "commandline -f expand-abbr" "commandline -i \n" both will be separate events and the commandline state will be synced after each of them. This fixes abbreviation expansion here. Also, we can now mix readline cmds and shell commands, which makes it shorter.

A long standing issue is that bindings cannot mix special input functions and shell commands. For example, bind x end-of-line "commandline -i x" silently does nothing. Instead we have to do lift everything to shell commands bind x "commandline -f end-of-line; commandline -i x" for no good reason. Additionally, there is a weird ordering difference between special input functions and shell commands. Special input functions are pushed into the the queue whereas shell commands are executed immediately. This weird ordering means that the above "bind x" still doesn't work as expected, because "commandline -i" is processed before "end-of-line". Finally, this is all implemented via weird hack to allow recursive use of a mutable reference to the reader state. Fix all of this by processing shell commands the same as both special input functions and regular chars. Hopefully this doesn't break anything. Fixes fish-shell#8186 Fixes fish-shell#10360 Closes fish-shell#9398

Today, bind foo "commandline -f expand-abbr; commandline -i \n" does not work because this 1. enqueues an expand-abbr readline event 2. "commandline -i" inserts \n 3. processes the expand-abbr readline event Since there is no abbreviation on the new line, this doesn't do anything. PR fish-shell#9398 would fix this particular instance however it does not fix the issue that "commandline -i" is run before the expand-abbr is processed by the reader. This is harmless here but there would be a problem if "commandline" tried to read commandline state that was created by a preceding command. It's not super clear to me whether the above binding should work as one would naively expect. That would imply that "commandline" would need to drain all input events (at least all synthetic ones) from the input queue, to ensure it sees the current state. Fortunately the parent commit makes it so if we separate them bind foo "commandline -f expand-abbr" "commandline -i \n" both will be separate events and the commandline state will be synced after each of them. This fixes abbreviation expansion here. Also, we can now mix readline cmds and shell commands, which makes it shorter.

krobelus · 2024-03-23T13:40:01Z

This should be fix for most practical cases by c3cd68d (Process shell commands from bindings like regular char events, 2024-03-02)

The only limitation is that commands that want to see each other's updates to the commandline need to be passed to bind as separate arguments. So something like

bind x foo bar

should work fine now; foo and bar are executed in this order and commandline updates are read in between

phillipwood added 6 commits November 30, 2022 14:50

add pexpect tests for commandline cursor movement and text insertion

6311a9d

Add some tests in preparation for changing the commandline builtin to use readline functions when moving the cursor and inserting text.

krobelus reviewed Dec 1, 2022

View reviewed changes

faho added this to the fish-future milestone Dec 22, 2022

faho closed this Mar 28, 2023

zanchey removed this from the fish-future milestone Apr 10, 2023

zanchey added this to the will-not-implement milestone May 28, 2023

krobelus mentioned this pull request Mar 3, 2024

Fix Vi mode autosuggestions and others #10338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builtin commandline: use readline #9398

builtin commandline: use readline #9398

phillipwood commented Nov 30, 2022

krobelus commented Dec 1, 2022 •

edited

krobelus Dec 1, 2022

phillipwood Dec 23, 2022

krobelus Dec 1, 2022

phillipwood Dec 23, 2022

krobelus Jan 4, 2023

krobelus Dec 1, 2022

krobelus Dec 1, 2022

krobelus Dec 1, 2022

krobelus Dec 1, 2022

phillipwood Dec 23, 2022

krobelus Dec 1, 2022

krobelus Dec 1, 2022

phillipwood Dec 23, 2022

krobelus Dec 1, 2022

krobelus Dec 1, 2022

faho commented Dec 1, 2022

krobelus commented Dec 3, 2022 •

edited

faho commented Dec 22, 2022

phillipwood commented Dec 23, 2022

faho commented Jan 14, 2023

faho commented Mar 28, 2023

krobelus commented Mar 23, 2024

		@@ -138,6 +138,34 @@ static acquired_lock<commandline_state_t> commandline_state_snapshot() {

		commandline_state_t commandline_get_state() { return *commandline_state_snapshot(); }

		void commandline_get_part(const wchar_t *current_buffer, size_t current_cursor_pos,

builtin commandline: use readline #9398

builtin commandline: use readline #9398

Conversation

phillipwood commented Nov 30, 2022

Description

Some notes on the commits:

TODOs:

krobelus commented Dec 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faho commented Dec 1, 2022

krobelus commented Dec 3, 2022 • edited

faho commented Dec 22, 2022

phillipwood commented Dec 23, 2022

faho commented Jan 14, 2023

faho commented Mar 28, 2023

krobelus commented Mar 23, 2024

krobelus commented Dec 1, 2022 •

edited

krobelus commented Dec 3, 2022 •

edited