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

Improve Swiper minibuffer highlighting #1551

Closed
wants to merge 2 commits into from

Conversation

@basil-conto
Copy link
Collaborator

commented May 2, 2018

Commentary

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. @abo-abo Can you think of a better way to achieve this, short of redesigning the whole regexp-builder/highlight-function system?

Re: #1550

Example

  1. make plain
  2. (setf (alist-get t ivy-re-builders-alist) #'ivy--regex-ignore-order)
  3. C-j
  4. C-xC-fswiper.elRET
  5. C-sswiper isearch with an overview

Before

before

After

after

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 2, 2018

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

It is actually not suboptimal, but rather entirely wrong - the improved substring highlighting in the After screenshot is only applied the first time around; any repeated invocation of swiper reverts to the highlighting in the Before screenshot.

@muirmanders

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

Coincidentally, I started trying to fix regex-ignore-order highlighting in swiper recently. I was looking at the buffer highlighting first instead of the minibuffer highlighting. For highlighting, swiper seems to only support regex builders that return a single regex. In particular swiper--update-input-ivy takes the caar of the regex parts, discarding all but the first regex part in the case of ignore-order. swiper--add-overlays similarly only works with a single regex. I was reworking these to operate on a list of regex parts.

Edit: just saw your comment in #1550 (comment)

Should I pause my effort and see what happens with this PR, or do you think they are unrelated? I'd like to contribute to making regex-ignore-order a first class regex option. I've noticed some other worrisome things like ignore-order does not make use of ivy--regex which seems to set some global state variables, so maybe other things don't work as well.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2018

@muirmanders

Should I pause my effort and see what happens with this PR, or do you think they are unrelated?

I opened this PR because I thought I had stumbled across a quick fix to minibuffer highlighting at least, but I was wrong. I'm not actually familiar with how everything interoperates and I don't currently have much free time, so please don't pause your efforts because of me.

I'd like to contribute to making regex-ignore-order a first class regex option.

As a fellow user, I look forward to seeing the fruit of your efforts. Thanks for working on this.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 6, 2018

Thanks for working on this.

Same from me. I would love to get this fixed, but I don't have much free time in the coming week or two.

basil-conto added 2 commits May 2, 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: #1550

@basil-conto basil-conto force-pushed the basil-conto:blc/issue-1550 branch to 45e1d75 May 13, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2018

Seeing as I won't be working on this again in the immediately foreseeable future, should I close this PR, or would you rather it be kept open as a reminder?

@muirmanders

This comment has been minimized.

Copy link
Contributor

commented May 13, 2018

If you are done with it, please close it.

@basil-conto basil-conto deleted the basil-conto:blc/issue-1550 branch May 14, 2018

raxod502 added a commit to raxod502/swiper that referenced this pull request May 31, 2018
abo-abo added a commit that referenced this pull request Jun 13, 2018
Highlight multiple regexps correctly
Fixes #654
Fixes #1550
Fixes #1587
Supersedes #1551
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.