Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

paste: redesign, fixes, 10x speedup #4448

Merged
merged 30 commits into from Aug 27, 2019
Merged

paste: redesign, fixes, 10x speedup #4448

merged 30 commits into from Aug 27, 2019

Conversation

@justinmk
Copy link
Member

justinmk commented Mar 14, 2016

This PR improves paste behavior and performance.

  • Performance: 10x-100x faster (try pasting MB of input, or paste a lot of text with filetype=cpp or filetype=ruby)
  • Extensibility: users can control paste behavior by redefining vim.paste (Lua)
  • API: nvim_paste for UIs/clients
  • API: nvim_put inserts text (weird but true: this was difficult before)
  • Fixes:

Tasks:

  • implement as callback instead of pre/post autocmds
  • Indicate paste start/stop as parameter to handler?
  • generalize string_to_array() ?
  • API: nvim_put #6819
  • API: nvim_paste
  • redo (dot-repeat)
  • 'nomodifiable' buffer
  • documentation

TODO (future):

  • Replace/Operator-pending/Visual modes
  • remove 'pastetoggle' p_pt
  • Dot-repeat is slow (same behavior in Vim) because it is subject to Vim auto-indent/etc.
  • Mechanism for associating a callback with a nvim_input chunk. #10826
  • TextPutPre/TextPutPost (analogs for TextYankPost) ?
  • Send paste sequences to :terminal
  • handle missing "stop paste" code (how? timeout...?) #10865
@marvim marvim added the WIP label Mar 14, 2016
@justinmk justinmk force-pushed the justinmk:paste branch 2 times, most recently from c9b85ab to 8898ee5 Mar 14, 2016
@@ -3435,11 +3435,11 @@ getchar([expr]) *getchar()*
:endfunction
<
You may also receive synthetic characters, such as
|<CursorHold>|. Often you will want to ignore this and get
|<LeftMouse>|. Often you will want to ignore this and get

This comment was marked as resolved.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

<CursorHold> pseudokey doesn't exist any more (replaced by K_EVENT).

This comment was marked as resolved.

Copy link
@bfredl

bfredl Aug 20, 2019

Member

<LeftMouse> is user input, just like <F10> or any other special key without canonical unicode representation, of which there are many. Seems weird to ignore only it specifically. CursorHold needed extra treatment here because it was generated by nvim without user intervention so getchar() terminated early, other special chars do not have this problem.

This comment was marked as resolved.

Copy link
@justinmk

justinmk Aug 20, 2019

Author Member

Good point. Maybe we can remove this blurb from the docs?

This comment was marked as resolved.

Copy link
@bfredl

bfredl Aug 20, 2019

Member

I think so. The concrete examples above of how to actually handle pseudokeys (including mouse) should be enough.

// didn't match ui_toggle_key and didn't try the whole typebuf,
// check the 'pastetoggle'
match = typebuf_match_len(p_pt, &mlen);
}

This comment has been minimized.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

Now we don't need this special-case logic in this low-level function...

@@ -233,9 +237,12 @@ static void tk_getkeys(TermInput *input, bool force)
}
}

