Skip to content
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

Add self-insert-nonempty as fast implementation of space-trimming #6713

Closed
wants to merge 5 commits into from

Conversation

ridiculousfish
Copy link
Member

(Note if we merge this, it will be into 3.1.1 as well as master.)

This implements the idea I sketched in this comment. It adds a new readline command self-insert-nonempty which inserts characters unless the command line is empty.

The space-trim-on-paste binding then becomes bind --preset -M paste " " self-insert-nonempty and it is quite noticeably faster.

For 3.1.1, I am OK merging this or reverting a5a643f. Post if you have strong feelings either way.

@faho
Copy link
Member

faho commented Mar 5, 2020

This should be documented in cmds/bind.rst.

src/reader.cpp Outdated
arr[i] = next_event.get_char();
} else if (evt.input_style == char_event_t::style_nonempty && accumulated_chars.empty() &&
active_edit_line()->empty()) {
// Skip this ltrim character.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if I understand this correctly: this bit is just an optimisation?

We can skip all normal chars on an empty commandline if the binding used is self-insert-nonempty. If anything else modifies the commandline, we won't be going through this path, and then once we do go back here the commandline might not be empty anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is just an optimization, but an important one . This loop accumulates normal input, while there's input available. If we break out of the loop, we end up redrawing (and redrawing on every space would be expensive).

Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good as far as I can tell

@@ -111,6 +112,17 @@ class char_event_t {
/// The type of event.
char_event_type_t type;

/// Hackish: the input style, which describes how char events (only) are applied to the command
/// line. Note this is set only after applying bindings; it is not set from readb().
enum input_style_t : uint8_t {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised for a bit because the enum is called input_style_t but it's used elsewhere using char_event_t.
It's obvious after a second look, but maybe it's better to make it a top-level enum class like the others above, since it needs to be public anyways.

src/reader.cpp Outdated Show resolved Hide resolved
This adds a new readline command self-insert-notfirst, which is
analogous to self-insert, except that it does nothing if the cursor
is at the beginning. This will serve as a higher-performance implementation
for stripping leading spaces on paste.
This adds basic support for self-insert-notfirst. When we see a
self-insert-nonempty char event, we kick it back to the outer loop,
which only inserts the character if the cursor is not at the beginning.
This changes a5a643f to use the new self-insert-notfirst binding.
It also adds a test.
This teaches the reader fast-path to use self-insert-notfirst, allowing
it to handle spaces. This greatly increases the performance of paste by
reducing redraws.

Fixes fish-shell#6603. Somewhat improves fish-shell#6704
@ridiculousfish ridiculousfish deleted the ltrim branch March 7, 2020 21:33
ridiculousfish added a commit that referenced this pull request Mar 7, 2020
ridiculousfish added a commit that referenced this pull request Mar 7, 2020
ridiculousfish added a commit that referenced this pull request Mar 7, 2020
This is the 3.1.1 set of changes to improve pasting behavior.

cherry-picked from #6713
@ridiculousfish
Copy link
Member Author

Merged into master as e3e82f5 and Integration_3.1.1 as 4c30e5a

@ridiculousfish ridiculousfish added this to the fish 3.1.1 milestone Mar 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2020
@fish-shell fish-shell unlocked this conversation Dec 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants