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

add highlighter function configuration to ivy #691

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jyp
Contributor

jyp commented Sep 26, 2016

This fixes #654

It also somewhat simplifies the logic assigning highlighter for swiper.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 27, 2016

Thanks, but some tests are failing. You can make test to check your work.

Additionally, you'll need an Emacs Copyright Assignment for changes >15 lines, see the README for more info.

@jyp

This comment has been minimized.

Contributor

jyp commented Sep 27, 2016

I could fix the tests for emacs 25 and 24.5, but older versions somehow do not work. What do you recommend to debug code for old emacs versions?

Besides, I have requested a copyright assignment form.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 28, 2016

I could fix the tests for emacs 25 and 24.5, but older versions somehow do not work.

This might be good enough. As long as 25 and 24.5 pass, I can understand what needs to be done.

What do you recommend to debug code for old emacs versions?

I recommend having a reproducible config that you're comfortable with, and having 2-3 versions of this config with ELPA packages and all. Since byte-compiled Emacs code is not compatible between Emacs versions.

Other than that, just start the appropriate Emacs version and eval the failing test, try to step in etc.

Besides, I have requested a copyright assignment form.

Thanks, let me know when it's done.

@jyp

This comment has been minimized.

Contributor

jyp commented Nov 21, 2016

I have received confirmation from FSF that they have processed my copyright assignment, so this PR is legally mergeable.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Nov 21, 2016

Thanks. Can you please ask them to update the copyright.list file on fencepost? I don't see your name there yet.

@jyp

This comment has been minimized.

Contributor

jyp commented Nov 30, 2016

The copyright clerk assures that I am now on the list

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Nov 30, 2016

Thanks, I see it now as well.

Unfortunately, the patch doesn't rebase cleanly anymore. Could you please do a manual rebase of your changes on top of the current master and update the PR?

@jyp

This comment has been minimized.

Contributor

jyp commented Nov 30, 2016

I have done the rebase.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Dec 1, 2016

Thanks. I think we can maybe avoid adding highlighter to ivy-state and to the arglist of ivy-read.
Since it's easily calculated from ivy--regex-function, we can do it like that.

The reasons are:

  1. Adding an extra arg to ivy-read that won't be used often distracts from the other args.
  2. Adding a new member to ivy-state can require a byte recompilation of all packages that use ivy. I did so a month ago and I still keep getting bug reports for that.
@jyp

This comment has been minimized.

Contributor

jyp commented Dec 1, 2016

@jyp

This comment has been minimized.

Contributor

jyp commented Dec 8, 2016

Pressing a little bit to avoid getting this PR lost in the abyss of time... :)
Do you think that my analysis makes sense?

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Dec 10, 2016

It won't get lost, don't worry.

I'm happy with the interface the re-builder is selected with:

  1. The package writer should almost never use the :re-builder arg to
    ivy-read, so that the user can always have his chosen re-builder
    with all commands. However, if there's a very specific reason the
    custom re-builders can't work, the author can make the restriction.

  2. The user can customize re-builder per-command, and easily select it for all commands.

I want the same for the highlighters: have an alist variable with keys
being the re-builder functions, and values being the highlighter
functions. This way:

  1. The user can still fine-tune the highlighter. But everything will
    work without any need for customization.

  2. The author doesn't have to be bothered examining the extra args to
    ivy-read, since the highlighter is specified only via the alist,
    not via the args.

I could even think of removing the :re-builder out of ivy-read
arglist if not for compatibility issues.

If you have the time, please update the PR to have all customization go through e.g. ivy-highlight-functions-alist.

@jyp

This comment has been minimized.

Contributor

jyp commented Dec 11, 2016

@jyp

This comment has been minimized.

Contributor

jyp commented Dec 11, 2016

Ok, I've updated the code as you asked (ifaiu). Let me know what you think.

@abo-abo abo-abo closed this in ca84f24 Dec 13, 2016

abo-abo added a commit that referenced this pull request Dec 13, 2016

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Dec 13, 2016

Thanks. Please check my changes on top of your commit.

@jyp

This comment has been minimized.

Contributor

jyp commented Dec 13, 2016

@abo-abo your commit has reintroduced the bug. There are two problems with your changes:

  1. When swiper is active (or indeed when the re-builder is customized), ivy--old-re is not correctly set. To fix the issue you can change the condition back to what it was at line 2458:
        (setq ivy--old-re
              (if (eq ivy--highlight-function 'ivy--highlight-ignore-order)
                  re
                (if ivy--old-cands
                    re-str
                  "")))
  1. Setting the ivy--highlight-function when swiper is active does not work when ignoring the order. To fix the problem you can put code akin to the following around line 1548:
(if (and (eq ivy--regex-function 'swiper--re-builder)
                   (eq (cdr (assoc t ivy-re-builders-alist))
                       'ivy--regex-ignore-order))
              #'ivy--highlight-ignore-order
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Dec 14, 2016

Thanks, could you open a new PR with these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment