Update command palette search to prioritize "longest substring" match.

[e82eric]

Summary of the Pull Request

Addresses: #6693

Repurposed work from #16586

• I think the fzf algo fits here where it optimizes to find the optimal match based on consecutive chars and word boundaries.
  • There are some edge cases where a match with a small gap could get a higher score than a match of consecutive chars when the match with a gap has other bonuses (FirstChar * Boundary Bonus). This can be adjusted by adjusting the bonuses or removing them if needed.
• From reading the thread in Command Palette search algorithm needs to prioritize "longest substring" match, may need multiple passes #6693 it looked like you guys were leaning towards something like the fzf algo.
• License file is now updated in https://github.com/nvim-telescope/telescope-fzf-native.nvim repository
  • Update license to include Junegunn nvim-telescope/telescope-fzf-native.nvim#148
  • Just a friendly reminder junegunn/fzf#4310
• Removed the following from the original implementation to minimize complexity and the size of the PR. (Let me know if any of these should be added back).
  • Query expressions "$:StartsWith ^:EndsWith |:Or !:Not etc"
  • Slab to avoid allocating the scoring matrix. This felt like overkill for the number of items in the command pallete.
  • Fallback to V1 algorithm for very long strings. I want to say that the command palette won't have strings this long.
• Added the logic from GH#9941 that copies pattern and text chars to string for comparision with lstrcmpi
  • It does this twice now which isn't great...

Feel free to close if it doesn't make sense for what ever reason (E.g. License file still an issue, wrong direction for fix, too complex, etc).

References and Relevant Issues

#6693

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

• Closes #xxx
• Tests added/passed
• Documentation updated
  • If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated (if necessary)

[e82eric]

Sorry for the spam open/close. Let me know if I you guys are interested in this and I can reopen it.

[lhecker]

Generally speaking, probably yes, but I personally haven't gotten a chance yet to look at it. Currently every member of the terminal team is unfortunately either working on another project or on sick leave, hence the overall delays since late last year. One of the things @zadjii-msft made was released just recently: https://learn.microsoft.com/en-us/windows/powertoys/command-palette/overview
The project I'm working on will be released soon as well.

I plan to review this PR after I fixed the remaining major issues the the Preview/Canary version has. (Which is what has occupied what little time I had for Windows Terminal since then.)

[e82eric]

Sorry, didn't mean to rush you, was more worried I was spamming you guys with all of these fuzzy finding PRs. Please let me know if it doesn't make sense for what ever reason and I can close it.

[lhecker]

First of all, I'd like to thank you so much for taking care of the licensing situation!

Unfortunately, there's still some issues with this code and non-English languages.

I'm also somewhat confused by the code's dissimilarity with the original fzf to be honest (that's not an issue in on itself; it just surprised me). It prompted me to try and use Copilot to transform the original Go code to C++ though. It almost one-shotted the entire translation. I've since patched up the issues it had and here's the current state: https://gist.github.com/lhecker/dd840ed2f74e8f66d0f43fd49e999b88
Unfortunately, it doesn't work correctly, and I ran out of time working on it. If you want to play around with that code, please feel free to do so!

That aside, for the upcoming project that I've worked on, I ported VS Code's fuzzy matcher to Rust. Here's the original: https://github.com/microsoft/vscode/blob/071eafaf5ab4de3814466b616fe00a66d2739ad2/src/vs/base/common/fuzzyScorer.ts
If you'd like I could generate a C++ port of it next week based on my Rust version. The benefit of that code is that it's a lot simpler. The disadvantage is that fzf is a leagues better algorithm and if we had it, it would definitely be superior of course. I guess it's a trade-off.

If we stick with this fzf port, I really don't think it has to be perfect for us to merge it. But I do think though that we should at least get rid of the towlower calls and similar. The current version uses lstrcmpiW to implement case-insensitive comparisons which do work correctly (even if it's honestly bad to abuse that function for this purpose haha).

[lhecker]

This will execute the fzf matching logic twice. I think we should replace the two functions with a single one called _update (or similar) which runs ParsePattern only once and updates both members.

[e82eric]

Updated, I think I may have gone too far with this, I realized that ParsePattern was getting executed for every item and moved the pattern parsing outside of UpdateFilter which ended up bringing the snippets pane and suggestions UI into scope. Hopefully this isn't too invasive.

[lhecker]

I finished reviewing this implementation today and it has me a little confused to be honest. It's quite different from the original fzf. Among others, the CharClass enum is missing the Delimiter, Letter, and Whitespace types.

That aside, I'd turn this enum into a enum class and assign it an uint8_t size explicitly, so that the LUT I'll suggest later is going to be 4x more compact.

[e82eric]

Updated

[lhecker]

FYI: find_first_not_of(L" ") and find_first_not_of(L' ') are not the same and the latter will be much faster. This can also exploit the fact that npos is guaranteed to be the max. size_t value:

const auto off = str.find_first_not_of(L' ');
return str.substr(std::min(off, str.size()));

[e82eric]

Updated

[lhecker]

These 3 functions are documented to only work with ASCII. I recommend using u_charType and a LUT. Something like this:

static constexpr auto lut = []() {
    std::array<CharClass, U_CHAR_CATEGORY_COUNT> lut;
    lut.fill(CharClass::NonWord);

    lut[U_UPPERCASE_LETTER] = CharClass::Upper;
    lut[U_LOWERCASE_LETTER] = CharClass::Lower;
    lut[U_TITLECASE_LETTER] = CharClass::Letter;
    lut[U_MODIFIER_LETTER] = CharClass::Letter;
    lut[U_OTHER_LETTER] = CharClass::Letter;

    lut[U_DECIMAL_DIGIT_NUMBER] = CharClass::Number;

    lut[U_SPACE_SEPARATOR] = CharClass::White;
    lut[U_LINE_SEPARATOR] = CharClass::White;
    lut[U_PARAGRAPH_SEPARATOR] = CharClass::White;

    return lut;
}();

return lut[u_charType(c)];

(This code is untested, and I wrote it for the fzf port I mentioned elsewhere.)

[e82eric]

Updated, I didn't realize those only work with ASCII, I am you glad mentioned that, I didn't understand why lstrcmpi fixed the original issue until now...

[lhecker]

Same applies here. I believe that creating this copy is unnecessary, as you can just lowercase each char from the original text within the loop below.

[e82eric]

Updated and then changed when I moved everything to UChar32.

[lhecker]

There are multiple uses of lstrcmpiW throughout this file (and in the current matching logic before this PR) and I don't think that's a good way to implement this. Each call to lstrcmpiW goes through multiple layers just to arrive at CompareStringEx which is rather expensive for just comparing two characters with folding.

In this particular instance, currentChar is already lowercase so the only problem is currentPatternChar which could simply get lowercases with ICU and then compared against the currentChar. Technically we then also need to take care of the difference between lowercasing and casefolding (e.g. the lowercase of Greek "sigma" has two variants), but I think that's a secondary issue.

FWIW, we also technically need to iterate by codepoint here instead of by character. Right now, this code is limited to UCS2 (no surrogate pair support), but I think that's similarly a secondary concern.

[e82eric]

I "think" I have this in a better state now.

• Updated to use u_foldCase for the lower casing and folding. I think this works for everything except for some of the more complex folding, ex fold results in multiple code points
• The text and pattern both get converted to UChar32 to iterate over the codepoints.
• The positions get converted back to Utf16 positions for the highlighting
• I tried to add some test cases for the multi language support and surrogate pairs, but there are probably still some major holes in my understanding of Unicode, are the there other scenarios that I should be testing for?

[lhecker]

This could use the same find_first_not_of logic as above, but without slicing the string.

[e82eric]

Updated and then changed when I moved everything to UChar32.

[lhecker]

Here's another 2 instances of towlower FWIW.

[e82eric]

Updated and then changed when I moved everything to UChar32.

[e82eric]

