lewang / flx

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

uppercase entry produces lower case matches #52

Closed aspiers closed 10 years ago

aspiers commented 10 years ago

Something is not right with flx when attempting to match uppercase letters. For example, if I'm visiting a buffer called mon.rb and I run ido-switch-buffer and then type M (uppercase), my minibuffer starts like this:

Buffer: M[essages*]
-> mon.rb
    *Messages*
    *Compile-Log*

(Yes, I'm using ido-vertical-mode.) Since I went to the effort of pressing Shift, there is no good reason for it to match mon.rb. Since we're fuzzy matching, I wouldn't mind if all the matches containing lowercase m characters appeared after all those containing uppercase M ... but for a lowercase match to appear first is definitely a bug. (And IMHO ideally it should be configurable whether fuzzy matching produces lowercase matches for uppercase searches.) In this case, if I press RET then it just stays in the current buffer, which is hardly a useful result.

The other bug in this situation is that the first line displays Buffer: M[essages*] which at a glance misleadingly suggests that the currently selected match is *Messages*.

FWIW I have ido-case-fold set to nil.

aspiers commented 10 years ago

Here is a partial fix, but I'm not sure how to build the hashes correctly to allow lower-case queries to match against upper-case file and buffer names.

lewang commented 10 years ago

The expected workflow does not take upper case letters into consideration. The original idea was to minimize keystrokes (i.e. no shift) and just have smart things happen. This is how Sublime Text's fuzzy matching works.

I can see your point though. I may play around with the scoring to take case into account, but I'm reluctant to because it's all in a kind of delicate balance. :)

lewang commented 10 years ago

I will add that there are other more serious ido related bugs before I get to this, unfortunately.

aspiers commented 10 years ago

Thanks for the reply.

The key point is that sometimes the Shift key is the best way to minimize keystrokes. For example, I often know that the only buffer I have open containing the capital letter M is *Messages*. If I want to jump to that, I should be able to jump immediately ido-switch-buffer M. But if you disregard capitalization, then I'll often have to type two or three extra keystrokes, because in English the bigram "me" is extremely common, and the trigram "mes" is also pretty common.

Additionally, there is an existing expectation from some users that query strings with no upper case characters should enable case folding, and query strings with some upper characters should disable it, because this is how it works with less(1), emacs' isearch when search-upper-case is enabled, and other intelligent search interfaces.

In conclusion, always disregarding capitalization is not only less efficient, but can also be counter-intuitive. Of course I am happy and grateful for you to focus on fixing more serious bugs, but when you are done with those, I would be even more grateful if you could fix this ;-) Of course it would probably be best to make this behaviour optional, since not everyone will agree with me.

Thanks again for your great work so far!

lewang commented 10 years ago

Can you try the 0.3 branch?

aspiers commented 10 years ago

Works beautifully, thanks a lot! However I had to reinitialise flx-file-cache and flx-strings-cache before it worked, and also the behaviour does not honour ido-case-fold, which I would expect it to. So I think both these issues need to be resolved before releasing.

lewang commented 10 years ago

I had to reinitialise flx-file-cache and flx-strings-cache before it worked

Fixed.

the behaviour does not honour ido-case-fold

Flx folds case by design, i.e. lower always matches upper, so I don't think there is an issue of honoring ido-case-fold.

What's changed is UPPER will only match UPPER. I can't see the value of honoring search-upper-case (BTW, I did not realize this variable existed until you mentioned it.) I'm happy to wait to see if anyone asks for it.

On Sun, Apr 20, 2014 at 7:30 AM, Adam Spiers notifications@github.comwrote:

Works beautifully, thanks a lot! However I had to reinitialise flx-file-cache and flx-strings-cache before it worked, and also the behaviour does not honour ido-case-fold, which I would expect it to. So I think both these issues need to be resolved before releasing.

— Reply to this email directly or view it on GitHubhttps://github.com/lewang/flx/issues/52#issuecomment-40892996 .

Le

aspiers commented 10 years ago

Flx folds case by design, i.e. lower always matches upper, so I don't think there is an issue of honoring ido-case-fold.

But that's not true. The 0.3 behaviour is that lower doesn't always match upper - specifically, it doesn't match upper iff the search contains uppercase. And that's how I personally like it.

What's changed is UPPER will only match UPPER. I can't see the value of honoring search-upper-case (BTW, I did not realize this variable existed until you mentioned it.)

Actually I didn't mention that variable - and I didn't know about it until you mentioned it just now ;-)

I'm happy to wait to see if anyone asks for it.

Well, that's precisely the behaviour I want, and which 0.3 currently offers - which is great. But some people might not like it. So perhaps I should have warned about the code not honouring search-upper-case rather than it not honouring ido-case-fold :-)

lewang commented 10 years ago

But that's not true. The 0.3 behaviour is that lower doesn't always match upper - specifically, it doesn't match upper iff the search contains uppercase.

No. Flx still folds all lower case characters. Only an UPPER must match UPPER.

aspiers commented 10 years ago

Either you are misunderstanding me, or the behaviour I'm seeing from flx is different from the behaviour you expect.

With ido-switch-buffer if I type mess then it matches *Messages*, but if I type mEss then it doesn't. So the latter case is a clear example of where a lower case m does not match an uppercase M. Both cases also serve as examples of how the code currently behaves like the behaviour offered by setting search-upper-case to t (which is how I like it). But if it was set to nil then it should have different behaviour.

This table explains when ido-switch-buffer should match the *Messages* buffer:

ido-case-fold search-upper-case mess Mess mEss
nil nil no yes no
nil t no yes no
t nil yes yes yes
t t yes yes no

IMHO flx should preserve this behaviour by honouring those two variables.

lewang commented 10 years ago

With ido-switch-buffer if I type mess then it matches Messages, but if I type mEss then it doesn't.

"m" is being folded to match "[mM]", but E must match a "E". Try mEss with "MessagEss".

So a search term with UPPERs do not disable fold in flx. You can never disable fold in flx, which is kind of fundamental to the magic of the algorithm. The addition handling of capitals is a natural extension of the algorithm, but the control variables don't really make sense.

On Sun, Apr 20, 2014 at 7:59 PM, Adam Spiers notifications@github.comwrote:

Either you are misunderstanding me, or the behaviour I'm seeing from flxis different from the behaviour you expect.

With ido-switch-buffer if I type mess then it matches Messages, but if I type mEss then it doesn't. So the latter case is a clear example of where a lower case m does not match an uppercase M. Both cases also serve as examples of how the code currently behaves like the behaviour offered by setting search-upper-case to t (which is how I like it). But if it was set to nil then it should have different behaviour.

This table explains when ido-switch-buffer should match the _Messages_buffer: ido-case-fold search-upper-case mess Mess mEss nil nil no yes no nil t no yes no t nil yes yes yes t t yes yes no

IMHO flx should preserve this behaviour by honouring those two variables.

— Reply to this email directly or view it on GitHubhttps://github.com/lewang/flx/issues/52#issuecomment-40908304 .

Le

aspiers commented 10 years ago

Ahh, I see. In that case it only remains to document this and then we can close this issue. Thanks!

lewang commented 10 years ago

@aspiers Thanks for your help with this.

aspiers commented 10 years ago

Thank you!!