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

Add Assistant Editor #10559

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

Add Assistant Editor #10559

wants to merge 2 commits into from

Conversation

csnover
Copy link

@csnover csnover commented Feb 21, 2023

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:

  • The Assistant Editor tab has no indicator that it is special in some way because there is no API to apply labels to tabs
  • The Assistant Editor tab will be deleted and recreated whenever it needs to switch to a new document, because there is no API to replace content of a tab, and no API to embed a standard TextEditor inside a custom Editor that would eliminate the need for any sort of tab management
  • The Assistant Editor tab could get lost if someone does a bunch of stuff to change their layout, because the Tab API invalidates the objects it exposes when tab groups change, and also has no API to determine whether a tab was invalidated despite not being closed (see What is the correct way to retain a reference to a specific tab? vscode-discussions#480 where I tried asking what the right way to do this is supposed to be), and I do not trust the hacks that currently exist in this code recover the tab. Hopefully this is a bug upstream and the extra tracking code can go away someday
  • The Assistant Editor tab will always jump to the end of the tab group it exists in whenever it switches to a new document, because there is no API to specify where a tab should be created within a tab group, and no API to move a tab other than some commands that only operates on the active editor and there is—you probably are noticing a theme by now—no API that I could find to switch focus to a particular tab

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 the requestSwitchHeaderSource call would never settle. (Even in a fresh checkout of the release 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,

@csnover
Copy link
Author

csnover commented Feb 22, 2023

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!

@bobbrow
Copy link
Member

bobbrow commented Mar 29, 2023

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.

@csnover
Copy link
Author

csnover commented Mar 30, 2023

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.)

@sean-mcmanus
Copy link
Collaborator

I thought @csnover was going to update the linting rules in this branch and not us.

Copy link

@gdamiaN538 gdamiaN538 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 participants