Add consult-keep-lines function

[oantolin]

Here's the keep-lines function we discussed. The increased-gc seemed to take care of the last remaining performance issues after switching to actual buffer manipulation instead of overlays.

[oantolin]

Figuring out how the change-group API for undo works was interesting.

[minad]

• Why are you using the after-change-functions instead of the post-command-hook? Is there an advantage?
• Regarding the with-increased-gc - I don't think we should use this macro like that. It should only be used around pure computations not around something which starts a recursive editing session.

[oantolin]

They just run fewer times. post-commad-hook runs after every command including cursor movement, after-change-functions only after commands that change the minibuffer text. It seemed more correct to me to not redisplay if you are just moving the cursor around. I bet if you hold down the left or right arrow keys you can tell the difference in responsiveness between the two approaches (I didn't test that, but I think that's what would happen).

[oantolin]

• Regarding the with-increased-gc

Sure I'll defer to your judgement on it. It does make a big difference, though. You know, it might be enough to put it around the call to string-split: before I added the increased gc, there would be a pause before showing the prompt.

[minad]

A few more comments:

• You have this when-let block - Is this correct, since when the result is empty all lines should be deleted?
• Regarding the string-split, I should have mentioned that before. This should be rewritten as a loop over the buffer, going one line forward each step, instead of first allocating the whole buffer-string.
• We should probably also add consult--fontify-all

[oantolin]

• You have this when-let block - Is this correct, since when the result is empty all lines should be deleted?

That's a bug. Thanks for catching it.

• Regarding the string-split, I should have mentioned that before. This should be rewritten as a loop over the buffer, going one line forward each step, instead of first allocating the whole buffer-string.

OK, will do.

• We should probably also add consult--fontify-all

I'll do that too.

Did I sell you on after-change-functions?

[minad]

Did I sell you on after-change-functions?

Yes, it is okay. I cannot use it for preview since there we also have to observe arrow movements. But I could probably use it for async too.

[oantolin]

We should probably also add consult--fontify-all

And turn off font-lock! Otherwise the lines get recolored anyway.

[minad]

And turn off font-lock! Otherwise the lines get recolored anyway.

Why? consult--fontify-all just ensures that font-lock is applied first such that the lines which are read after that are colored. Should this be changed somehow? Maybe you have a better idea? I am using the same mechanism for consult-line.

EDIT: I think I got it, you mean that font lock should not be applied when doing the preview? Then it makes sense to turn it off!

[oantolin]

Because I'm inserting those lines into the original buffer. When they are inserted font-lock recolors them.

[oantolin]

I think I got it, you mean that font lock should not be applied when doing the preview? Then it makes sense to turn it off!

Yes, this is what I meant.

[minad]

Okay, this way we are trading a small startup overhead for a faster preview. I would take this!

[oantolin]

Here's a corrected version. It's possible that increased gc around the line-gathering loop would still help.

[minad]

@oantolin I just tried it. I like it! And it seems to be pretty fast. But unfortunately the undo does not seem to work in all cases. When quickly typing and then pressing C-g it seems not to restore the original content. This should be more robust, but I am not familiar with this undo mechanism you are using.

Two more comments:

• There is the initial argument which is ignored. Just remove it?
• Instead of string-join we should also use a loop and insert directly, this should also improve performance noticeably for large files.

[oantolin]

I just tried it. I like it! And it seems to be pretty fast. But unfortunately the undo does not seem to work in any cases. When quickly typing and then pressing C-g it seems not to reset. This should be more robust, but I am not familiar with this undo mechanism you are using.

Mmh. I can't seem to reproduce that.

Two more comments:

• There is the initial argument which is ignored. Just remove it?

Sorry! 🤣 I thought about doing something, but then I completely forgot about it. I'll just remove it.

• Instead of string-join we should also use a loop and insert directly, this should also improve performance noticeably for large files.

I used string-join on purpose here, thinking it would be faster. See alphapapa's benchmarks.

[minad]

Mmh. I can't seem to reproduce that.

I don't know what the problem is. As if some other computations is prematurely interrupted. Maybe I am interrupting the restore operation even?

I used string-join on purpose here, thinking it would be faster. See alphapapa's benchmarks.

I find this hard to believe. Since string-join is maybe an intrinsic it is probably faster for small strings. But for many larger strings (whole buffer content), the loop may win. I will take a look at the benchmarks :)

[oantolin]

I find this hard to believe.

I don't know, maybe some hooks run after each insert? In which case fewer is better. One thing that would run if I weren't disabling it is jit-font-lock (I believe, not completely sure). Maybe there are other things that would run too.

[oantolin]

Since string-join is maybe an intrinsic

Close enough, it's a thin wrapper around mapconcat, which is written in C.

