Abracadabra does not play nice with cursorless about abracadabra HOT 29 CLOSED

Not === now? Thank you! I'll give it a try

from abracadabra.

Really nice response. From the Cursorless developers (and users) I want to convey our appreciation!

from abracadabra.

I will have a look at this one. I wonder if the code behaves differently.
Let me know if you spot other scenarios that don't work well with cursorless… or if that's more of a race condition thing (maybe) 🤔

from abracadabra.

Thanks for the feedback. I'll install cursorless and dig into this to understand what's going on—in theory it should be working, but reality says I'm missing something here 😄

from abracadabra.

I did a quick investigation in the code yesterday and this is the place where we tell VS Code what to do when a suggestion is selected:

Here, we should be able to retrieve the editor's selection from

And we could pass it as an argument to the command, according to the docs: https://code.visualstudio.com/api/references/vscode-api#Command

And that would require updating the refactorings so they use the given selection instead of the current one when executed. This sounds a lot of work, but there is actually a single place that passes the editor to the refactoring functions:

There is no argument here, but I guess we would get the optional arguments we pass to the command. This function is what we give to vscode.commands.registerCommand here:

If you want to give it a try, go for it. Otherwise I'll try it out when I have some time 😉

from abracadabra.

This is now available with 9.2.0. Let me know how it goes 😉

from abracadabra.

I spoke too soon. It seems that some things are working but others are not. Can you take a look at "invert condition" to see if there might be an issue going on there? It behaves differently if I put the cursor there and run the quick fix from there versus if I try to run it using cursorless and the cursor is in another place.

from abracadabra.

Thanks for investigating! Let me know if you need a hand reproducing.

from abracadabra.

FYI, I have a very busy week, so I reserved some time in my calendar to dig into this next week 😉
I'll let you know how it goes.

from abracadabra.

That seems to have done the trick! Thank you so much 🙏

from abracadabra.

Just to clarify the order of events here:

1. The user says "quick fix funk"
2. Cursorless selects the current function
3. Cursorless runs the editor.action.quickFix VSCode command
4. Cursorless moves the cursor back to where it was
5. The user selects a refactoring from abracadabra

Note that steps 1-4 happen over the course of a few hundred milliseconds in response to step 1; step 5 is in response to a subsequent user interaction

My hypothesis is that abracadabra uses the cursor position at 3) to determine the list of proposed quick fixes, but then uses the cursor position at 5) when applying the quick fix. We propose that abracadabra should use the cursor position from 3) when applying the quick fix. The latter behaviour is what we've observed for the built-in refactorings

from abracadabra.

Ooooh, thanks for reporting and making me aware of cursorless (sounds awesome, I need to give it a try).

@pokey assumption is certainly right. Abracadabra uses the position of the cursor when the refactoring is triggered, but it uses the correct position to suggest refactorings.

I think a proper fix would be to use the cursor position that resulted in the refactoring suggestion instead of asking the editor where the cursor is—if the refactoring was executed by accepting a suggestion, of course.

It may or may not be really hard to do. I'll have a look at that when I have some time 😉

from abracadabra.

Ooooh, thanks for reporting and making me aware of cursorless (sounds awesome, I need to give it a try).

🙏😊

I think a proper fix would be to use the cursor position that resulted in the refactoring suggestion instead of asking the editor where the cursor is—if the refactoring was executed by accepting a suggestion, of course.

Yep that sounds right to me

It may or may not be really hard to do. I'll have a look at that when I have some time 😉

Haha yep fingers crossed it's an easy one; always hard to tell before you get in there 😅

from abracadabra.

I think I got it fixed, it was as easy as I expected.

I'll let you confirm if the fix worked for cursorless with the next release—I should craft one today or tomorrow 😉 If not, feel free to re-open the ticket and I'll investigate deeper. But I feel confident 😄

from abracadabra.

@all-contributors please add jaresty and pokey for ideas

from abracadabra.

@nicoespeon

I've put up a pull request to add @jaresty! 🎉

from abracadabra.

@all-contributors please add pokey for ideas

(thanks for giving me more context on this issue, it was helpful)

from abracadabra.

@nicoespeon

I've put up a pull request to add @pokey! 🎉

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

from abracadabra.

Let's try that again 🙄

@all-contributors please add pokey for ideas

from abracadabra.

@nicoespeon

I've put up a pull request to add @pokey! 🎉

from abracadabra.

I just tried it out and it's working great! Thank you so much for the quick turnaround-this is awesome 🙏

from abracadabra.

Had to remove the screen grab - not open source code there. LMK if you have trouble reproducing.

from abracadabra.

Hey I wanted to give a quick heads up that I'm not super confident in my initial assessment that some were working. I'm not going to be at my computer for a while but will run a test when I can. I wanted to say something quick in case my message led you down the wrong path.

from abracadabra.

Ok, now I'm pretty confident. The "wrap in for loop" one is working but the ones targeting conditions aren't. I'll try to go through the list more thoroughly when I can. However, I could still be confused about what's default versus coming from abracadabra. Do all the abracadabra refactoring have the sparkles emoji in the menu? If so, I'm not sure I tested the right commands.

from abracadabra.

The toggle braces (JSX Element) command also seems to have this problem.

from abracadabra.

So does the "Convert to template literal" command.

from abracadabra.

Update: I found why. The problem wasn't solved by passing the editor used to generate the quick fix to the action itself because… it is a reference to the original VS Code editor that gets mutated. I have planned to refactor the code to not have these class instances around and pass immutable data structures instead… but that will take some time.

I'll find a workaround for this scenario. The main thing we need here is the selection, and we can create our own Selection instance from the VS Code one. That should cut the reference and solve the problem.

I'll do that and report when it's fixed.

from abracadabra.

There we go:

Before the fix, it would likely have failed because the refactoring was executing with the wrong cursor position (because the ref was mutated). I patched the selection so it preserves the position from when the quick fix was triggered—not ideal, but good enough until I refactor the architecture as planned.

from abracadabra.

9.2.1 should work with cursorless. But again, let me know if it still doesn't 😄

from abracadabra.