-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Assistant Editor #10559
base: main
Are you sure you want to change the base?
Add Assistant Editor #10559
Conversation
ab25265
to
1616197
Compare
The CI is failing due to a linting issue. I opened #10573 about it and will update this patch set based on what is decided there to get the checks to pass. Thanks! |
Thank you for submitting this PR. I'm sorry we haven't made much progress on it yet since we've had a couple of other projects in flight, but we have seen it and will find a release to prioritize this for when we've sorted out some of the open issues. |
No worries, thanks for the update. Running with this change locally there is still an outstanding issue where the assistant tab gets lost sometimes, but since I didn’t receive any feedback for a while I moved my time to something else. Let me know when you are about ready so I can revisit this problem. (Unfortunately, no one upstream ever responded to my question so I am doing the best I can with what I have.) |
I thought @csnover was going to update the linting rules in this branch and not us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR closes issue #10532.
This patch set includes a prerequisite patch to deduplicate the code that detects the document language since the Assistant Editor also needs to do this and the checks were just copied and pasted all over the place.
The main thing this patch set introduces is a MVP version of Assistant Editor. In this implementation, the Assistant Editor can be toggled in the command menu. It opens beside the active tab group and will track the active tab from that group until it is closed.
Multiple Assistant Editors can be active at the same time for different editor groups, so e.g. you can have two editor groups each with their own Assistant Editor.
The Assistant Editor persists its state to the workspace state so when the workspace is reloaded, the Assistant Editor continues to operate normally.
There are some caveats to the current implementation that are largely the result of what seem to be very many missing APIs in VSCode:
There is also a caveat caused by the closed-source (as far as I can tell) binary blob that this extension uses. I had to write redundant (and presumably less robust) code in the Assistant Editor that tries to find the counterpart file for the currently open file, instead of being able to reuse the existing API call that the Switch Header/Source command uses.
I was actually unable to get cpptools to respond to requests at all from the extension debugger when I followed the build instructions and I don’t know what was going on there; the client appeared to be running, but the
Promise
for therequestSwitchHeaderSource
call would never settle. (Even in a fresh checkout of therelease
branch, the already existing Switch Header/Source command did not work.) I didn’t bother to try troubleshooting this since without access to the cpptools binary source it felt like a waste of time, since I can’t change the stuff in the binary anyway, and it calls this the “didSwitchHeaderSource” command and not the “queryCounterpartFileName” or whatever command, so it would be wrong.Looking forward to your feedback. Apologies in advance if any comments seem frustrated and please feel free to tone police them if it feels necessary; this is the first time I have worked on a VSCode extension, and I did not expect such a bad developer experience given how well VSCode itself works as an IDE.
Thanks,