[minad]

Okay, it seems insert is slow since it does a lot of hook stuff and handles markers. Then we should keep the string-join I guess.

[oantolin]

Would consult--with-increased-gc around just the line gathering block be OK? It makes a big difference in how long it takes to show the prompt initially.

[minad]

Regarding the undo - when the undo does not work it prints the message "Undoing to some unrelated state".

[minad]

Would consult--with-increased-gc around just the line gathering block be OK? It makes a big difference in how long it takes to show the prompt initially.

Yes it is precisely meant for such things.

[oantolin]

Regarding the undo - when the undo does not work it prints the message "Undoing to some unrelated state".

I saw that message several times while I was screwing up how you use the change group API. I haven't seen it since the last few changes. I wonder what I'm still doing wrong.

[oantolin]

I also see the message sometimes. I think I just wasn't cancelling soon enough after typing.

[minad]

:+1: I also tried atomic-change-group right now! It does it :)

[oantolin]

I think that last commit takes care of the undo error, but I'm not certain.

[oantolin]

👍 I also tried atomic-change-group right now! It does it :)

Ah, good. Did you try it in the same spot? Or around something more?

[minad]

I tried it around everything. See the simplify commit.

[minad]

But I should probably keep the point resetting behavior you had, this is better.

[oantolin]

I think you simplified too much: your version does not amalgamate the changes. If you exit normally, without cancelling, your version doesn't undo in a single step.

[minad]

If you exit normally, without cancelling, your version doesn't undo in a single step.

Ouch, I thought atomic group means that it automatically amalgamates. I didn't read the docs, I just tried this :see_no_evil:

[oantolin]

The name does sound like it will amalgamate. But what it does is ensure that either all changes in the group apply, or none of them do. It does not amalgamate the changes into just one.

[minad]

I still wonder what you are doing wrong then. The atomic-change-group code looks like your code.

[oantolin]

Maybe it has to do with while-no-input? I really don't know.

[minad]

I am not sure. But the point is that this atomic-change-group around everything seems to work for me. And it is just like what you had. The only difference are these undo-limit, undo-outer-limit settings etc.

[oantolin]

Do you want to just add the limit settings to my version and see if that works?

[oantolin]

Oh, you reverted. I bet you are adding the limit stuff right now.

[minad]

I am trying locally - but you can push again if you want, I just wanted to make clear that my changes are not the right ones.

[oantolin]

Did you try my last version that makes each preview operation atomic? I couldn't trigger the message anymore with that change.

[oantolin]

If we can't figure this out, maybe your original approach with overlays can be made fast enough. It certainly should be safer.

[minad]

I made some changes, it seems more robust now. Maybe the problem is that while-no-input can somehow corrupt the undo state?

[oantolin]

Maybe, I also wondered if while-no-input was responsible. Does while-no-input really make much difference here in responsiveness? Maybe we can just remove it.

[minad]

@oantolin I think the current version is sufficiently robust!

[minad]

I merged it in the current form, please give it a test and another check!

[oantolin]

I'll test soon. In the meantime, maybe instead of binding after-change-functions to nil, it's even better to bind inhibit-modification-hooks to t. That disables both after-change-functions and before-change-functions.

[minad]

I applied some more performance improvements. It is funny what kind of monster this function has become in comparison to the simple prototypes we had originally :rofl:

But now it performs reasonably well in my tests also on large files and seems robust too.

[oantolin]

I tried it and the performance is a little funny: it feels faster than before on some Emacs Lisp files of around 2000 lines I tried (consult and embark, actually), and is pretty snappy on a LaTeX file with long lines too, but at the same time it also feels slower than before in the *Packages* buffer. Before, in the *Packages* buffer it struggled to get started and struggled with the first couple of letters but was snappy after that. Now it seems to struggle after every character.

So in general it seems faster. I also couldn't get it to fail, so it also seems more robust. It's weird about the Packages buffer, but I don't think it matters.

[minad]

I've also enlarged the scope of the gc macro. It is around everything now. https://github.com/minad/consult/commit/58be54e07f1d4e08769af77df7b51c8536891b53

[jixiuf]

currently consult-keep-lines/consult-flush-lines use delete-region delete unwanted lines, Is there any possibility just make the unwanted lines invisibles like hide-lines And add a consult-show-all-lines.

IEdit now support skipping invisible lines,say https://github.com/victorhge/iedit/issues/121

[minad]

@jixiuf hide-lines uses the 'invisible text property, right? It would certainly possible to add such show/hide-lines commands. But I am not sure if they are as useful. Since keep/flush-lines edit the actual text, they add value as editing commands. Note that you can also use undo to go back. Is there some use case you have in mind where invisibles would be significantly better?