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

RFE: swiper - add highlighting of regex matched/saved groups #1550

Closed
ghost opened this issue May 2, 2018 · 8 comments

Comments

@ghost
Copy link

commented May 2, 2018

The regex builder has a really helpful feature where, as well as highlighting matches in the current buffer for the regex, if that regex contains saved groups (i.e. stuff between (...) which can be referenced in a replace string using \1 etc), those are also highlighted in the matches, in a different face to how the whole match is highlighted.

At the moment, swiper highlights fully matched strings, but gives no visual indication of what's matched in each saved group. It would be super nice if that could be added.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2018

At the moment, swiper highlights fully matched strings, but gives no visual indication of what's matched in each saved group.

Are you referring to Swiper's highlighting of the original buffer or the minibuffer candidates? Which regexp builder do you use with swiper?

The reason I ask is because the default regexp builder, ivy--regex-plus, has better highlighting support than, say, ivy--regex-ignore-order, which is what I use.

Compare:

  1. make plain
  2. C-xC-fswiper.elRET
  3. C-sswi lex
    plus

with:

  1. make plain
  2. (setf (alist-get t ivy-re-builders-alist) #'ivy--regex-ignore-order)
  3. C-j
  4. Steps (2) and (3) as in previous example
    ignore-order

Note that with the default regexp builder, match groups in both the buffer and minibuffer are highlighted independently.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2018

After a quick glance at the source, it looks like the reason the minibuffer candidates are not highlighted for any regexp builder other than ivy--regex-plus is that swiper hard-codes swiper--re-builder as the :re-builder for its session, without taking ivy-highlight-functions-alist into account. This should be relatively straight forward to hack around: users can add an entry for swiper--re-builder in ivy-highlight-functions-alist, or, even better, swiper--ivy can temporarily extend the alist for its purpose. The latter is a simple but still ugly solution; maybe I'm missing a better "monkey patch", but AFAICT the whole regexp-builder/highlight-function system has to be rethought a little for a more elegant solution.

As for the buffer overlays, I think they are not highlighted properly for other regexp builders because swiper--update-input-ivy only considers the first regexp returned by ivy--regex-function for highlighting, even when this is a multi-item alist like (("foo" . t) ("bar". t)). The reason this limitation is not seen with the default regexp builder is because it returns all matching substrings as a single regexp, e.g. (("\\(foo\\).*?\\(bar\\)" . t)).

@jonathanunderwood

This comment has been minimized.

Copy link

commented May 2, 2018

I am the same user as @jgubmll - I accidentally filed this RFE with my work account. I have to admit, I am struggling to follow everything you write, as I have no familiarity with the innards of swiper/ivy etc.

I am using swiper as configured with spacemacs - it looks like that doesn't come configured to use the -plus builder - it behaves like your second example.

To answer your earlier question, I'd like to see the saved regex groups highlighted in both the original buffer and the candidate list.

To give you a better example of what I mean, start the regexp-builder with swiper.el opened in a buffer and

  1. Switch to "string" syntax (M-x reb-change-syntax)
  2. Enter: "GNU General \([A-Za-z]+\) \(License\)" in the regexp-builder.
    screenshot from 2018-05-02 22-18-40
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2018

I'd like to see the saved regex groups highlighted in both the original buffer and the candidate list.

Sounds like you want the highlighting that ivy--regex-plus (the default regexp builder) currently provides, assuming you're happy with its in-order processing of space-delimited search strings (i.e. foo bar becomes \\(foo\\).*?\\(bar\\)).

If so, then perhaps you should raise the issue/question with Spacemacs as to how to go about reverting to the default regexp builder. Otherwise, we'll both have to wait until the highlighting of other regexp builders improves.

@jonathanunderwood

This comment has been minimized.

Copy link

commented May 3, 2018

Ok, I switched to ivy--regex-plus, but still I don't see saved groups highlighted in the matched text. I'll dig some more, and I presume this is a spacemacs config problem, but any pointers you might have would be great.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 6, 2018

but any pointers you might have would be great.

Everything separated by a space is a group, no need for extra \(...\) parens:
swiper-capture-group

basil-conto added a commit to basil-conto/swiper that referenced this issue May 13, 2018
swiper.el (swiper--ivy): Improve highlighting
ivy-highlight-functions-alist does (and probably should) not include
an entry for swiper--re-builder, so highlighting always falls back
to ivy--highlight-default, even when ivy--regex-function is set to,
say, ivy--regex-ignore-order.

Temporarily updating ivy-highlight-functions-alist to include an
entry for swiper--re-builder is one suboptimal way to adapt
highlighting accordingly.

Re: abo-abo#1550
@ghost

This comment has been minimized.

Copy link
Author

commented May 15, 2018

Right - but I can't then use those groups in a replace, can I? I.e. reference them as \1 \2 etc.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 16, 2018

@jgubmll you can use those groups in a replace.

raxod502 added a commit to raxod502/swiper that referenced this issue May 31, 2018

@abo-abo abo-abo closed this in 7ec2380 Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.