-
Notifications
You must be signed in to change notification settings - Fork 0
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 document tracker to set activeFileEntrypoint
context
#1883
Conversation
activeFileEntrypoint
context
bf49247
to
c03b1b5
Compare
d475928
to
a826fdd
Compare
Remove console logs Clean up disposable code
Correct params for entrypoint inspection Use vscode-uri utils for dirname and basename
a826fdd
to
df6aca0
Compare
@@ -685,6 +698,7 @@ | |||
"eventsource": "^2.0.2", | |||
"get-port": "5.1.1", | |||
"mutexify": "^1.4.0", | |||
"retry": "^0.13.1" | |||
"retry": "^0.13.1", | |||
"vscode-uri": "^3.0.8" |
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.
Added vscode-uri
as a direct dependency to use the basename
and dirname
functions: https://github.com/microsoft/vscode-uri/blob/main/src/utils.ts
{ | ||
"command": "posit.publisher.deployWithEntrypoint", | ||
"title": "Deploy with this Entrypoint", | ||
"icon": "$(cloud-upload)", |
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.
Using a cloud-upload
icon to avoid needing to deal with a PNG, or getting an SVG into a font for release.
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.
As discussed, do we just want to use the PNG that we have for the old publisher icon, so we can replace both at once to the new one?
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.
I tested this out and it does work, and doesn't look too bad even though it is being scaled down from 450x450
to 16x16
However since it is a png
it doesn't get colored using the active theme. So even if I changed it to be white/gray/etc it would look incorrect depending on the theme used.
Personally I'd prefer to avoid the non-native look until we get that fixed.
If others disagree, it can be easily changed while I'm out with the below example:
{
"command": "posit.publisher.deployWithEntrypoint",
"title": "Deploy with this Entrypoint",
"icon": "assets/img/color/posit-publisher.png",
"category": "Posit Publisher"
}
extensions/vscode/src/extension.ts
Outdated
new DocumentTracker(), | ||
commands.registerCommand(Commands.DeployWithEntrypoint, () => { | ||
commands.executeCommand(Commands.HomeView.Focus); | ||
console.log("'Deploy with this Entrypoint' button hit!"); |
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.
Currently the button only console logs.
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.
Easily built upon! Great!
dir: workspace.asRelativePath(dirname), | ||
entrypoint: uriUtils.basename(document.uri), | ||
}); | ||
return hasKnownContentType(response.data); |
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.
I'm only marking a document as an entrypoint if any of the inspections have a ContentType
different than Unknown
if (dirname.fsPath === workspaceFolder.uri.fsPath) { | ||
return "."; | ||
} |
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.
workspace.asRelativePath(dirname)
returns a path relative to the workspace folder, but if the path IS the workspace folder then it will return an absolute path.
That doesn't work for us, so here I check to see if the file system path is the same and return a "."
.
* @returns If the text document is an entrypoint | ||
*/ | ||
async function isDocumentEntrypoint(document: TextDocument): Promise<boolean> { | ||
const dir = relativeDir(document.uri); |
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 was quite tricky digging into what VSCode could do for us and what else is out there. I broke it out into its own utils function so we can get a dir
path for any API requests.
* @param options Options for the activation | ||
* @param options.forceUpdate Whether to force the entrypoint to update | ||
*/ | ||
async activate(options?: { forceUpdate?: boolean }) { |
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.
The API calls only happen when we create a TrackedEntrypointDocument
or when it is activated and needs an updated.
This occurs when the document is saved, or the document is re-opened.
TrackedEntrypointDocument
s don't update if the file is open in VSCode, another file becomes active, then the original file is active again. That is because nothing has changed since the first API call.
onTextDocumentClosed(document: TextDocument) { | ||
this.removeDocument(document); | ||
} |
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.
Documents are removed when closed since another editor could potentially change the document. This way when a document is re-opened we recheck just in case.
async function isDocumentEntrypoint(document: TextDocument): Promise<boolean> { | ||
const dir = relativeDir(document.uri); | ||
// If the file is outside the workspace, it cannot be an entrypoint | ||
if (dir === undefined) { |
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.
If we don't want anything but .
level entrypoints for now we can easily add that check here.
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.
True, but I don't think we need to per our implementation plan.
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 looks really good and I think I have a handle on it.
If you could update the command to accept the document URI parameter that will be passed in, per my comment, and then output it in the console log, I think you'll have this PR at a good state. I'll approve it so you aren't blocked by me. Thanks!
{ | ||
"command": "posit.publisher.deployWithEntrypoint", | ||
"title": "Deploy with this Entrypoint", | ||
"icon": "$(cloud-upload)", |
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.
As discussed, do we just want to use the PNG that we have for the old publisher icon, so we can replace both at once to the new one?
async function isDocumentEntrypoint(document: TextDocument): Promise<boolean> { | ||
const dir = relativeDir(document.uri); | ||
// If the file is outside the workspace, it cannot be an entrypoint | ||
if (dir === undefined) { |
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.
True, but I don't think we need to per our implementation plan.
extensions/vscode/src/extension.ts
Outdated
new DocumentTracker(), | ||
commands.registerCommand(Commands.DeployWithEntrypoint, () => { | ||
commands.executeCommand(Commands.HomeView.Focus); | ||
console.log("'Deploy with this Entrypoint' button hit!"); |
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.
Easily built upon! Great!
This PR adds a new context to the extension that tracks if the active document is an entrypoint with a
ContentType
that is known (aka notUnknown
)If the active document is an entrypoint - found via
/api/inspect
- we show a button which currently is non-functional, but will lead a user through deploying.Note that
licenses.md
didn't need to change sincevscode-uri
is already included from dependencies.Intent
Start of #1848
Type of Change
Approach
The approach here was to create a tracker that created all of the listeners needed itself, tracked the documents it needed to, and updated the context.
This way the extension could create a
DocumentTracker
and add it to itsdiposables
with no other interaction between the two.The
DocumentTracker
handles which documents need to be tracked via VSCode listeners, andTrackedEntrypointDocument
s handle updating themselves if they are or are becoming the active document.Everything else was broken off as a utility function.
Directions for Reviewers
Test the entrypoint context in a few cases:
code some-file.txt
from the Desktop for example)Then do the same from a folder at a higher level. The
dir
andentrypoint
query params should take care of the nested folder part for us.