Alexey-T / CudaText

Cross-platform text editor, written in Free Pascal
Mozilla Public License 2.0
2.39k stars 166 forks source link

Complete_From_Text issues #5513

Closed Alexey-T closed 1 month ago

Alexey-T commented 1 month ago

From #5511, copy/paste from @pintassilgo

I just realized Completete from text already should support this, but it isn't working. From readme:

  • 'case_split': expands suggestions, autocompletion for 'AB' will include 'AzzBzz', so to get 'ValueError' just typing 'VE' (or 'VaE', 'VErr', 'ValErr' etc.) and calling on autocompletion will suggest it.
  • 'underscore_split': expands suggestions, autocompletion for 'AB' will include 'AZZ_BZZ', so to get 'supports_bytes_environ' just typing 'sb' (or 'sbe', 'supbyenv' etc.) and calling on autocompletion will suggest it.

Well, I set the option and restarted, but AB doesn't suggest AzzBzz (yes, I did include this word in the text).

Other broken option is case_sens. I believe it should be disabled by default, but it's enabled, but case is always ignored regardless of this option.

Once this gets fixed, I guess it should be easy to port the code for LSP plugin (not for me lol).

Note: I tested the reported "Complete from text" issues after disabling LSP plugin to avoid any conflict.

Alexey-T commented 1 month ago

From #5511

I looked at the code and realized case_split is only being used for ACP words, which is a bug, it should also apply to words collected from document.

Also, it should be improved to not require user to write in proper case. Currently, I need to type qS to get the suggestion querySelector. Other inputs qs, QS and QS don't suggest the word, but they should because I use case_sens=0.

Other issue is that qA doesn't work to suggest querySelectorAll, only qSA, so I can't skip typing any upper case (in this case, I shouldn't need to type S for selector, qA should be enough).

Alexey-T commented 1 month ago

mentioning @ildarkhasanshin because he did some fixes, maybe he wants to work here?

Alexey-T commented 1 month ago

I looked at the code and realized case_split is only being used for ACP words, which is a bug, it should also apply to words collected from document.

Yes, I confirm it. How to change the code to fix it? I forgot the code so pls help.

pintassilgo commented 1 month ago

Did you turn on these options?

Yes, I said about them in the initial comment you moved to this issue.

pintassilgo commented 1 month ago

Yes, I confirm it. How to change the code to fix it? I forgot the code so pls help.

option_case_split is only used in is_text_with_begin function, which is only called by get_acp_words function.

https://github.com/CudaText-addons/cuda_complete_from_text/blob/master/__init__.py

The fix is currently out of my range as I don't know Python.

Alexey-T commented 1 month ago

Try my fixes! git branch in plugin's repo https://github.com/CudaText-addons/cuda_complete_from_text/tree/fixes_for_words_match

pintassilgo commented 1 month ago

Thanks. Now the options are working for non-acp words.

But reminding there are still two issues that I already reported here:

  1. case_sens is not being followed. Its value being 0, qs should find querySelector.
  2. qa (and qA) should find querySelectorAll. Currently I need to type qSA.
Alexey-T commented 1 month ago

We should make fix here somewhere

    if option_case_split or option_underscore_split:
        if option_case_split  and  s[0] == begin[0]:
            searchwords = re.findall('.[^A-Z]*', begin)
            wordwords = re.findall('.[^A-Z]*', s)

            swl = len(searchwords)
            wwl = len(wordwords)
            if swl <= wwl:
                for i in range(swl):
                    if not wordwords[i].startswith(searchwords[i]):
                        break
                else:
                    return True

it's not my code, I don't get idea here. yet.

pintassilgo commented 1 month ago

I read somewhere that startswith is case sensitive. Don't know if this is related, because I'm asking to follow case_sens option, so for me it would be case insensitive.

Alexey-T commented 1 month ago

Problem is different. It is here

            searchwords = re.findall('.[^A-Z]*', begin)
            wordwords = re.findall('.[^A-Z]*', s)

it is splitting of word (begin or s) by upcase-letters. so upcase letters are required in your typing!

pintassilgo commented 1 month ago

I understand. This should be fixed, The screenshots examples I posted in the other issue confirms every other editor (at least the three ones I tested, VSCode, ST and Kate) supports lower case, so qs matches querySelector and qa matches querySelectorAll.

VSCode: image

Sublime: image

Kate: image

Alexey-T commented 1 month ago

I can make QUICK fix for 'qa' matching 'qqqqSsssAaaa' but my fix will break things on longer typing words: 'qqaa' WONT match qqqqSsssAaaa.

pintassilgo commented 1 month ago

I just tested more and all the tree editors do the simple fuzzy search. So, for instance, qcl also matches querySelectorAll. This is way easier to implement and would fix the issue. But the "smart fuzzy" I proposed is the best fix.

Alexey-T commented 1 month ago

OK, so we need simple fuzzy search, and so remove options 'case_split' / 'underscore_split' , but add 'fuzzy' option. maybe ok idea.

pintassilgo commented 1 month ago

If you think what I proposed is too hard, I suggest adding an option to both Complete_from_text and for LSP: "fuzzy_search". I suggest to enable it by default, because that's how other editors work.

Alexey-T commented 1 month ago

and so remove options 'case_split' / 'underscore_split'?

pintassilgo commented 1 month ago

These are a good step to the "smart fuzzy" I proposed, just not perfect yet. I will surely use fuzzy if it's the only fix applied, but I can imagine some users that don't like the full fuzzy and think current case_split/underscore_split is better. So, unless if you think code gets too bloated, I suggest to keep these.

pintassilgo commented 1 month ago

Just saying, if you don't remember, this issue is the same thing I asked here.

Alexey-T commented 1 month ago

Good, to do for me: add 'fuzzy_search' opt. Later maybe in LSP too.

pintassilgo commented 1 month ago

Thanks. Just saying, imo, this is way more important/useful than #5509. Because for command pallete there's no harm in needing to type space. But the difference in needing to type querySelectorA and qa, for instance, is huuuge. In this example alone are 12 chars less for each time I type this method.

pintassilgo commented 1 month ago

Unless if I'm missing something, fuzzy search is very simple, you just add .* between every char of the word. At least in Javascript, .* means "0 or more chars of anything".

In fuzzy search, searching for qa would match any word having q followed by a, no matter how many chars are in between.

So you just split every char of the word and join .* between them to generate the regex.

So qsa, for instance, becomes q.*s.*a.

The example in the link, from what I see, is more to calc the relevance of the match to sort the results. I guess Cuda doesn't need that.

Alexey-T commented 1 month ago

Added option 'fuzzy_search'. test it from git.

Alexey-T commented 1 month ago

Q: must i use 'case_sens' option inside fuzzy mathcing function?

pintassilgo commented 1 month ago

Great, thanks, it's working! image

Now it's just add the same for LSP.

pintassilgo commented 1 month ago

Q: must i use 'case_sens' option inside fuzzy mathcing function?

Hmm... supposing you'll keep case_split, no. I can only think of user using case_sens for case_split, not for fuzzy.

Alexey-T commented 1 month ago

Is it solved now?

pintassilgo commented 1 month ago

Yes, thanks.