Skip to content

[RFC] Make inccomand work with multiline substitutions #7231

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[RFC] Make inccomand work with multiline substitutions #7231

wants to merge 9 commits into from

Conversation

KillTheMule
Copy link
Contributor

@KillTheMule KillTheMule commented Sep 2, 2017 •
edited
Loading

Inccomand substitution now properly works with multiline matches and substitutions, handles multibyte stuff and does highlighting.

  • Deal with the code duplication for highlighting without waiting for anyone, should simplify the logic anyways
  • Merge the new tests into inccommand_spec.lua
  • Lint
  • Reenable timeout and its test

TODO for later:

Closes #5608.

@KillTheMule KillTheMule changed the title Make inccomand work with multiline substitutions [WIP] Make inccomand work with multiline substitutions Sep 2, 2017
@KillTheMule
Copy link
Contributor Author

So, the approach of having a char_u *text in a SubResult does not seem appropriate. I'll make it a vec of lines, that should streamline things a bit.

@marvim marvim added the WIP label Sep 2, 2017
@KillTheMule
Copy link
Contributor Author

Ok, this looks much better, though it's still wrong (I add too many matches somehow, and the preview window isn't shown). Still going :)

@KillTheMule KillTheMule force-pushed the icm-multi branch 2 times, most recently from 70de238 to 9aaac75 Compare September 3, 2017 16:57
@KillTheMule
Copy link
Contributor Author

How the hell did the previous incarnation pass the tests on appveyor O_o

Anyways, logic seems sortof solid now, expect for that special casing https://github.com/neovim/neovim/pull/7231/files#diff-b9d70f1e57978ae9fbf83f73315b0c0fR3690. Next step will be to try to display the correct things in the preview window.

@KillTheMule
Copy link
Contributor Author

I did try some things, and now I've settled on a design I like. I'll do a bit of description below, if anyone does not think it appropriate please let me know. Status right now: Builds, works in simple manual tries, previews, but does not yet highlight. I'll first need to try some more elaborate substitutions, and then work on highlighting.

Design: I do as little as possible in do_sub, so performance isn't as much impacted for non-preview or even nonsplit. I only save the numbers of the lines I will need to fetch from the original buffer, as well as the position data where the match starts and ends (adjusted, because we will mostly fetch lines after the substitution). Most (hopefully all) information is already there, it just needs to be saved and passed on to the preview function. In the preview function, we fetch the right lines from the original buffer (after the substitution) and print them out. The positional data for the matches will be needed for the highlighting only, but this will also make it easy to add highlighting to the original buffer for the substituted text.

Of course, comments welcome :)

This was referenced Sep 6, 2017
Closed
Closed
@KillTheMule
Copy link
Contributor Author

Ok, the code is a mess, it will kindof spam your log file... BUT it's working, as far as I can see. Highlighting can be pretty weird with complex substitutions, but as far as I can see, it's correct.

So, I'd be happy for anyone to try this. I'd especially be interested in "Multi"-Situations: Multiple matches on a line, multiple-line-matches, multiple-line-substitutions, multiple-size-machtches, multiple-size-replacements, you name it!

Really though, the most useful would be a report from someone using Multibyte chars, I'll basically have to research how to input them to begin with...

justinmk
justinmk reviewed Sep 9, 2017
View reviewed changes
src/nvim/ex_cmds.c Outdated
static linenr_T pre_started;
static linenr_T pre_ended;
static int pre_src_id; // initialized to 0, see
// https://stackoverflow.com/questions/13251083
Copy link
Member

Choose a reason for hiding this comment

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

could be explicit, and avoid the comment :)

static int pre_src_id = 0;

Copy link
Member

Choose a reason for hiding this comment

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

Usually we put static locals at the very start of a function. I think it would be surprising to see them in the middle of other locals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About that explizit, I'm not sure about the semantics. I don't want it to be reset to 0 every time the function is called, of course. That won't happen?

Copy link
Member

Choose a reason for hiding this comment

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

@KillTheMule won't happen. static locals are initialized only once for the life of the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which, thinking about it, makes sense! Thanks, I'll change that.

