[RFC] Incsub 4 (renamed to 'inccommand') #5561

Merged
merged 16 commits into from Nov 9, 2016

Projects

None yet

9 participants

@justinmk
Member
justinmk commented Nov 3, 2016 edited
  • rename to 'inccommand'
  • back out special cases in do_ecmd() and buflist_new()
  • reduce shared/global state
  • preserve b:changedtick #5561 (comment)
  • add tests for feedkeys(':%s/foo/bar/'), feedkeys(":%s/foo/bar/\<CR>"), ...
    • check KeyTyped to avoid preview-mode during feedkeys()
  • should preview split appear if :{range} is in the viewport? (answer: yes, be predictable)
  • disable 'cursorline', 'spell' in preview window
  • set buffer name to [Preview] instead of [inc_sub]
  • change [lnum] display to |lnum| (like quickfix window)
  • highlight/multiline bug #5226 (comment)
  • re-use window id for preview buffer (:h win_getid())
    • best approach here would be to keep the window open and close it when command ends
  • should NOT change 'modified' (punting this for now; too risky)
  • ctrl-p/ctrl-n/up/down in : should not trigger 'inccommand' until user edits the command
    • going to wait for feedback before doing this

Renamed 'incsubstitute' to 'inccommand'

So that we can expand it to other commands besides :substitute without adding new options (that was already implied in the implementation, re eap->is_live).

Future considerations

  • block RPCs/events during incremental/live preview (eap->is_live)?
    • the buffer is in a weird state during the preview, don't want plugins interacting with it
  • instead of u_undo_and_forget(), it might be more robust to create a special/dummy buffer and throw it away
    • crazy talk: to show live previews of multi-buffer commands like :argdo, :cdo, overlay a new, temporary instance of nvim?
KillTheMule added some commits Aug 12, 2016
@KillTheMule KillTheMule Incsubsitution feature
Originally implemented by

* Clement0
* DesbyP
* aym7
* Adrey06
* Robinhola

in #4811. Major reworkings and bug
fixes by

* bfredl

Most tests suggested by ZyX-l, suggestions for improvements by oni-link.
13841a5
@KillTheMule KillTheMule Tests for incsubstitution feature 561c1e3
@KillTheMule KillTheMule Linted
e8c0f90
@marvim marvim added the WIP label Nov 3, 2016
src/nvim/buffer.c
- } else { // create new
- (void)do_ecmd((int)bufnr, NULL, NULL, NULL, ECMD_ONE, ECMD_HIDE, NULL);
- }
+ (void)do_ecmd((int)bufnr, NULL, NULL, NULL, ECMD_ONE, ECMD_HIDE, NULL);
@justinmk
justinmk Nov 5, 2016 Member

This seems to work fine when combined with setlocal bufhidden=hide to prevent unloading the buffer.

The switch_to_win_for_buf() approach caused a double-free and other invalid access pain arising from some unknown interaction where the fragile order-of-operations for buffer creation/re-use falls apart.

src/nvim/ex_cmds.c
emsg_off--;
+ if (save_changedtick != curbuf->b_changedtick
+ && !u_undo_and_forget(1)) {
@justinmk
justinmk Nov 5, 2016 Member

u_undo_and_forget() does not restore b:changedtick, which is used by some plugins like repeat.vim. @bfredl @KillTheMule should we restore it in u_undo_and_forget() or would it not be appropriate there?

@bfredl
bfredl Nov 5, 2016 Member

No, undo_and_forget should work in a branch that's not the latest, and so can't manage changedtick. ex_substitute should restore it.

@justinmk
Member
justinmk commented Nov 5, 2016

Only 2 tests failing, and valgrind seems OK.

@justinmk justinmk changed the title from [WIP] Incsub 4 to [RFC] Incsub 4 Nov 5, 2016
@marvim marvim added RFC and removed WIP labels Nov 5, 2016
@KillTheMule
Contributor

add tests for feedkeys(':%s/foo/bar/'), feedkeys(":%s/foo/bar/"), ...

If you could elaborate on this (I'm not sure what you mean), I could see if I can write them.

@justinmk
Member
justinmk commented Nov 6, 2016

@KillTheMule Generally, to verify that "live" substitution (or any "live" handling in general) should not be invoked during anything except actual interactive : use.

@KillTheMule
Contributor

Ok we have some basics: 561c1e3#diff-4dbe255ebfc94f6664c66ffeab750809R83

You mean along the lines of 561c1e3#diff-4dbe255ebfc94f6664c66ffeab750809R818, just with incsubstute=''? Note sure what to do about any "live" handling in general though.

@justinmk
Member
justinmk commented Nov 6, 2016 edited

Ok we have some basics: 561c1e3#diff-4dbe255ebfc94f6664c66ffeab750809R83

Confusingly, the helpers.feed() method is not equivalent to VimL feedkeys(). helpers.feed() simulates actual user input. Whereas I would like to verify that if plugins call :substitute or feedkeys(':s...'), incsub should not be invoked. Easy way to verify this is to check if the incsub buffer exists after either of those scenarios. (With this PR, the incsub buffer hangs around as an unlisted buffer.)

@justinmk
Member
justinmk commented Nov 6, 2016

Woo, ASAN looks good. Going to wrap this up now.

@KillTheMule
Contributor

highlight bug #5226 (comment)

Note that this is a highlighting bug for single-line substitutions containing escaped chars, but makes the whole thing totally break down with multiline matches.

@justinmk
Member
justinmk commented Nov 6, 2016 edited

Damn, that's disappointing. I would guess that multiline searches aren't common for many users, but it's still a bad bug.

update: Multiline matches seem to work mostly ok, actually.

@KillTheMule
Contributor

It still works as a substitution, just the preview is borked :) But it also means the data structure needs to be reworked, because right now it can only save one line that contains the whole match.

+ end
+ end)
+
+ it('work then mappings move the cursor', function()
@KillTheMule
KillTheMule Nov 6, 2016 Contributor

Small typo there: then -> when

@justinmk justinmk changed the title from [RFC] Incsub 4 to [RFC] Incsub 4 (renamed to 'inccommand') Nov 7, 2016
+ it("when invoked by feedkeys() in a script ", function()
+ source(':call feedkeys(":%s/tw/MO/g\\<CR>")')
+ wait()
+ eq(1, eval("bufnr('$')"))
@KillTheMule
KillTheMule Nov 7, 2016 Contributor

Are you sure this checks what we want? By putting \<CR> at the end of the source, the command would finish, so the preview window is supposed to be closed at this point anyways. Maybe it's useful to check for it, but you can't be sure it didn't show inbetween, can you?

@justinmk
Member
justinmk commented Nov 7, 2016 edited

Are you sure this checks what we want? By putting <CR> at the end of the source, the command would finish, so the preview window is supposed to be closed at this point anyways.

@KillTheMule The compromise made in this PR to avoid the special cases added to buflist_new() and do_ecmd() , is to keep the buffer around as an unlisted buffer. (That's more or less idiomatic, and less prone to bugs.)

Checking bufnr('$') is sufficient because bufnr('$') includes unlisted buffers. Note that before 48a986c (which checks KeyTyped) the test correctly fails.

@justinmk
Member
justinmk commented Nov 7, 2016 edited

@bfredl @KillTheMule I added a new test which compares undotree() before and after a canceled :sub preview. Here's the failure:

:substitute, 'inccommand' preserves undotree()
test/functional/ui/incsubstitute_spec.lua:164: Expected objects to be the same.
Passed in:
(table) {
 *[entries] = {
    [1] = {
      [seq] = 1
      [time] = 1478533735 }
    [2] = {
      [alt] = { ... more }
      [seq] = 3
      [time] = 1478533735 }
    [3] = {
      [seq] = 4
      [time] = 1478533735 }
   *[4] = {
     *[newhead] = 1
      [seq] = 5
      [time] = 1478533735 }
    [5] = {
      [curhead] = 1
      [seq] = 6
      [time] = 1478533735 }
    [6] = {
      [seq] = 7
      [time] = 1478533735 } }
  [save_cur] = 0
  [save_last] = 0
  [seq_cur] = 5
  [seq_last] = 7
  [synced] = 1
  [time_cur] = 1478533735 }
Expected:
(table) {
 *[entries] = {
    [1] = {
      [seq] = 1
      [time] = 1478533735 }
    [2] = {
      [alt] = { ... more }
      [seq] = 3
      [time] = 1478533735 }
    [3] = {
      [seq] = 4
      [time] = 1478533735 }
   *[4] = {
      [seq] = 5
      [time] = 1478533735 }
    [5] = {
      [curhead] = 1
      [seq] = 6
      [time] = 1478533735 }
    [6] = {
      [newhead] = 1
      [seq] = 7
      [time] = 1478533735 } }
  [save_cur] = 0
  [save_last] = 0
  [seq_cur] = 5
  [seq_last] = 7
  [synced] = 1
  [time_cur] = 1478533735 }

The difference is that [newhead] = 1 on entry 6 (expected) vs entry entry 4 (actual). I don't understand u_undo_and_forget() at all, so I'm hoping you have some insight. Is this expected?

@KillTheMule
Contributor
KillTheMule commented Nov 8, 2016 edited

I don't fully understand it either, but I think this is not expected. Could you rerun the test with feed([[:%s/e<C-\><C-N>]]) (or if this does not fail, add keys til the first failure to produce a minimal example)? I'd have a shot at going through the logic then.

(I'm not at home and can't run anything here other than the web browser, that's why I'm asking :))

@bfredl
Member
bfredl commented Nov 8, 2016

Here is the thing. Whenever "curhead" points to a valid head, the value of "newhead" is meaningless (and really should be set to null). The undo state for a buffer is logically the enum:

enum UndoState {
    CurrentHead(head),
    NewHead(head),
    EmptyTree
}

nvim represents this as, whenever curbuf->b_u_curhead is nonnull it should be used as the current head, and curbuf->b_u_newhead is ignored.If the there is a current head, then this will be redoed on the next redo, and its parent will be undone on next undo.
Only if b_u_curhead is NULL, b_u_newhead will be used as the head to undo (and it is not possible to redo). Also both can be NULL, to indicate an empty undotree. (To be fair, this only strictly true when calling undo.c from the outside, in some places within a function in undo.c both values might be meaningful)

Apparently undotree() breaks this non-abstraction, this cosmetic issue can easily be fixed by ex_substitute also saving and restoring b_u_newhead, but is doesn't reflect any error really how u_undo_and_forget manipulates the actual state of the undo tree.

@justinmk
Member
justinmk commented Nov 8, 2016

Thank you @bfredl , see e99890f

justinmk and others added some commits Oct 31, 2016
@justinmk justinmk 'inccommand': rework
- Eliminate/isolate static/global variables
- Remove special-case parameter from buflist_new()
- Remove special-case ECMD_RESERVED_BUFNR
- To determine when u_undo_and_forget() should be done, check
  b_changedtick instead of a heuristic.
- use mb_string2cells() instead of strlen() to measure the :sub patterns
- call ml_close() before buf_clear_file(). Avoids leaks caught by ASan.

Original patch by:
  Robin Elrharbi-Fleury (Robinhola)
  Audrey Rayé (Adrey06)
  Philémon Hullot (DesbyP)
  Aymeric Collange (aym7)
  Clément Guyomard (Clement0)
c04ffe8
@justinmk justinmk lint e4e7b2d
@justinmk justinmk 'inccommand': preserve b:changedtick e31f900
@justinmk justinmk 'inccommand': disable 'cursorline', 'spell' in preview d0689eb
@justinmk justinmk 'inccommand': set buffer name to [Preview]
[inc_sub] is less obvious for users. Also, in the future we may want to
generalize the idea of a "preview buffer", or "incremental commands"
besides :substitute.
3ccf69c
@justinmk justinmk 'inccommand': format line numbers as "|123| "
This matches what Quickfix traditionally does.
1e0e301
@justinmk justinmk 'inccommand': rename 'incsubstitute'
'inccommand' allows us to expand the feature to other commands, such as:
    :cdo
    :cfdo
    :global

Also rename "IncSubstitute" highlight group to "Substitute".
6a3f8d4
@KillTheMule @justinmk KillTheMule 'inccommand': test: scripts/feedkeys() should not trigger preview 527ba2b
@justinmk justinmk 'inccommand': preserve 'modified'
During the live preview, the buffer-local 'modified' flag
should not be changed.
f3e8ca3
@justinmk justinmk 'inccommand': Do not trigger during scripts, feedkeys(). ff6ec70
@justinmk justinmk perf: do_sub(): avoid work, avoid screen updates
- Don't fill matched_lines if not showing a preview (!eap->is_live).
- Encapsulate: Move cursor placement logic to show_sub().
21dfbfb
@justinmk justinmk 'inccommand': Preserve curbuf->b_u_newhead.
Add tests for undotree().

Helped-by: Björn Linse <bjorn.linse@gmail.com>

When "curhead" points to a valid head, the value of "newhead" is
meaningless (and really should be set to null). The undo state for
a buffer is _logically_ the enum:

  enum UndoState {
    CurrentHead(head),
    NewHead(head),
    EmptyTree
  }

nvim _represents_ this as: whenever `curbuf->b_u_curhead` is nonnull it
should be used as the current head, and `curbuf->b_u_newhead` is
ignored. If the there is a current head, then this will be redoed on the
next redo, and its parent will be undone on next undo. Only if
`b_u_curhead` is NULL, `b_u_newhead` will be used as the head to undo
(and it is not possible to redo). Also both can be NULL, to indicate an
empty undotree. (To be fair, this only strictly true when calling undo.c
from the outside, in some places _within_ a function in undo.c both
values might be meaningful)

Apparently `undotree()` breaks this non-abstraction, this _cosmetic_
issue can easily be fixed by `ex_substitute` also saving and restoring
`b_u_newhead`, but is doesn't reflect any error really how
`u_undo_and_forget` manipulates the _actual_ state of the undo tree.
aa0e09d
@justinmk justinmk Merge #5561 'inccommand'
Initial work by:
  Robin Elrharbi-Fleury (Robinhola)
  Audrey Rayé (Adrey06)
  Philémon Hullot (DesbyP)
  Aymeric Collange (aym7)
  Clément Guyomard (Clement0)

Major revisions by:
  KillTheMule
  Björn Linse <bjorn.linse@gmail.com>
  Justin M. Keyes <justinkz@gmail.com>
0213e99
@justinmk justinmk merged commit 0213e99 into neovim:master Nov 9, 2016

3 of 6 checks passed

QuickBuild Build pr-5561 finished with status FAILED
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 71.241%
Details
@justinmk justinmk removed the RFC label Nov 9, 2016
@justinmk
Member
justinmk commented Nov 9, 2016 edited

Merging this, but I think the cursor movement should be revisited. The "jumping around" while building the :sub command is not really helpful. Especially with inccommand=split where results are shown without moving the cursor at all. (Updated: fixed)

Thank you @KillTheMule @bfredl and the original contributors!

@justinmk justinmk deleted the justinmk:incsub branch Nov 9, 2016
@balta2ar

@justinmk I'm not sure, but maybe this should be Substitute? That's what I would think after this commit: 6a3f8d4

Member

Yes, thanks.

@dmerejkowsky
Contributor

For the curious, here's what it looks like:
https://asciinema.org/a/92207

@Alok
Alok commented Nov 10, 2016

Could this be used to extend the incsearch option to highlight all candidate matches rather than just the current one (I think emacsdoes this)?

@justinmk
Member

@Alok that's being discussed on vim_dev so we'll wait for it to land there.

@gaving
gaving commented Nov 11, 2016

Congrats on the merge of this rockin' feature, this [and the associated PR's] are a great example of OSS done right!

@justinmk justinmk added a commit to justinmk/neovim that referenced this pull request Nov 27, 2016
@justinmk justinmk NVIM v0.1.7
FEATURES:
0213e99 PR #5561 'inccommand'

FIXES:
c685879 PR #5632 SECURITY FIX
d28d108 CheckHealth: Fix version comparison.
7be113d PR #5670 shell_write_cb: Schedule error message.
1d45637 jobs: ensure calling jobclose() on a pty job sends SIGHUP.
36c0ec6 tui/suspend_event(): set STDIN to "blocking"
7a4d069, cf52b88 man.vim: avoid errors in unusual circumstances
ed19873 PR #5546 ex_global: Catch CTRL-C even if it is mapped.

CHANGES:
9147331 PR #2905 encoding: only allow encoding=utf-8
5f02608 PR #5636 build: Upgrade jemalloc
f1fed42 PR #5567 l10n: Update Ukrainian translation
0542baa
@xdxter
xdxter commented Jan 13, 2017

Thanks for the work! This feature rocks!!

Though, I wonder how hard would it be to make it work with the :Subvert command from tpope's abolish plugin, which is immensely helpful in real-life substitution use cases.

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