I'm also somewhat confused by the code's dissimilarity with the original fzf to be honest (that's not an issue in on itself; it just surprised me).

I think there are a couple of things that contributed this.

• The original telescope-native port that I ported was done back in 2022 before the delimiter class type etc, and I think he had removed some stuff like the normalization.
• When I ported it to c++ I mangled all of the variable names trying to understand how all of the dynamic programming and back tracking work.

Should I try to update use the latest fzf logic?

[e82eric]

That aside, for the upcoming project that I've worked on, I ported VS Code's fuzzy matcher to Rust. Here's the original: https://github.com/microsoft/vscode/blob/071eafaf5ab4de3814466b616fe00a66d2739ad2/src/vs/base/common/fuzzyScorer.ts If you'd like I could generate a C++ port of it next week based on my Rust version. The benefit of that code is that it's a lot simpler. The disadvantage is that fzf is a leagues better algorithm and if we had it, it would definitely be superior of course. I guess it's a trade-off.

Totally understand if the VS Code algo is more maintainable etc, let me know what direction you guys want to go. I am happy to try to port the typescript or the rust code or incorporate your port of the rust code.

[e82eric]

The other thing that I am worried about is that I still haven't figured out how to run LocalTests_TerminalApp which will probably have some failures from the new algo.

Oh, right, I forgot your question. First you need to build these two projects:

Then you do this in PowerShell:

Import-Module .\tools\OpenConsole.psm1
Invoke-OpenConsoleTests -Test localTerminalApp

For me this currently fails in 18 tests, but only 3 of those are due to your PR. We're not doing a great job of maintaining these tests unfortunately. You can tell it to only run your relevant tests like this:

Invoke-OpenConsoleTests -Test localTerminalApp -TaefArgs /name:TerminalAppLocalTests::FilteredCommandTests::*

Edit: I fixed them.

That worked, I am able to run them locally now. Thank you!!

[e82eric]

What I'm missing mostly is some Unicode normalization like fzf has: https://github.com/junegunn/fzf/blob/master/src/algo/normalize.go
This could be part of https://github.com/microsoft/edit/tree/main/tools/grapheme-table-gen in the future so we can generate a small trie for the mapping.

This may be way beyond my understanding of unicode. Would that be adding something like this (from the fzf port you had done)?

    // Normalize rune using ICU NFD and removing combining marks (Mn)
    // This aims to replicate the Go version's accent removal.
    UChar32 normalizeRune(const UChar32 c)
    {
        if (isAscii(c))
        {
            return c;
        }

        static const auto norm = []() {
            UErrorCode status = U_ZERO_ERROR;
            return unorm2_getNFDInstance(&status);
        }();
        if (!norm)
        {
            return c;
        }

        UChar decomp[8];
        UErrorCode status = U_ZERO_ERROR;
        const auto len = unorm2_getDecomposition(norm, c, &decomp[0], ARRAYSIZE(decomp), &status);
        if (U_FAILURE(status) || len <= 0 || !isAscii(decomp[0]))
        {
            return c;
        }

        return decomp[0];
    }

[lhecker]

Yes, although I'm not entirely sure if that would be fast enough and whether it's really the same thing that fzf does. That's something we'd have to experiment with over time.

[lhecker]

Thank you for your hard work!

[DHowett]

seriously, thank you. this is excellent work

[DHowett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DHowett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[lhecker]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DHowett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DHowett]

@e82eric certainly appreciates us duking it out on his PR. 😁

[DHowett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DHowett]

such size
wow so _t

[e82eric]

Just catching up on this, sorry about this! Guessing I should have been running the x86 build locally to catch that?

[lhecker]

Wohoo it merged!

Yes, that type of failure only comes up in the x86 build. But it's fine - you already did way enough for us. 🙂 (I hope I didn't make a mistake with the size_t conversion!)

[DHowett]

Perhaps the most annoying thing is that you can't actually run the x86 build. It just... builds or it doesn't. Nobody tests it on Windows 11. It isn't compatible with the console kernel driver.

Thank you so much @e82eric for literally everything. Seriously.