flx icon indicating copy to clipboard operation
flx copied to clipboard

Highlight current candidate

Open bbatsov opened this issue 12 years ago • 8 comments

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?

bbatsov avatar Jul 12 '13 15:07 bbatsov

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

dgutov avatar Jul 12 '13 18:07 dgutov

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

artagnon avatar Jul 28 '13 10:07 artagnon

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).

bbatsov avatar Jul 28 '13 14:07 bbatsov

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.

artagnon avatar Jul 28 '13 14:07 artagnon

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

bbatsov avatar Jul 28 '13 14:07 bbatsov

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.

dgutov avatar Jul 28 '13 16:07 dgutov

@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.

bbatsov avatar Jul 28 '13 17:07 bbatsov

Any update on this ?

twmr avatar Oct 12 '13 20:10 twmr