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

May swiper-isearch provide an option not showing multiple times for one line's multiple match? #2034

Closed
dolmens opened this issue Apr 18, 2019 · 25 comments

Comments

@dolmens
Copy link

@dolmens dolmens commented Apr 18, 2019

I personally still want to use avy to move to the match quickly, and dislike the duplicated lines for multiple matches in one line, It's somewhat not that beautiful as other ivy things.

@otadmor
Copy link

@otadmor otadmor commented Apr 21, 2019

One appriach cold be count-lines between two search forward.
Second approach would be to search forward a \n after each existing search forward. Searching incrementally for \n could be a fast way of getting line numbers with only one pass on the buffer.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 21, 2019

@dolmens Why wouldn't you use swiper instead of swiper-isearch for that?

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 21, 2019

@otadmor FWIW, count-lines uses the current buffer's newline cache, so it shouldn't be too slow.

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 21, 2019

I have actually benchmarked line-number-at-pos and found it very slow. As line-number-at-pos uses count-lines I assumed count-lines was the slow one.
Does the "buffer's newline cache" expects the whole buffer to be visited in the past? This might not be a good assumption when using swiper.

https://github.com/emacs-mirror/emacs/blob/master/lisp/simple.el

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 21, 2019

I have actually benchmarked line-number-at-pos and found it very slow. As line-number-at-pos uses count-lines I assumed count-lines was the slow one.

Yes, count-lines is probably the lion's share of line-number-at-pos. I didn't say count-lines is negligible; I said it shouldn't be too slow when called once for each change in swiper. Having said that, I have not understood what you are suggesting to do with count-lines, and there is likely a better way to code this, as calling count-lines too frequently is an anti-pattern.

Does the "buffer's newline cache" expects the whole buffer to be visited in the past?

Sorry, I don't know.

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 22, 2019

I don't really know whether that would help, but instead of "count-lines" we could do something like (search-forward (concat "\n|" ivy--re) nil 'noting )
And then decide if we want to count a new line depends on looking-at. I'm not sure if this is faster as this would cause many elisp - native c jumps. This is probably depends on the buffer s content.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 22, 2019

@otadmor Can you please explain what the purpose of doing that is? I don't understand how that relates to the OP.

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 22, 2019

New candidates should not be pushed unless a new \n was found. This would resolve this issue.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 22, 2019

How would that be different to using swiper instead of swiper-isearch?

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 22, 2019

They could give different performance. Swiper saves the whole buffer in its own structure (list) and the could take time. Using forwardsearch and storing the relevant results would save both time and space.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 22, 2019

Swiper saves the whole buffer in its own structure (list) and the could take time.

AFAICT, swiper-isearch does the same. In other words, it's not incremental.

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 22, 2019

swiper-async does (re-search-forward re)
With given
[Code]
(let* ((re-full (funcall ivy--regex-function str))
(re (ivy-re-to-str re-full))
[/Code]

Then it does buffer-substring with the line caused re-search-forward to return.
The swiper-isearch will return all the lines in the buffer only if re will match a newline.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 22, 2019

I'll answer my own question: the difference between swiper and restricting swiper-isearch to a single match per line is that the former collects all lines in the buffer before Ivy applies its filtering, whereas the latter only collects those lines which match the current input. There is a tradeoff between collecting all buffer lines only once, and collecting them every time the input changes.

The OP was about limiting matches to one per line, which is what swiper already does. The OP did not say that swiper was so slow to start up that they had to use swiper-isearch instead. That would be a separate issue, I think.

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 22, 2019

The question of using swiper or swiper-isearch is indeed irrelevant here, its the author choice to use swiper-isearch. I agree the suggestion of using swiper is the quickest solution which does not require any coding. As of implementing this feature for swiper-isearch - it is a nice discussion of how to do it the best and I believe everyone will benefit of it - both us and who-ever would want to implement it in the future.
BTW - I believe the second quickest solution require about one line of code with beginning-of-line/forward-line after finding a match.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 22, 2019

I believe the second quickest solution require about one line of code with beginning-of-line/forward-line after finding a match.

Would you be willing to provide a PR for this?

Loading

@otadmor
Copy link

@otadmor otadmor commented Apr 23, 2019

No

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 8, 2019

@dolmens I agree that it's not beautiful. But I don't want to break the invariants of ivy. If there are 6 candidates in the collection, there must be 6 candidates in the minibuffer. Otherwise, the whole logic breaks.

cou cou
counsel.el (counsel-company): Use company-prefix

For the input co, swiper-isearch has 6 candidates, and swiper has 2 candidates. C-n and C-p can move between them, they don't have to know that they're dealing with either swiper or swiper-isearch: they're just generic ivy functions.

What we can do is fix the highlighting. Right now, the highlights look like this:

|co|u |co|u
|co|u |co|u
|co|unsel.el (|co|unsel-|co|mpany): Use |co|mpany-prefix
|co|unsel.el (|co|unsel-|co|mpany): Use |co|mpany-prefix
|co|unsel.el (|co|unsel-|co|mpany): Use |co|mpany-prefix
|co|unsel.el (|co|unsel-|co|mpany): Use |co|mpany-prefix

It would look at lot better like this:

|co|u cou
cou |co|u
|co|unsel.el (counsel-company): Use company-prefix
counsel.el (|co|unsel-company): Use company-prefix
counsel.el (counsel-|co|mpany): Use company-prefix
counsel.el (counsel-company): Use |co|mpany-prefix

I'll try to implement this when I get some time.

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 8, 2019

I just got the idea that it can also be improved in ivy-format-function without breaking the invariants for C-n / C-p.
This function gets a list of e.g. 10 candidates to display in the minibuffer. It can easily detect consecutive strings that are the same and fold them together in a way that still shows which one of the folded equal strings is selected.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 8, 2019

This function gets a list of e.g. 10 candidates to display in the minibuffer. It can easily detect consecutive strings that are the same and fold them together in a way that still shows which one of the folded equal strings is selected.

Will this work even when one of the consecutive candidates is scrolled out of view?

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 8, 2019

@basil-conto, of course. We can collapse the 4 lines in the example to:

|co|unsel.el (|co|unsel-|co|mpany): Use |co|mpany-prefix

Where all 4 instances of co are highlighted in one line, and one of them is current. We basically need to highlight the one line in the minibuffer the same way we do in the main buffer.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 8, 2019

Right, thanks for explaining.

Loading

@otadmor
Copy link

@otadmor otadmor commented May 8, 2019

Highlighting in the minibuffer would have to remember the offset of the candidate in the line itself. As far as I remember the highlighting is done after the transformfn, so this data would have to get from the original candidate from ivy-candidates to the wnd candidates. This would also have to change the re to start with ^ and start the highlighting from the offset in the line.
Saving the line begin point and the match starting point will allow both fast check of duplicated lines (compare line start point instead of the whole line) and calculation of the relative position of the match inside the line for highlight.

Loading

@abo-abo abo-abo closed this in 39a9e94 May 9, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 9, 2019

Thanks, all. I think it looks a lot better now. Please test.

Loading

@manuel-uberti
Copy link
Contributor

@manuel-uberti manuel-uberti commented Jun 26, 2019

@abo-abo is something changed after your fix? I'm not able to theme swiper-isearch-current-match, see for instance purcell/color-theme-sanityinc-tomorrow#131

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 26, 2019

@manuel-uberti That face is now removed, see the comment on PR. You can style swiper-line-face instead.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants