Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding auto complete support for multi cursors #3442

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Neko-Box-Coder
Copy link
Contributor

Currently, auto-complete only works on the latest cursor, which makes multi-cursor editing not pleasant.

This small change should cover most of the scenarios for auto-complete on multi-cursors.

@dmaluka
Copy link
Collaborator

dmaluka commented Aug 25, 2024

image

Press Tab -> what we expect is this, right?

image

Whereas with this PR we get this:

image

@Neko-Box-Coder
Copy link
Contributor Author

Neko-Box-Coder commented Aug 25, 2024

@dmaluka
No, I don't think it should auto-complete different things. I think most of the scenario users would place their cursors on the same text occurrence to complete.

Although slightly different but VSCode is doing something similar to what I am doing. The way it handles texts that are not the same is it will fill in the whole auto-complete word instead of partial word for places that don't have the same text.

So like

Test
Te|    ->    Test|
Te|    ->    Test|
Pl|    ->    PlTest|
Te|    ->    Test|

Instead what this PR do atm:

Test
Te|    ->    Test|
Te|    ->    Test|
Pl|    ->    Plst|
Te|    ->    Test|

But I think very few people would expect (and use) auto-complete to work properly in such situation anyway and the complexity is not worth it.

@dmaluka
Copy link
Collaborator

dmaluka commented Aug 25, 2024

I think most of the scenario users would place their cursors on the same text occurrence to complete.

So in those scenarios when they don't, we should at least not autocomplete the text at the given cursor at all, rather than autocomplete it with something unexpected and meaningless.

I imagine it should not be difficult to add corresponding checks, for example by iterating the cursors not in Buffer.CycleAutocomplete() as you do but in BufPane.Autocomplete() instead.

But I'm still not sure what's wrong with autocompleting different words at different cursors. That would allow making Autocomplete a real multicursor action, instead of manually iterating cursors for it as a special case. And to implement that, roughly speaking, just need to move b.Suggestions etc from Buffer to Cursor, which seems to be a more appropriate place for it anyway and would make things conceptually cleaner.

@Neko-Box-Coder
Copy link
Contributor Author

I think most of the scenario users would place their cursors on the same text occurrence to complete.

So in those scenarios when they don't, we should at least not autocomplete the text at the given cursor at all, rather than autocomplete it with something unexpected and meaningless.

I imagine it should not be difficult to add corresponding checks, for example by iterating the cursors not in Buffer.CycleAutocomplete() as you do but in BufPane.Autocomplete() instead.

Okay sure, I will give a go.

But I'm still not sure what's wrong with autocompleting different words at different cursors. That would allow making Autocomplete a real multicursor action, instead of manually iterating cursors for it as a special case. And to implement that, roughly speaking, just need to move b.Suggestions etc from Buffer to Cursor, which seems to be a more appropriate place for it anyway and would make things conceptually cleaner.

At first, I tried just putting AutoComplete into multi actions, and obviously it didn't work at all since it was fixated on the last cursor. But even with a bit of tweaking, what happened is that it will cycle the autocomplete field as many times as the multi cursors, which makes sense considering the function is being called for each cursor.

This raises the question of how does the user know which autocomplete is being filled for each multi-cursor if each of them can fill in a different word. Not to mention there would be no way to choose which autocomplete entry for each cursor.

@dmaluka
Copy link
Collaborator

dmaluka commented Aug 25, 2024

This raises the question of how does the user know which autocomplete is being filled for each multi-cursor if each of them can fill in a different word. Not to mention there would be no way to choose which autocomplete entry for each cursor.

Ah yeah indeed, good point, the autocomplete menu would be different for different cursors, so it is not an option.

@Neko-Box-Coder
Copy link
Contributor Author

Neko-Box-Coder commented Aug 25, 2024

I imagine it should not be difficult to add corresponding checks, for example by iterating the cursors not in Buffer.CycleAutocomplete() as you do but in BufPane.Autocomplete() instead.

What checks should I be adding?
I can do the checks in BufPane.Autocomplete() for each cursor relatively easily.
But checking if we are auto-completing the same word for each cursor is slightly tricky, for me at least.

Extracting autocomplete functions,
Adding autocomplete checks to each cursor
@glupi-borna
Copy link
Contributor

For me personally, the most intuitive behavior would be to only allow completions which are the same across all cursors. Anything else I think of has very limited usability.

Here's an edge(?) case to illustrate why I think other approaches don't work:

test_name 
t| -> test_name
T| -> ???

I could think of a number of different valid completions for 'T': TestName, TEST_NAME, Test_Name. The one we can get by forcing the same completion across cursors is Test_name (at best, if we keep the 'T'). Maybe that's exactly what one might want in some cases, but I think it will not be satisfactory in most cases.

So, IMO, it would be enough to filter the list of completions to ones that apply to all cursors and then cycle through that.
But that's just my 2 cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants