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

Lazy activation for most commands, jres view #150

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

Conversation

jwunderl
Copy link
Member

probably can leave this one to merge in after our initial release to put in preview channel until we have more changes / things to release

  • for activation command, check for nested pxt.json as well. That file is a pretty good indication it's a 'pxt focused' repo in the first place. It does appear that there's an issue with workspaceContains when checking top level files (workspaceContains:./pxt.json or workspaceContains:/pxt.json or worksapceContains:pxt.json), as it is firing off the activation event even when the folder is completely empty:
    image
    but with workspaceContains:**/pxt.json it's correctly firing off only when the file is present.
  • hide the asset tree views / commands that aren't help or initializing a repo until a makecode project has been loaded:
    when loading empty repo:

image

image

Then after 'create a new project' or loading in a folder that contains pxt.json they'll pop in as they are now:

image

@@ -24,8 +24,7 @@
"url": "https://github.com/microsoft/vscode-makecode"
},
"activationEvents": [
"onCommand:makecode.create",
"workspaceContains:./pxt.json",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not actually sure this is a valid glob pattern. Doesn't seem to work for me looking at a glob checker online. Might be why it was activating too much before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't completely sure what was going on here, was planning on looking into it a bit more and possibly filing a bug; I tried the three variants of 'top level file checking' (workspaceContains:./pxt.json, workspaceContains:/pxt.json, workspaceContains:pxt.json) and saw the same behavior for each, and it seems like they should have worked (or at least the last one) as I see similar checks in other extensions activation events; e.g. https://github.com/microsoft/vscode-python/blob/main/package.json#L71-L77 (and if it's an invalid pattern I would think it would want to throw an error or default to false?)

That said, I did forget to mention that this still doesn't fully fix the issue as we still appear to be getting hit by the bug mentioned in #147 (comment) and activating early for that treason at times. (that's what I get for putting up a draft pr late on a friday) -- it does minimize it a bit though as fewer extra commands would show up.

@@ -24,8 +24,7 @@
"url": "https://github.com/microsoft/vscode-makecode"
},
"activationEvents": [
"onCommand:makecode.create",
"workspaceContains:./pxt.json",
"workspaceContains:**/pxt.json",
Copy link
Member

Choose a reason for hiding this comment

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

Would you still need makecode.create as an activation event here? From my (small) knowledge that command should still be an activation command options since you'd be running it with nothing but an empty workspace open.

Copy link
Member Author

Choose a reason for hiding this comment

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

In vscode that line was giving a warning saying it was now unneeded and is taken as an activation event automatically since we define the command

Copy link
Member

Choose a reason for hiding this comment

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

Nice. That used to be a common extension issue (forgetting to add a command activation) so that makes sense for core vscode to have added. Can't run a command if you are not activated.

@jwunderl jwunderl marked this pull request as ready for review March 29, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants