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

Highlight current candidate #22

Open
bbatsov opened this issue Jul 12, 2013 · 8 comments
Open

Highlight current candidate #22

bbatsov opened this issue Jul 12, 2013 · 8 comments

Comments

@bbatsov
Copy link
Contributor

bbatsov commented Jul 12, 2013

I think that flx should use some special face for the current candidate (although it's always the first one) to make it visually more apparent that it's the one selected. The matched characters in it should also be highlighted of course.

Basically I'm suggesting that we combine the standard ido face for the current candidate with the flx face for the matched characters in a candidate. I guess a face overlay would do the trick?

@dgutov
Copy link

dgutov commented Jul 12, 2013

font-lock-prepend-text-property (or append) should be a better fit.

@artagnon
Copy link
Contributor

Nope, it won't work. Look at how flx fits into ido:

(defadvice ido-set-matches-1 (around flx-ido-set-matches-1 activate)
  "Choose between the regular ido-set-matches-1 and flx-ido-match"
  (if flx-ido-mode
      (setq ad-return-value (flx-ido-match ido-text (ad-get-arg 0)))
    ad-do-it))

Now, ido writes text properties in ido-completions, which is called via ido-exhibit, a post-command-hook. Since ido is unaware of flx's existence, it does not account for text properties being set by the callchain following flx-ido-match, and simply overrides all text properties using put-text-property. You might argue that we should patch ido to add-text-properties instead, but there's no guarantee that no stray program will write undesirable text properties. You can probably get away with resetting all text properties in flx (see flx-ido-undecorate), but people who use just ido without flx may suffer.

Currently, if you enable ido faces along with flx faces, what will happen is exactly what I described: the ido faces will overwrite any text properties set by flx. Do you see why my approach in #36 does not suffer from this deficiency?

2013-07-28-155813_960x37_scrot

@bbatsov
Copy link
Contributor Author

bbatsov commented Jul 28, 2013

Yep, I did some digging into the code as well and don't think we can use directly font-lock-prepend-properties as @dgutov suggested, but I consider it a deficiency in the way flx-ido interacts with ido. Patching ido doesn't seem totally unreasonable. Maybe we'll be able to think of something else as well. @dgutov Any other ideas?

At any rate it stands to reason that ido-flx should play well with the default ido faces and not require custom code for all of them (as in #36).

@artagnon
Copy link
Contributor

font-lock-{prepend, append}-properties just calls put-text-property with a value-list built from appending the given value to values fetching from get-text-property. Nothing fundamentally new about it; whatever you use, you cannot avoid patching ido.

Maybe patch ido-completions to take an optional "append" argument, and have flx can advise it to preserve existing text-properties? That way we won't break other callers, or users of vanilla ido.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jul 28, 2013

Overriding ido-completions seems like a good option to me.

@dgutov
Copy link

dgutov commented Jul 28, 2013

Patching sounds good, but I'd prefer if it were done in Emacs trunk. The current candidate highlighting is not an essential feature anyway, and the users of older Emacs should be able to live without it.

@bbatsov
Copy link
Contributor Author

bbatsov commented Jul 28, 2013

@dgutov The sigle-match and the subdir faces are also affected. While the faces are non-essential, indeed, I think we should start by patching ido locally in ido-flx and when we're happy with the results we should submit them upstream.

@twmr
Copy link

twmr commented Oct 12, 2013

Any update on this ?

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

No branches or pull requests

4 participants