@KillTheMule
Copy link
Contributor Author

KillTheMule commented Sep 10, 2017 •
edited
Loading

Fixed the old tests and an outstanding issue. Also started writing new tests.

I'm baffled that the new tests 8d4efc8#diff-ddcc7112118dfbcc54565044d90602eb also pass on master. When I try this manually on current master, nvim locks up and the substitution does not happen, so the expect should fail. But the tests do pass...

@KillTheMule KillTheMule mentioned this pull request Sep 12, 2017
Open
@KillTheMule KillTheMule force-pushed the icm-multi branch 3 times, most recently from ff6a3c0 to 9321b9b Compare September 16, 2017 21:57
@KillTheMule KillTheMule changed the title [WIP] Make inccomand work with multiline substitutions [RFC] Make inccomand work with multiline substitutions Sep 16, 2017
@marvim marvim added RFC and removed WIP labels Sep 16, 2017
@KillTheMule
Copy link
Contributor Author

Fixed a lot of bugs during writing the tests. Basically it is polished enough for a review, although I definitely want to simplify the highlighting logic in show_sub soon.

If anyone can think of another test to be added, I'd be happy to hear it.

@KillTheMule KillTheMule force-pushed the icm-multi branch 4 times, most recently from 3b845d7 to a0a144d Compare September 18, 2017 21:29
@KillTheMule KillTheMule force-pushed the icm-multi branch 4 times, most recently from d35813d to 8fca927 Compare September 23, 2017 20:16
@KillTheMule
Inccommand: Multiline substitutions, highlighting, multibyte.
fc0c282 
Make inccomand work with multiline patterns and substitutions. Also care
for proper highlighting and multibyte characters.
@KillTheMule
Fix old inccomand tests, and add more for the new functionality.
7f7d8f9
@KillTheMule
Lint
ee6be74
@KillTheMule
Move tests into original file, lint, and add a test
75d1d0f
@KillTheMule
Remove pat/sub from show_sub
9ed1066 
They were only used to not show the preview window when typing "s/" or
"s//" only, in which case the previous pattern would be reused. Now the
window is shown in that case.
@KillTheMule
Fix the last line in the preview buffer
7355791 
It would always show an empty line at the end that didn't belong.
@KillTheMule
Adjust tests for the new preview window
d8cdf2f 
... that does not have that superflous last line.

Also, remove some indeterminism for the freebsd64 tests. Partially,
those were suggested by the tests themselves, while successfull. Some of
them were added after some testing because the lookaround test would
fail on freebsd64 only.
@KillTheMule
Copy link
Contributor Author

I've added another little fix: The preview buffer would always show one superflous line (I guess that was the one that's added to a buffer upon creation). I removed that, and therefore had to adjust quite some tests.

@KillTheMule KillTheMule mentioned this pull request Sep 24, 2017
Merged
@KillTheMule KillTheMule changed the title [RFC] Make inccomand work with multiline substitutions [WIP] Make inccomand work with multiline substitutions Sep 30, 2017
@KillTheMule
Copy link
Contributor Author

KillTheMule commented Sep 30, 2017 •
edited
Loading

I've had to put in a bug fix, I'm gonna leave this as separate commits.

@marvim marvim added WIP and removed RFC labels Sep 30, 2017
@KillTheMule KillTheMule changed the title [WIP] Make inccomand work with multiline substitutions [RFC] Make inccomand work with multiline substitutions Oct 2, 2017
@marvim marvim added RFC and removed WIP labels Oct 2, 2017
@justinmk justinmk added this to the 0.2.2 milestone Oct 21, 2017
@justinmk
Copy link
Member

Merged in #7315

@justinmk justinmk closed this Oct 31, 2017
@justinmk justinmk removed the RFC label Oct 31, 2017
@KillTheMule KillTheMule deleted the icm-multi branch January 26, 2018 15:39
@justinmk justinmk mentioned this pull request Apr 23, 2018
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk justinmk justinmk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@KillTheMule @justinmk @marvim