-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Enable extensions.json
#9043
Enable extensions.json
#9043
Conversation
@Kreyren, I should have some time to circle back to this shortly. Would you be willing to let me know which features listed above (or not listed above) you'd like to see in the initial version of this functionality? |
@vince-fugnitto, since you've been active in this area, I wonder which of the features mentioned above - or others not mentioned - you'd like to see implemented as part of this PR. I tried reaching out to the original issue creator, but haven't gotten any response. |
@colin-grant-work I think what you’ve listed is already great, maybe only thing we could add would be the toast which is the main way I ever installed recommended extensions (although this is subjective). I’d also be fine if it is omitted in the initial pull-request :) |
49130ca
to
e4e4507
Compare
e4e4507
to
4ae3a7f
Compare
@@ -133,45 +133,59 @@ export class FoldersPreferencesProvider extends PreferenceProvider { | |||
} | |||
|
|||
async setPreference(preferenceName: string, value: any, resourceUri?: string): Promise<boolean> { | |||
const sectionName = preferenceName.split('.', 1)[0]; | |||
const configName = this.configurations.isSectionName(sectionName) ? sectionName : this.configurations.getConfigName(); | |||
const firstPathFragment = preferenceName.split('.', 1)[0]; |
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.
A lot of code has been rewritten here. The substantive change is that after trying the files matching the section name (e.g. extensions.json
if the preference starts with 'extensions'
), we fall back to settings.json
if the change has been rejected. This allows the provider for extensions.json
to reject changes that contain the prefix extensions
but are not in the schema for that file (e.g. extensions.ignoreRecommendations
).
There is also a stylistic change in the setup of the final iterators. The code on master
uses an array containing functions that either return a provider or add another function to the end of the array. The conditions for returning the provider are increasingly broad with each subsequent function, so it was effectively a prioritization mechanism that avoided a sort
call.
The new code follows a similar principle, but instead of gradually adding to the same array, it creates a number of prioritized buckets, guaranteeing that we only consider each candidate once after it has been prioritized, and, I hope, clarifying the precedence order of the various conditions.
for (const provider of providers) { | ||
if (this.configurations.getName(provider.getConfigUri()) === configName) { | ||
return provider.setPreference(preferenceName, value, resourceUri); | ||
const defaultConfigName = this.configurations.getConfigName(); |
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 is also rewritten to allow us to fall back to settings.json
if the change is rejected by section providers (e.g. extensions.json
). There isn't any real change to the other operations.
4ae3a7f
to
cd38859
Compare
@vince-fugnitto, in the end, to address the |
@vince-fugnitto, would you be willing to take a look at this for me? I think you've tested it out previously, and I've since implemented a solution to the preference naming issue, so I think it's in working order. |
cd38859
to
66e7310
Compare
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 verified the changes under different use-cases, and it looks very good.
I have a few improvements in mind:
-
Possibly re-ordering the tree parts in the extensions-view - we might want to keep
recommendations
the highest (so they are discoverable and align with vscode), whilebuiltins
should be last. -
Minor: Should we collapse 'recommended' - should the
recommended
tree-part be collapsed if no recommendations are present? -
More user-friendly problem marker for invalid extensions - when an invalid format is present in the
extensions.json
for an extension, we display a message, albeit not so user friendly compared to vscode:
theia:
String does not match the pattern of "^\w[\w-]+\.\w[\w-]+$".
vscode:
Expected format '${publisher}.${name}'. Example: 'vscode.csharp'.
- I've had some issues where the
recommended
stops displaying in the browser after a reload, I suspect it might have to do withcors
issues I reported in the past: (error: cors issue when using post api endpoints eclipse/openvsx#289).
- improvement (can be omitted initially): toast when an extension cannot be found (at the moment there is an error log regarding refreshing):
theia:
root ERROR [dbaumer.vscode-eslint]: failed to refresh, reason: Error: Extension with id dbaumer.vscode-eslint not found at https://open-vsx.org/api
at VSXRegistryAPI.<anonymous> (file:///home/evinfug/workspaces/theia/examples/electron/lib/71.bundle.js:4049:31)
at step (file:///home/evinfug/workspaces/theia/examples/electron/lib/71.bundle.js:3908:23)
at Object.next (file:///home/evinfug/workspaces/theia/examples/electron/lib/71.bundle.js:3889:53)
at fulfilled (file:///home/evinfug/workspaces/theia/examples/electron/lib/71.bundle.js:3880:58)
66e7310
to
e99051b
Compare
@vince-fugnitto, thanks for the review. Mostly sounds great, and I've done what could be done easily. Let me know what of the rest you definitely want to see in this PR.
👍 , done.
Maybe, but the timing and interaction with state restoration are awkward. The preferences won't settle for a while, but we could wait until the same time that we show the toast and then collapse it if we haven't found any recommendations? Might upset state-restoration purists who deliberately opened it in the last session, and leaving it open because we think there are going to be plugins to populate it with could wind up failing if the plugins just don't load as you describe below. Happy to go either way.
👍, done.
Seems like we could use better error handling in the loading process more generally? Most of the time, when I do a search, the progress bar just keeps going forever, so there's also something odd going on there. I'm willing to take a look at smoothing things out, but I might be inclined to do it in a separate PR, if it's about interacting with the VSX registry generally, rather than the functionality here. |
e99051b
to
0f276f0
Compare
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 verified the functionality and confirmed the following 👍
- the
extensions.json
file is recognized when under.theia
. - the
extensions.json
file is recognized when under.vscode
. - the
extensions.json
schema correctly identifies when the id for the extension is not formatted correctly following ({publisher}.{name}
) - users are prompted with a user-friendly warning when they have an invalid extension id.
- extensions from the
extensions.json
are correctly displayed, and placed under therecommended
tree-view part. - extensions under
recommended
are responsive to changes made underextensions.json
(additions, deletions, adding underunwantedRecommendations
). -
unwantedRecommendations
correctly have precedence overrecommendations
. - the
show recommended
action correctly opens the extensions-view with the@recommended
prefix. - the
show recommended extensions
command correctly opens the extensions-view with the@recommended
prefix. -
extensions.ignoreRecommendations
set totrue
will ignore recommendations and hide the notification on startup. -
extensions.ignoreRecommendations
set tofalse
will prompt users if they want to install recommended extensions on startup. - the notification successfully installs extensions when selecting
install
. - in a multi-root workspace, the individual
extensions.json
(potentially under each workspace root) will successfully aggregate all recommended extensions.
packages/vsx-registry/src/browser/vsx-extensions-contribution.ts
Outdated
Show resolved
Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-contribution.ts
Outdated
Show resolved
Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-contribution.ts
Outdated
Show resolved
Hide resolved
packages/vsx-registry/src/browser/vsx-extensions-contribution.ts
Outdated
Show resolved
Hide resolved
0f276f0
to
945fa1e
Compare
Signed-off-by: Colin Grant <[email protected]>
945fa1e
to
d6270c1
Compare
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 changes work well for me 👍 The feedback I had last iteration and discussed with you offline regarding issues when reading from .theia/extensions.json
has been resolved along with some minor UI updates.
What it does
Fixes #8982 by adding support for
extensions.json
to the preference system.Also fixes #9034.
Also fixes #8070 (for the @recommended case).
I'd like to see a real solution to #9255, but this PR just creates overrides to be used with this package that adds some extra rules for determining whether a preference should be saved in
extensions.json
orsettings.json
.Here are the features of VSCode's
extensions.json
system that I'm aware of. Feel free to mention or add any I've missed:extensions.json
recognized by preference system.Recommended
widget appears in plugin view.Recommended
widget.extensions
prefix as the name of the file, which will be a bit of a challenge for our preference providers to handle, since they assume that a section name (extensions
,tasks
, etc.) should not be used as a prefix in thesettings
object.)@recommended
query in Plugin View search barHow to test
extensions.json
file in a.theia
or.vscode
folder under the/a root.recommendations
array of the following structure:{"recommendations": []}
<alphanumeric or - >.<alphanumeric or - >
(with-
barred from first position in both alphanumeric groups)Recommended
is present. (May require resetting workbench layout.)Installed
widget.recommendations
, add a few to a new array labeledunwantedRecommendations
(leaving them inrecommendations
, as well). Observe that any listed underunwantedRecommendations
do not appear in theRecommended
widget.extensions.json
file to each root of a multi-root workspace, and theRecommended
widget should aggregate and display (all recommendations) - (all anti-recommendations). That is, an anti-recommendation anywhere will override a recommendation anywhere, even if they aren't the same folder.extensions
object with the same schema to thesettings
object of a workspace file (after Enable Workspace-Scoped Tasks #8917, it will be possible to putextensions
outside thesettings
object.)extensions.ignoreRecommendations
. Observe that the value is set in asettings.json
file, not anextensions.json
file.Review checklist
Reminder for reviewers