Comments (29)
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:
abracadabra/src/action-providers.ts
Lines 133 to 142 in a924332
Here, we should be able to retrieve the editor's selection from
abracadabra/src/action-providers.ts
Lines 28 to 34 in a924332
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:
Lines 7 to 12 in a924332
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:
Lines 128 to 131 in a924332
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:
- The user says "quick fix funk"
- Cursorless selects the current function
- Cursorless runs the
editor.action.quickFix
VSCode command - Cursorless moves the cursor back to where it was
- 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.
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.
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.
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:
fixed.mp4
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.
Related Issues (20)
- Automatically "Convert to Template String" if we write ${} in a string HOT 2
- Inlining JSX removes curly braces HOT 1
- Extract refactoring runs on JSX attribute HOT 3
- Inline async arrow function misses braces HOT 1
- Abracadabra replaces system key binding HOT 3
- Extract Variable insert position doesn't right HOT 2
- change activationEvent to onLanguage
- Idea: Migrate to TypeScript plugin HOT 4
- "Change Signature" doesn't work on a nested function HOT 3
- "Add explicit type" refactoring HOT 1
- Inline variable if ternary misses parens HOT 5
- split declaration & init when working inside class
- JS and TS code without `;` is broken after refactoring HOT 3
- Abracadabra extension causes performance issues with other extensions (prettier formatting, etc.) HOT 7
- More inline variable missing parentheses edge-cases HOT 1
- Can't move class to an existing file HOT 2
- Move to new file: configure case of the new filename HOT 3
- Upgrade babel/traverse ti eliminate vulnerability. HOT 1
- Convert to Arrow Function: Does not work with a `export default function` HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from abracadabra.