lewang / flx

Fuzzy matching for Emacs ... a la Sublime Text.
GNU General Public License v3.0
518 stars 37 forks source link

Highlight current candidate #22

Open bbatsov opened 11 years ago

bbatsov commented 11 years ago

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 commented 11 years ago

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

artagnon commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

dgutov commented 11 years ago

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 commented 11 years ago

@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 commented 10 years ago

Any update on this ?