if (result != TERMKEY_RES_AGAIN || input->paste_enabled) {
if (result != TERMKEY_RES_AGAIN /*|| input->paste_started */) {

This comment has been minimized.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

@tarruda Is this skip just an optimization?

This comment has been minimized.

Copy link
@tarruda

tarruda Mar 14, 2016

Member

No, but this probably isn't needed anymore(I don't think it is possible to have result == TERMKEY_RES_AGAIN && input->paste_enabled because read_cb will only push data up to the next \x1b(esc). I'm not sure why this is there, probably an oversight or leftover from previous code.

if (enable) {
loop_schedule(&loop, event_create(1, apply_pastepre, 0));
} else {
enqueue_input(input, PASTEPOST_KEY, sizeof(PASTEPOST_KEY) - 1);

This comment has been minimized.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

@tarruda / @oni-link, Wondering if you can help my understanding. loop_schedule() on the "start paste" case (line 306) works well, because it puts the event on the main queue before any input gets processed. But for the "stop paste" case, I had to enqueue this PASTEPOST_KEY in order to guarantee that all paste data was processed before queuing the PastePost event.

Do we currently have any better mechanism for placing an event at a precise location in the input stream (I'm guessing the answer is "no", hence why we still have these pseudokeys)? I also tried using a mutex, but that had problems and isn't really desirable anyways.

I am wondering if it would be a good idea to provide some internal generic mechanism for placing K_EVENT in the input stream and associating it with a callback.

This comment has been minimized.

Copy link
@tarruda

tarruda Mar 14, 2016

Member

Do we currently have any better mechanism for placing an event at a precise location in the input stream (I'm guessing the answer is "no", hence why we still have these pseudokeys)

No we don't.

I am wondering if it would be a good idea to provide some internal generic mechanism for placing K_EVENT in the input stream and associating it with a callback.

K_EVENT is not meant to be used by UIs, it is only a general mechanism for waking nvim when an asynchronous event happens. An UI placing a bunch of keys in the input stream is an asynchronous event, but not "certain key gets processed"(And the only use case I could think for this is toggling paste, so I'm not sure it would be useful)

This comment has been minimized.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

"certain key gets processed"(And the only use case I could think for this is toggling paste, so I'm not sure it would be useful)

@tarruda So does that mean we can safely use loop_schedule() to schedule FocusGained/FocusLost instead of the K_FOCUSGAINED/K_FOCUSLOST?

This comment has been minimized.

Copy link
@tarruda

tarruda Mar 14, 2016

Member

@tarruda So does that mean we can safely use loop_schedule() to schedule FocusGained/FocusLost instead of the K_FOCUSGAINED/K_FOCUSLOST?

Possibly yes. I may be missing something, but FocusGained/FocusLost doesn't seem to be events that needs to be handled at an exact point during input processing.

// Make sure the next input escape sequence fits into the ring buffer
// without wrap around, otherwise it could be misinterpreted.
// CSI and K_SPECIAL are double-escaped later (input_enqueue).
// Make sure they can fit into the ring buffer without wrap around.
rbuffer_reset(input->read_stream.buffer);

This comment was marked as resolved.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

@tarruda @oni-link I tried to clarify this a bit. Is the updated comment accurate?

This comment was marked as resolved.

Copy link
@tarruda

tarruda Mar 14, 2016

Member

No, input->read_stream.buffer is only read by the loop above. There could be an multi-byte key sequence that is trimmed by the end of the buffer(wrapping around to the beginning), but termkey_push_bytes is not aware of that, so it could misinterpret the key. rbuffer_reset simply moves the data so it is all in contiguous memory.

This comment was marked as resolved.

Copy link
@justinmk

justinmk Mar 14, 2016

Author Member

Oh, I get it: this problem occurs because of rbuffer_read_ptr() exposing the raw buffer to callers (defeating the ring-buffer abstraction).

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Mar 14, 2016

@justinmk I'm not sure I understand the goal behind this PR, is it to decouple the logic of automatically toggling paste with a special key?

If so, I don't see much gain here: We still have bracketed paste parsing hardcoded into the TUI, only the set paste logic will be moved to vimscript(even so, what would be the purpose of allowing autocmds to be attached to paste?). But I do see a worthly goal in this:

I am wondering if it would be a good idea to provide some internal generic mechanism for placing K_EVENT in the input stream and associating it with a callback.

If you could implement this, then yes, we could completely decouple bracketed paste(and possibly many other terminal sequences) from the tui. But it should not be K_EVENT, since it is already used for another purpose(and no, we can't place arbitrary events so they are processed at specific times during input processing).

However, this seems to overlap with @bfredl's work on #4419 :

map <command> <Paste> if &paste | set nopaste | else | set paste

Maybe it would be even better if we simply supported user-defined synthetic keys:

map <command> <User:SOME_KEY_ID> if &paste | set nopaste | else | set paste

The question is: How can a plugin tell the UI to put such synthetic keys into the input buffer? Right now we can't, but the TUI will soon allow configuration, so a bracketed paste plugin could be summarized to something like this:

let g:tui_keys_override = {
\  '\x1b[200~': '<User:Paste>'
\  '\x1b[201~': '<User:Paste>'
\}
imap <command> <User:Paste> if &paste | set nopaste | else | set paste

That is, the plugin asks the tui to translate bracketed pastes into <user:paste>, and maps it to a code that toggles the paste option.

@@ -328,6 +326,9 @@ int main(int argc, char **argv)
"'\\c\\m" PROTO "\\(.\\{-}\\)//'), 1, '')})");
#undef PROTO

do_cmdline_cmd("autocmd PastePre * set paste|if mode()!~#'c\\|t'|call feedkeys('\x1C\xEi','n')|endif");
do_cmdline_cmd("autocmd PastePost * set nopaste|if mode()!~#'c\\|t'|call feedkeys('\x1C\xE','n')|endif");

This comment has been minimized.

Copy link
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

\xE looks weird, better write \x0E.

This comment has been minimized.

Copy link
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

Though no, it is better to write this as \"\\<C-\\>\\<C-n>\".

This comment has been minimized.

Copy link
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

And I would also suggest to create autocmd group __Builtin for all built-in autocommands (currently only this one and term://* BufReadCmd), so that they are easy to identify, making user aware of what he sees and, possibly, changes.

This comment has been minimized.

Copy link
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

Regex is better written as stridx('ct', mode()) == -1 or mode() !~# '[ct]'.

@justinmk

This comment was marked as resolved.

Copy link
Member Author

justinmk commented Mar 14, 2016

If so, I don't see much gain here:

This fixes several bugs, and removes some special-case logic.

We still have bracketed paste parsing hardcoded into the TUI, only the set paste logic will be moved to vimscript

Hard-coded handling of bracketed paste belongs in tui/input.c--that's not a problem. And this PR removes inspection of ui_toggle[] in vgetorpeek(). That's a net gain.

(even so, what would be the purpose of allowing autocmds to be attached to paste?).

I addressed that here: #3906 (comment) I'll also add that some users would want to control whether they return to normal-mode after a paste, or whether a newline is always created.

Moreover, @bfredl mentioned wanting Paste event as the dual for TextYankPost.

However, this seems to overlap with @bfredl's work on #4419 :

I don't think so. The <Paste> toggle does not work. It's broken. The system needs to know "this is the start" and "this is the end". The state of 'paste' cannot be a factor in this, the user may already have it enabled.

The old code was sending <Paste> to toggle on and another one later in the stream to toggle off. This is the cause of bugs we've had for 6 months.

@bfredl

This comment was marked as resolved.

Copy link
Member

bfredl commented Mar 14, 2016

TextPutPre(/Post ?) and PastePre/PastePost seems to me to be completely different events.The former would handle putting, p (and maybe i<c-r> and :<c-r> as well) while the event here is for terminal bracketed paste.

@justinmk

This comment was marked as resolved.

Copy link
Member Author

justinmk commented Mar 14, 2016

@bfredl See the PR description where I mention that blackhole register could be used to mean "external paste" (called "bracketed paste" in terminals, but for all other UIs it's a "clipboard paste").

IOW, though this PR is narrow at the moment, it's just a first step (which fixes very old bugs).

I can rename the events to TextPutPre and TextPutPost.

@bfredl

This comment was marked as outdated.

Copy link
Member

bfredl commented Mar 14, 2016

called "bracketed paste" in terminals, but for all other UIs it's a "clipboard paste"

Not really, "+p or "*p (or the insert mode variants) would be the closest nvim notion of a UI clipboard paste. The difference is that terminal paste (at least before this PR?) doesn't do any putting operation, rather it changes behavior of insert mode to insert typed text verbatim. (but it doesn't for instance switch to insert mode automatically)

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

@justinmk Bracketed paste is terminal-UI-specific thing. GUIs do not have anything like this, all they have are events from user: clipboard and mouse. There is no “external paste”, there are things like "+p, :put and events which map to this. In any case clipboard contents is received in one piece, it is not a stream like {paste start}{contents}{paste end}.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

@justinmk <MiddleMouse>, <C-v>, etc are handled by the application itself and it is UI which initiates pasting upon receiving an appropriate event. There are conventions and toolkits which follow conventions by default, but they are not enforced and toolkits can be told to not follow them as well. With bracketed paste it is terminal which initiates pasting, which is an “external” application.

@bfredl

This comment was marked as resolved.

Copy link
Member

bfredl commented Mar 14, 2016

That two different operations sometimes can have the the same goal does not make them the same operation, I don't see any pedantry in that.
In TextPutPre the autocmd could be allowed to change the text and the register type. What would that even mean for PastePre. (The oprational sematics are the relevant here and not any naming)

@ZyX-I

This comment was marked as resolved.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

@justinmk It is only TUI which has to “raise PastePre, send bytes, PastePost” (and “has” is mostly because Neovim needs to be prepared to missing PastePost). Other UIs may say “paste this”, but they don’t need this at all, there is no “external” paste, nobody will raise PastePre, PastePost and send bytes because pasting is done by "*p or equivalents. The only thing is that some UIs may want to alter clipboard providers.

When “Paste*” is used for handling pastes from register a semantics changes significantly: previously input was put into an input queue. Now input is located in register a and nobody wants it in the input queue.

@justinmk

This comment was marked as resolved.

Copy link
Member Author

justinmk commented Mar 14, 2016

In TextYankPre the autocmd could be allowed to change the text and the register mode. What would that even mean for PastePre

Not sure I follow. We increment textlock for TextYankPost. We can set textlock for Paste events as well, regardless of what we do for TextYankPre.

Other UIs may say “paste this”, but they don’t need this at all, there is no “external” paste, nobody will raise PastePre, PastePost and send bytes because pasting is done by "*p or equivalents.

Why couldn't we recommend that UIs raise the events? This would enable users to have consistent paste behavior for any UI. E.g., some users may want to return to normal-mode always after a paste. Some users may want to respond to a non-modifiable buffer by opening a new tab instead of just showing an error.

When “Paste*” is used for handling pastes from register a semantics changes significantly: previously input was put into an input queue. Now input is located in register a and nobody wants it in the input queue.

Fair point, then we can drop that idea. It's not part of this PR.

@bfredl

This comment was marked as resolved.

Copy link
Member

bfredl commented Mar 14, 2016

Sorry meant TextPutPre: It could change the text/type pair being pasted but that doesn't make any sense for PastePre.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

By the way, consider that user pressed <S-Insert>foo, which translated into {paste start}{inserted contents}{paste end}foo. Can I use Paste* events to get {inserted contents} without foo?

Also I saw that you test ignoring second {paste start} in {paste start}{paste start}. This is incorrect, I checked konsole using

echo $'\e[200~' | xclip -i
echo $'\e[?2004h' ; stty -echo ; cat -v

(with <MiddleMouse> after last <CR>) and got

^[[200~^[[200~
^[[201~

: no escaping. Spurious {paste start} should be part of the pasted text, not ignored. Double {paste end}/{paste end} without {paste start} should be ignored though I think.

@justinmk

This comment has been minimized.

Copy link
Member Author

justinmk commented Mar 14, 2016

Can I use Paste* events to get {inserted contents} without foo?

The text is not provided in any register or v: variable, but the buffer contents at time of PastePost should not contain foo. And you could compare this to buffer contents at time of PastePre.

Spurious {paste start} should be part of the pasted text, not ignored. Double {paste end}/{paste end} without {paste start} should be ignored though I think.

Thanks, I will fix it.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Mar 14, 2016

I don't think so. The toggle does not work. It's broken. The system needs to know "this is the start" and "this is the end". The state of 'paste' cannot be a factor in this, the user may already have it enabled.

The old code was sending to toggle on and another one later in the stream to toggle off. This is the cause of bugs we've had for 6 months.

Alright. What about your suggestion of adding a generic mechanism to allow execution of vimscript when a certain synthetic keys are processed? Assuming the TUI allows the user to override terminal code parsing(it will), a bracketed paste plugin could do this:

let g:tui_keys_override = {
\  '\x1b[200~': '<User:PasteEnable>'
\  '\x1b[201~': '<User:PasteDisable>'
\}

It seems like something that could work. Even FocusGain/FocusLost could work nicely on such infrastructure.

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

I also do not see tests for pasting while in all other modes (visual, select, operator-pending, replace, virtual replace).

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Mar 14, 2016

Adapting my previous comment to @bfredl's idea, here's how bracketed paste would be implemented as a plugin:

let g:tui_keys_override = {
\  '\x1b[200~': '<Cmd>set paste<cr>'
\  '\x1b[201~': '<Cmd>set nopaste<cr>'
\}
@justinmk

This comment has been minimized.

Copy link
Member Author

justinmk commented Mar 14, 2016

Assuming the TUI allows the user to override terminal code parsing(it will), a bracketed paste plugin could do this:
let g:tui_keys_override

I suppose that would be fine if no one is convinced that PastePre/PastePost will be useful for GUIs (I gave use-cases above). But:

  • It will block this fix for a long time. Paste has been broken for more than 6 months.
  • It means we're back to asking users to fiddle with termcodes--even when the work was already done to abstract it for certain cases.
  • It won't apply to GUIs, so configuration of this behavior won't be consistent across UIs.
@bfredl

This comment has been minimized.

Copy link
Member

bfredl commented Mar 14, 2016

Hmm I wonder how to implement "Send paste sequences to :terminal" with that. Is the remapping done by the tui module or do you mean runtime will add those mappings to insert mode?

@ZyX-I

This comment has been minimized.

Copy link
Contributor

ZyX-I commented Mar 14, 2016

@bfredl Neither of suggested variants allow implementing “send paste sequences to :terminal”, because this needs hooking "*p. Though bracket pasting to terminal may be implemented based on PastePre/PastePost, this also needs getting terminal state.

justinmk added 10 commits Aug 19, 2019
This is "readfile()-style", see also ":help channel-lines".
Fixes strange behavior where sometimes the buffer contents of a series
of paste chunks (vim._paste) would be out-of-order.

Now the tui_spec.lua screen-tests are much more reliable. But they still
sometimes fail because of off-by-one cursor (caused by "typeahead race"
resulting in wrong mode; fixed later in this patch-series).
- Send `phase` parameter to the paste handler.
- Redraw at intervals and when paste terminates.
- Show "..." throbber during paste to indicate activity.
Workaround this failure:

    [  ERROR   ] test/functional/terminal/tui_spec.lua @ 192: TUI paste: exactly 64 bytes
    test/functional/helpers.lua:403:
    retry() attempts: 478
    test/functional/terminal/tui_spec.lua:201: Expected objects to be the same.
    Passed in:
    (table: 0x47cd77e8) {
     *[1] = 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz endz' }
    Expected:
    (table: 0x47cd7830) {
     *[1] = 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz end' }

This happens because `curwin->w_cursor.col` is sometimes decremented at
the end of `do_put`... because the editor is in Normal-mode instead of
the expected Insert-mode.

Caused by "typeahead race" (#10826): there may be queued input in the
main thread not yet processed, thus the editor mode (`State` global)
will be "wrong" during paste. Example: input "i" followed immediately by
a paste sequence:

    i<start-paste>...<stop-paste>
    ^
     "i" does not get processed in time, so the editor is in
     Normal-mode instead of Insert-mode while handling the paste.

Attempted workarounds:
- vim.api.nvim_feedkeys('','x',false) in vim._paste()
- exec_normal() in tinput_wait_enqueue()
- LOOP_PROCESS_EVENTS(&main_loop,…,0) in tinput_wait_enqueue()

ref #10826
@justinmk justinmk force-pushed the justinmk:paste branch from 7dc335b to ce155b4 Aug 27, 2019
justinmk added 6 commits Aug 24, 2019
- Normal-mode redo idiom(?): prepend "i" and append ESC.
- Insert-mode only needs AppendToRedobuffLit().
- Cmdline-mode: only paste the first line.
HACK: The cursor does not get repositioned after the paste completes.
Scheduling a dummy event seems to fix it.

Test case:
0. Revert this commit.
1. Paste some text in Normal-mode.
2. Notice the cursor is still in the cmdline area.
- Show error only once per "paste stream".
- Drain remaining chunks until phase=3.
- Lay groundwork for "cancel".
- Constrain semantics of "cancel" to mean "client must stop"; it is
  unrelated to presence of error(s).
- nvim_paste(): Marshal through luaeval() instead of nvim_execute_lua()
  because the latter seems to hide some errors.
- Handle 'nomodifiable' in `nvim_put()` explicitly.
- Require explicit `false` from `vim.paste()` in order to "cancel",
  otherwise assume true ("continue").
@justinmk justinmk force-pushed the justinmk:paste branch from 34d1989 to 2fd3b6c Aug 27, 2019
@justinmk

This comment has been minimized.

Copy link
Member Author

justinmk commented Aug 27, 2019

Resolution/Summary

vim-patch:8.1.1198
vim-patch:8.1.0224
vim-patch:8.0.1299
vim-patch:8.0.0569
vim-patch:8.0.0303
vim-patch:8.0.0296
vim-patch:8.0.0244
vim-patch:8.0.0238
vim-patch:8.0.0232
vim-patch:8.0.0231
vim-patch:8.0.0230
vim-patch:8.0.0210

Notes:

  • cursor is hidden after paste completes. Workaround: send dummy event 4344ac1:
    loop_schedule_deferred(&main_loop, event_create(x_dummy_event, 0));
    

Locked to keep the summary visible. You can always chat or open a ticket if you have new information/topics to discuss.

@justinmk justinmk force-pushed the justinmk:paste branch 2 times, most recently from 99a1753 to fee2e1c Aug 27, 2019
- Introduce TRY_WRAP() until we have an *architectural* solution.
  - TODO: bfredl idea: prepare error-handling at "top level" (nv_event).
- nvim_paste(): Revert luaeval() hack (see parent commit).
  - With TRY_WRAP() in nvim_put(), 'nomodifiable' error now correctly
    "bubbles up".
@justinmk justinmk force-pushed the justinmk:paste branch from fee2e1c to 3157bae Aug 27, 2019
@justinmk justinmk merged commit 82d52b2 into neovim:master Aug 27, 2019
6 checks passed
6 checks passed
QuickBuild pr-4448: build finished with status success
Details
builds.sr.ht: openbsd.yml builds.sr.ht job completed successfully
Details
codecov/patch 81.5% of diff hit (target 74.77%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +6.73% compared to 09cbd67
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justinmk justinmk deleted the justinmk:paste branch Aug 27, 2019
@neovim neovim locked as resolved and limited conversation to collaborators Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.