Add "Recommend" action to extension viewlet #50419

reyronald · 2018-05-24T21:09:54Z

Related to #13456

So I've been configuring a lot of workspaces recently and found it incredibly annoying to have to manually add the recommended extensions to the configuration file. Thought it would be nice to have a button to do so in the extension viewlet and that it wouldn't be that difficult to implement, so I gave it a shot.

I put this PR together as a proof of concept. It is still a work in progress, but I would like it to be reviewed along with the feature itself before investing more time in it.

What do you think? In case the feature is accepted, is the overall approach I took worth pursuing? I'm committed to finishing the PR if the maintainers agree.

msftclas · 2018-05-24T21:10:42Z

All CLA requirements met.

ramya-rao-a

@reyronald Thanks for the PR! This would be a good feature to have.

I have left some comments to start with. We can work on getting this into the next milestone.

ramya-rao-a · 2018-05-25T20:44:32Z

product.json

The changes to .vscode/settings.json and product.json should not be part of the PR

ramya-rao-a · 2018-05-25T20:58:48Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

 		@IWorkspaceContextService protected contextService: IWorkspaceContextService,
 		@IFileService private fileService: IFileService,
 		@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
 		@IJSONEditingService private jsonEditingService: IJSONEditingService,
 		@ITextModelService private textModelResolverService: ITextModelService
 	) {
-		super(id, label, null);
+		super(id, label, cssClass, enabled);


There are setters for the class and enabled. So why pass them here? This then forces us to send null values for the existing 2 actions

ramya-rao-a · 2018-05-25T21:03:42Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

+			if (this.extension) {
+				this.getFolderRecommendedExtensions(this.configurationFile).then(recommendedExtensions => {
+					this.enabled = recommendedExtensions.indexOf(this.extension.id) === -1;
+					this.class = this.enabled ? RecommendToFolderAction.Class : `${RecommendToFolderAction.Class} hide`;


Wont setting enabled to false in the previous line hide the action? Do we have to set "hide" in the class explicitly?

We don't :P. This was before I figured out I had to modify the extensionsActions.css file to get them to dissappear. Will fix.

ramya-rao-a · 2018-05-25T21:04:45Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

+
+	run(): TPromise<any> {
+		return this.addRecommendedExtensionToFolder(this.configurationFile, this.extension.id)
+			.then(() => this.update());


Why not just set enabled to false here than call update?

+1. This raises another question for me though, we should handle the case when the promise fails and reenable the button, would this suffice? Not sure if it would be necessary to call update in that case since we already know we failed.

run(): TPromise<any> { this.enabled = false; return this.addRecommendedExtensionToFolder(this.configurationFile, this.extension.id) .then(() => this.update()) .catch(() => { this.enabled = true; }); }

ramya-rao-a · 2018-05-25T21:09:55Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

+// TODO: make sure we preserve comments
+
+export class RecommendToFolderAction extends AbstractConfigureRecommendedExtensionsAction {
+	private static readonly Class = 'extension-action recommend-to-folder';


Css wise the 2 actions look the same. Then why not use the same css class for both?

ramya-rao-a · 2018-05-25T21:14:35Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

+		@IExtensionsWorkbenchService private extensionsWorkbenchService: IExtensionsWorkbenchService
+	) {
+		super(
+			'extensions.install',


This should be a dedicated id for this action.

ramya-rao-a · 2018-05-25T21:16:47Z

src/vs/workbench/parts/extensions/browser/extensionsActions.ts

+			jsonEditingService,
+			textModelResolverService
+	);
+		this.disposables.push(this.extensionsWorkbenchService.onChange(() => this.update()));


The extensionWorkbenchService change event is fired when an extension is installed/updated/uninstalled which shouldnt affect the recommend actions.

Instead, we should update on this.contextService.onDidChangeWorkbenchState which gets fired when workspace changes from multi-root to single root and vice versa

reyronald · 2018-05-25T23:12:16Z

Hello @ramya-rao-a ! Glad to see the suggestion was accepted and happy to work on the PR!

A lot of the code on that first commit is intentionally unpolished just because I didn't spend much time thinking about it in case the team wanted to go in another direction. Also it was at the end of my work day and I had to go home 😅 !

That said, I have a few general questions:

Do you think it would be worth it to have a transitioning middle state for the button where it is visible but disabled while the async work is being done?
I created two different classes for each action to keep the logic as isolated as possible, but they ended up looking really similar, are you ok with us leaving it like that or would you rather have some more code reusing? Like unifying them in one Action with conditional statements for each workbench state, have both actions inherit from a new common parent, etc...?
Do you think we should update the extensions list UI in the sidebar when the action completes successfully to include the green bookmark in the top-left corner immediately?
Do you agree with the color, placement, and labeling of the buttons?
Is there a task or script to automatically format the code 🙈 ?

reyronald · 2018-05-25T23:29:06Z

Just pushed a few changes that you can review. In the mean-time I'll try to figure out how to edit the files while preserving formatting and comments, already found the ConfigurationEditingService which may help I assume (?).

Also, if you feel like you need a more direct / immediate communication you can @ me in https://gitter.im/Microsoft/vscode, I'm online often and should be able to get back to you quickly if I see the notification.

ramya-rao-a · 2018-06-12T22:02:06Z

Hey @reyronald,

Thanks for your patience. Let's work towards getting this feature in the next release.
Coming to your questions..

Do you think it would be worth it to have a transitioning middle state for the button where it is visible but disabled while the async work is being done

Yes, let's transition to something like Adding to workspace recommendations and then when the work is done, it can say Added to workspace recommendations and stay like that in a non-clickable state until either the tab is closed or the VS Code window is closed (which ever is easier)

I created two different classes for each action to keep the logic as isolated as possible, but they ended up looking really similar, are you ok with us leaving it like that or would you rather have some more code reusing? Like unifying them in one Action with conditional statements for each workbench state, have both actions inherit from a new common parent, etc...?

I do think having a single action is better. Have it such that

It always says Add to workspace recommendations
In case of mutli-root set-up, it shows all the folders in the quick-pick. Just like how the Extensions: Configure Recommended Extensions (Workspace Folder) command does. Refer to https://github.com/Microsoft/vscode/blob/release/1.24/src/vs/workbench/parts/extensions/browser/extensionsActions.ts#L1609
This way we will actually end up providing a way to add to workspace folder but not to the entire workspace in the multi-root setup. This is fine and we will be consistent with what the pencil icon on the Workspace recommendations view does.

Do you think we should update the extensions list UI in the sidebar when the action completes successfully to include the green bookmark in the top-left corner immediately?

Ideally yes, but not required. For the green bookmark to appear the item has to be re-rendered which is not trivial. Changing the button text to Added to workspace recommendations is enough to indicate that the operation as completed successfully

Do you agree with the color, placement, and labeling of the buttons?

Color and placement are ok. I've covered labeling above.

Let's have this button show up only if the user has installed this extension. Having this button on every extension (there are thousands of them) can get noisy as well as unnecessary. The chances of someone wanting to recommend an extension that they dont even use is very less. Thoughts?

Is there a task or script to automatically format the code

You can add the setting "editor.formatOnSave": true which will format the code on file save.

reyronald · 2018-06-18T22:05:52Z

Ok, just committed some changes. Like we discussed on Gitter you can review this while I work on the automated tests. I did this in one sitting so there might still be things that can be cleaned-up, I'll review it myself later to see if I catch anything. Here's a run-down of the current functionality:

Button goes from "Add to workspace recommendations" (clickable) to "Adding to workspace recommendations..." (unclickable) to "Added to workspace recommendations" (unclickable).
It is only visible if the current extension is installed and there's at least one folder in the root that doesn't have it recommended yet. In other words, if it's already recommended in every folder of the root,
it is not visible.
In a single-root setup, it's immediately added to the recommendations.
In a multi-root setup, a quick-pick is displayed with only the folders where it is not yet recommended available as options.
In a multi-root setup, the button will go back to "Add to workspace recommendations" (clickable) instead of the unclickable state if there are still folders remaining where the current extension is not yet recommended.
An error or success notification is displayed after the work is done.
Configuration files are created if don't exist, modified otherwise.

I added the IPickOptions.filterPicks to be able to re-use the PICK_WORKSPACE_FOLDER_COMMAND_ID command but only display the folders where the current extension is not yet recommended. However, I do recognize this might be an architectural change so let me know if you rather have this done in a different way or removed altogether.

Looking forward to your comments!

ramya-rao-a · 2018-06-18T22:34:29Z

I added the IPickOptions.filterPicks to be able to re-use the PICK_WORKSPACE_FOLDER_COMMAND_ID command but only display the folders where the current extension is not yet recommended. However, I do recognize this might be an architectural change so let me know if you rather have this done in a different way or removed altogether.

Let's not show the button if the extension is recommended in any of the workspace folders. We can re-visit this if we get feedback about the need to add a recommendation to a workspace folder when it is already recommended via another folder in the same multi-root setup.

ramya-rao-a · 2018-06-19T04:25:29Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+			.then(({ created, content }) => {
+				const folderRecommendations: string[] = (<IExtensionsContent>json.parse(content)).recommendations || [];
+
+				if (folderRecommendations.indexOf(extensionId) !== -1) {


It is possible that the recommendations from the file do not use the same casing as the extension. Therefore make this

if (folderRecommendations.map(e => e.toLowerCase()).indexOf(extensionId.toLowerCase()) !== -1)

ramya-rao-a · 2018-06-19T04:45:47Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+				// TODO:
+				// This will actually overwrite the contents of this key in the file,
+				// removing comments and any additional user modifications.
+				return this.jsonEditingService.write(extensionsFileResource,


We should probably look into how the settings file is edited. It is also a json file. But when new entries are added via the Copy the settings command, comments on the old ones are retained.

Will check on that

Seconded on this because when the extensions.json is initially created, it has comments explaining format and uses for each field. Right now, we overwrite those comments as soon as the file is created in cases where this is called with no extensions.json in existence, which leaves the user with no information about the field structure.

I followed up on this in this comment and the case is not as bad as I initially thought it was: #50419 (comment).

In summary, the file is created if it doesn't exist, in that case there's nothing to overwrite so nothing is "lost". The created file wil have the instructions in comments since those are coming from the template, and will have the added recommendation too.

In the case of an existing files, the instructions or any comments that live outside of the recommendations key are not removed either because we are only replacing the contents of that key in the file and nothing else. In this image you can see the diff of the file when this action is executed. Notice how outside comemnts are preserved:

This TODO is referring to comments inside of the key. Not sure if we should aim to preserve those but wanted to bring it up anyway in case the team decided we should act on it or leave it as is.

ramya-rao-a · 2018-06-19T04:48:54Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+				// Don't enable the button if the selected extension is
+				// recommended in ANY the folders of the current workspace
+				this.enabled = !foldersRecommendationsArray.some((recommendations: string[]) => {
+					return recommendations.some(r => r === this.extension.id);


Lower case both r and this.extension.id when checking.

ramya-rao-a · 2018-06-19T04:56:16Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+		this.class = AddToWorkspaceRecommendationsAction.AddClass;
+		this.enabled = false;
+		this.disposables.push(this.contextService.onDidChangeWorkbenchState(() => this.update()));
+		this.update();


At this point, the extension is not set yet. So the call to update will exit early. Any reason to call update at this point at all? Since we call update in the setter of the extension, isn't that enough?

Valid observation, like you said it's not really necessary. Will remove

ramya-rao-a · 2018-06-19T05:04:04Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+			return null;
+		}
+
+		this.enabled = false;


I thought setting this to false made the action disappear... Is that not the case?

In the case of this action it will only disappear if this.enabled is true and the class is AddClass. If the class is either AddingClass or AddedClass then it'll be dimmed and unclickable instead (but still visible), which is what I'm doing in the next two lines. What really makes the button disappear is the combination of the this.enabled and this.class field. See the changes in the extensionActions.css

.monaco-action-bar .action-item.disabled .action-label.extension-action.enable, .monaco-action-bar .action-item.disabled .action-label.extension-action.reload, +.monaco-action-bar .action-item.disabled .action-label.extension-action.add-to-workspace-recommendations:not(.adding):not(.added), .monaco-action-bar .action-item.disabled .action-label.disable-status.hide, .monaco-action-bar .action-item.disabled .action-label.malicious-status.not-malicious { display: none; }

ramya-rao-a · 2018-06-19T05:18:02Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+			}, err => {
+				this.notificationService.error(err);
+			})
+			.then(() => this.update());


At this point, I am guessing all we need to do is update the label/class and enabled state of the action. If so, why not update them directly here? Calling this.update() would mean the extensions.json files of each folder will be read one more time for no reason.

Yeah I was thinking the same thing actually, I'm also going to update the class and labeling back to the clickable state if the quickPick is cancelled, it would end up looking like this:

.then((pickCancelled: boolean) => { if (pickCancelled) { this.enableButton(); } else { this.notificationService.info(localize('AddToWorkspaceRecommendations.success', 'The extension was recommended in the selected folder.')); this.enabled = false; this.label = AddToWorkspaceRecommendationsAction.AddedLabel; this.class = AddToWorkspaceRecommendationsAction.AddedClass; } }, err => { this.notificationService.error(err); this.enableButton(); });

Look out for that change in the next commit

ramya-rao-a · 2018-06-19T05:28:30Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+		);
+		this.class = AddToWorkspaceRecommendationsAction.AddClass;
+		this.enabled = false;
+		this.disposables.push(this.contextService.onDidChangeWorkbenchState(() => this.update()));


Take the case of an extension for which this action is enabled.
User adds a folder to current workspace and this workspace already recommends the extension.
Now when user runs this action the pickFolder promise will return all workspace folders

I wonder if we should instead listen to the onDidChangeWorkspaceFolders event.

We will have access to whether a workspace was added, removed or changed.
Changed => we don't need to react
Removed => we should update if the current state of the action is disabled
Added => we should update if the current state of the action is enabled

reyronald · 2018-06-19T15:53:50Z

Comitted the trivial changes you suggested. Will check on the JSON editing later today.

reyronald · 2018-06-19T18:43:52Z

@ramya-rao-a On closer inspection, the jsonEditingService service doesn't actually delete comments or formatting that are OUTSIDE of the recommendations key, just anything that's inside because the entire contents of that key are replaced, take a look:

This is the diff of the extension.json after recommending the vscode-icons extensions with the new action, notice how it removed the inner comment and re-indented the vscode-eslint extension, while the rest stayed the same.

This is consistent with what the "Copy to settings" context menu does. It adds ("Copy") or replaces existing keys in the User settings file, so any inner comments/formatting inside objects or array configuration keys get lost too. So it makes me wonder if it's actually possible to preserve those. Do you know of any other action in VSCode where this doesn't happen?

ramya-rao-a · 2018-06-20T22:21:25Z

Hey @reyronald

I had a discussion around the ux of this feature with a few other members of the team today.

On any given day, it is unlikely that more than a few users would be using this feature.
We arrived at the conclusion of not having a dedicated button (one with a very long text at that, which we cant help because recommendation is a big word), for a feature that is not commonly used.

Instead, we can expose this feature as a command. When the command is run it can act on the extension that was opened in the extension editor.

Apologies on the churn. I know I earlier said that the buttons are fine, but further discussions with the team convinced me otherwise.

Thoughts?

reyronald · 2018-06-20T23:13:34Z

No worries! That's part of the process of developing a new feature! I don't mind at all.

And yeah, I did think the labeling we ended up with was a little too long, maybe I would've gone with "Recommend" and a tooltip with a description of the action

reyronald · 2018-06-20T23:18:47Z

Whoops!! I accidentally clicked the close button in the middle of my last comment 🙈 (didn't mean too, was on mobile and everything is closer together there)

as I was saying...

So the command you propose would only be available if there's an extension open in the viewlet, right? This would make the feature a little less discoverable but if you don't mind it then neither do I :), I don't want to have a strong opinion on how the UX should be so I'll let you guys decide that, just let me know if you're settled on it so that I can start making the changes

EDIT: By the way, if the team decides vscode shouldn't have this feature because of how rarely it would be used, that's ok too! No hard feelings :P

ramya-rao-a · 2018-06-21T00:20:34Z

No worries! That's part of the process of developing a new feature! I don't mind at all.

Thanks for understanding!

By the way, if the team decides vscode shouldn't have this feature because of how rarely it would be used, that's ok too!

I do believe in the value of this feature. Especially if we have it as a command, then we can have it applicable to uninstalled extensions as well which now that I think of is the added value.

Right now, the extensions.json file provides you auto-completion feature for installed extensions. Uninstalled extensions are not discoverable from there.

So having this command can help.

So the command you propose would only be available if there's an extension open in the viewlet, right

It should be available when the extension is open in the editor area. Otherwise, I dont see how we can pass the context of the extension.

reyronald · 2018-06-21T00:29:19Z

I do believe in the value of this feature. Especially if we have it as a command, then we can have it applicable to uninstalled extensions as well which now that I think of is the added value.

True! I hadn't thought of that, great! It would indeed be best that way.

It should be available when the extension is open in the editor area. Otherwise, I dont see how we can pass the context of the extension.

Yeah that's what I meant. Cool, so I'm going to work on those changes in the following days. Let me know if anything else comes up 👍

- Button goes from "Add to workspace recommendations" (clickable) to "Adding to workspace recommendations..." (unclickable) to "Added to workspace recommendations" (unclickable). - It is only visible if the current extension is installed and there's at least one folder in the root that doesn't have it recommended yet. In other words, if it's already recommended in every folder of the root, it is not visible. - In a single-root setup, it's immediately added to the recommendations. - In a multi-root setup, a quick-pick is displayed with only the folders where it is not yet recommended available as options. - In a multi-root setup, the button will go back to "Add to workspace recommendations" (clickable) instead of the unclickable state if there are still folders remaining where the current extension is not yet recommended. - An error or success notification is displayed after the work is done. - Configuration files are created if don't exist, modified otherwise.

In the previous commit, the button would be displayed as long as there was at least one folder in the workspace where the extension wasn't recommended. Now, the button will be displayed only if the current extension is not recommended in any of the folders, as suggested in microsoft#50419 (comment)

- Lowercasing the extension ID before comparing - Directly styling & labeling the button on the `run` command after work is done instead of calling `update`. - Fix & delete unnecessary styles

@ramya-rao-a

After a discussion with @ramya-rao-a and other members of the team, it was decided that it wasn't reasonable to have a dedicated button for a feature that's rarely used in such a visible place. So instead, the action will be exposed as a command that will only be available when there's an extension open in the editor area. In contrast with the previous implementation, this has the added benefit of allowing the user to use this action to recommend uninstalled extensions as well.

JacksonKearl · 2018-06-23T03:21:17Z

On another note, I have a question, when do the other two actions that inherit from AbstractConfigureRecommendedExtensionsAction get disposed exactly?

It looks like the event handlers aren't being added to the dispose list. This is likely a bug. onDidChangeWorkspaceFolders returns a disposable but we aren't currently capturing it. If you try pushing that return value to disposables does the behavior improve?

JacksonKearl · 2018-06-23T03:45:22Z

src/vs/workbench/parts/extensions/test/electron-browser/extensionsActions.test.ts

@@ -1198,6 +1198,10 @@ suite('ExtensionsActions Test', () => {
 			});
 	});

+	test(`RecommendToFolderAction`, () => {
+		// TODO: Implement test


You should be able to use 'getWorkspaceRecommendationsinextensionTipsService` to help test this, which will do the file lookups and parsing automatically.

JacksonKearl · 2018-06-23T03:45:59Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+			this.boundAddToRecommendationsContextKey.set(false);
+			const extensionId = (this.editorService.activeEditor as ExtensionsInput).extension.id;
+			serviceAccesor
+				.get(IInstantiationService)


Style nit: Formatting here is pretty different from the above cases, which makes it less immediately obvious that they're all doing the same general thing.

JacksonKearl · 2018-06-23T03:46:06Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+				)
+				.run(extensionId)
+				.then(
+					(quickPickCancelled) => this.boundAddToRecommendationsContextKey.set(quickPickCancelled),


I'd prefer just calling the update function in these cases, to keep the enablement logic a bit more self contained. It is a few file accesses, but this should be happening infrequently enough that that isn't a concern.

What's controlling the visibility of these three commands is not in the instance of the action, it is the bounded ContextKey that's passed to the when property in MenuRegistry.appendMenuItem. Also, all of these actions are instantiated at the moment they are called every time and then immediately go out of scope, they don't stick around. There's nothing we could do in update to handle the enablement logic because of this. In fact, the previous two actions have to repeat the enablement logic here too, it's just not very noticeable because it's a one-liner for both of them (see https://github.com/reyronald/vscode/blob/1b8ff839fb1910d57c68a6516897a06a18aaa9dd/src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts#L1686 and https://github.com/reyronald/vscode/blob/1b8ff839fb1910d57c68a6516897a06a18aaa9dd/src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts#L1690)

The reason why the other two actions DO have an update and try to keep the this.enabled field updated is because they are being reused as UI buttons somewhere else (not just as commands). This is not the case with the new action and setting this.enabled to false has no effect on wether this will show up in the menu or not, that's why I had to move it up from the action itself to here.

Would you like us to put it inside again as public static methods that can be accessed from here or something similar?

@reyronald

The need to disable/hide this action if any of the workspace already has the current extension as recommendation arose when this was a button. As a button which clearly calls to action, we wanted to avoid it being shown unless absolutely required.

Now that this is in the command pallet and comes to the user's sight only when the user explicitly looks for it, I believe we can relax the conditions. If the action is indeed performed when the extension is already recommended, we can show a simple notification saying the same.

I'll push a commit simplifying this.

JacksonKearl · 2018-06-23T03:46:19Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+		const folders = this.workspaceContextService.getWorkspace().folders;
+		const configurationFiles = folders
+			.map(workspaceFolder => workspaceFolder.toResource(paths.join('.vscode', 'extensions.json')));
+		const getFoldersRecommendedExtensionsPromises = configurationFiles.map(configurationFile => {


one line arrow functions can lose the brackets and return function. ( configurationFile => this.getFolderRecommendedExtensions(configurationFile) does the same as this, but a bit more concisely).

Also, I'd look into just chaining these maps together to reduce the clutter of temporary variables.

JacksonKearl · 2018-06-23T03:46:27Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+				// recommended in ANY the folders of the current workspace
+				const extensionId = this.editorService.activeEditor.extension.id.toLowerCase();
+				const enabled = !foldersRecommendationsArray.some((recommendations: string[]) => {
+					return recommendations.some(r => r.toLowerCase() === extensionId);


if we change enabled to something more specific, the comments above wont be needed. someWorkspaceReccomends?

Also, now that this is just a command rather than a button, I could see enabling it whenever any extension folder doesn't have this extension. This does mean you'd need to filter the quick pick or something, so I'd be fine leaving it as is too. Not a big deal.

This was the behavior I aimed for initially and actually comitted a working version of it even when it was a button. Pay attenttion to the sequence of actions in the GIF in this comment and see how the quickPick is filtered #50419 (comment). You can see the code of that in this commit: e261ae5

However @ramya-rao-a suggested we removed it, perhaps because the way I implemented it wasn't the best and we were running out of time. Maybe you can discuss with her the reasoning and we could add it back. There's also the possibility of adding it as an improvement in a new PR if the concern is time and getting it right. I'm available to retake that

JacksonKearl · 2018-06-23T03:46:32Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

@@ -1757,6 +1832,29 @@ export abstract class AbstractConfigureRecommendedExtensionsAction extends Actio
 			}));
 	}

+	protected addRecommendedExtensionToFolder(extensionsFileResource: URI, extensionId: string): TPromise<any> {
+		return this.getOrCreateExtensionsFile(extensionsFileResource)
+			.then(({ created, content }) => {


Don't need to capture created if not using it

JacksonKearl · 2018-06-23T03:46:40Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+		@ICommandService private commandService: ICommandService,
+		@INotificationService private notificationService: INotificationService,
+	) {
+		super(


Style nit: these can all be one line

JacksonKearl · 2018-06-23T03:50:43Z

Only a few minor style issues, functionally it seems to work well. If you're having issues testing it, I'd look in the extension tips service tests, we tend to deal with recommendations more on that side of things, so there might be more helper functions available for your use.

reyronald · 2018-06-23T04:32:26Z

It looks like the event handlers aren't being added to the dispose list. This is likely a bug. onDidChangeWorkspaceFolders returns a disposable but we aren't currently capturing it. If you try pushing that return value to disposables does the behavior improve?

I did check on this at the time and yes, the disposable is captured properly

https://github.com/reyronald/vscode/blob/1b8ff839fb1910d57c68a6516897a06a18aaa9dd/src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts#L1962

But even if it wasn't being captured that wouldn't be the problem anyway because the thing is that the dispose of the action is not being called, so it never gets a chance to dispose the elements of the disposables array. Whatever's supposed to trigger the disposing is either not doing it, not happening, or just happening much later.

…dded programatically

…action

ramya-rao-a · 2018-06-24T00:56:23Z

@JacksonKearl I have moved the comments in the extensions.json file template 1 line above, so the issue with the comments being overridden will not affect newly created extensions.json file, but will affect existing extensions.json files. See cae228c

@reyronald As mentioned in #50419 (comment), the various conditions to disable/enable this action was needed when this was a button, but can be relaxed for the action in the command pallet. 6923709 does this simplification.

Also added a36ea5b that will remove the extension from the unwanted list before adding to the recommendations.

@JacksonKearl, @reyronald Can you both take another look at the PR? If all is good, I'll merge this on Monday and @reyronald can continue to add the tests in another PR.

reyronald

Looks good to me! Great work on it, both UX-wise and code-wise is more intuitive, thanks for jumping in.

I can't approve my own PR though. After it's merged to master then I'll open a new one with the tests.

reyronald · 2018-06-24T02:41:35Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+							key: 'recommendations',
+							value: folderRecommendations
+						},
+						true)


If the first promise succeeds but the second fails the user would endup with the extension removed from unwanted recommendations but not added to the recommended ones. Not sure if we could safely do anything about it right now because the rollback wouldn't be guaranteed either, and the jsonEditingService as it is now apparently can't edit multiple keys in one go. This might be highly unlikely so it's probably fine.

This should be fine. The rejected promise would bubble up as an error and shown in the notification.

reyronald · 2018-06-24T02:46:44Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+						index = i;
+						break;
+					}
+				}


Array.prototype.findIndex() is supported in V8, also is Array.prototype.includes() but the local TypeScript configuration of the project is erroring out on it, maybe there's a valid reason for it but if there isn't probably something for the team to look into later, will make these kind of things easier to write. (Just a comment, not a requested change)

In all the goodness of map, filter, some, I sometimes miss the good old for loop :)
So I am completely happy with this :) :)

reyronald · 2018-06-24T02:49:23Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+						index = i;
+						break;
+					}
+				}


I would do just one .toLowerCase() at the top just to not have to recalculate it again in each iteration. Realistically speaking pretty insignificant given the small size of the arrays and strings though

reyronald · 2018-06-24T02:52:04Z

src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts

+				}
+				const configurationFile = workspaceFolder.toResource(paths.join('.vscode', 'extensions.json'));
+				return this.getFolderRecommendedExtensions(configurationFile).then(recommendations => {
+					if (recommendations.map(e => e.toLowerCase()).indexOf(extensionId.toLowerCase()) > -1) {


This will iterate twice and create a temporary array in memory, that's why I went with Array.prototype.some(). Again, pretty insignificant footprint though

reyronald · 2018-06-24T03:00:02Z

Sorry for the constant back-and-forth, I was always trying to make the UX as smart as possible since I didn't know how far the team wanted to go on it. Next time I'll ask those questions before going ahead and coding them 😛

JacksonKearl · 2018-06-24T18:18:23Z

Oh much simpler now. Agreed that the conditions can be relaxed for actions in the command pallet.

ramya-rao-a · 2018-06-24T18:22:19Z

@JacksonKearl On Monday, can you rename this action to AddToWorkspaceFolderRecommendationsAction and create a new AddToWorkspaceRecommendationsAction which can add the extension to the workspace recommendations in the multi-root set-up.

Basically, we can model these actions after the ConfigureWorkspaceFolderRecommendedExtensionsAction and ConfigureWorkspaceRecommendedExtensionsAction.

Then this feature will be truly complete and we will have a nice story to tell in this release about your work on ignoring recommendations and @reyronald's on adding them

reyronald · 2018-06-24T22:32:55Z

@ramya-rao-a @JacksonKearl A pleasure working with you! Glad this could be merged in time 🎉 🎉

reyronald force-pushed the feat/recommend-ext-action branch from dc1ec25 to 6ae4ae3 Compare May 24, 2018 22:00

weinand assigned sandy081 May 24, 2018

sandy081 self-requested a review May 25, 2018 12:46

sandy081 added the extensions Issues concerning extensions label May 25, 2018

sandy081 added this to the June 2018 milestone May 25, 2018

ramya-rao-a self-assigned this May 25, 2018

ramya-rao-a suggested changes May 25, 2018

View reviewed changes

sandy081 removed their request for review May 28, 2018 12:43

sandy081 removed their assignment May 28, 2018

reyronald force-pushed the feat/recommend-ext-action branch 2 times, most recently from 7b40ef3 to b8dfccf Compare June 18, 2018 22:54

ramya-rao-a suggested changes Jun 19, 2018

View reviewed changes

reyronald force-pushed the feat/recommend-ext-action branch from 6e0fd93 to 2eb85d8 Compare June 19, 2018 18:47

reyronald closed this Jun 20, 2018

reyronald reopened this Jun 20, 2018

reyronald added 3 commits June 21, 2018 21:42

Improvements based on PR suggestions

79467c4

- Lowercasing the extension ID before comparing - Directly styling & labeling the button on the `run` command after work is done instead of calling `update`. - Fix & delete unnecessary styles

reyronald force-pushed the feat/recommend-ext-action branch from e9abb39 to c50cfe0 Compare June 22, 2018 01:48

reyronald force-pushed the feat/recommend-ext-action branch from c50cfe0 to 1b8ff83 Compare June 22, 2018 01:49

JacksonKearl self-assigned this Jun 22, 2018

JacksonKearl self-requested a review June 23, 2018 03:24

JacksonKearl reviewed Jun 23, 2018

View reviewed changes

JacksonKearl suggested changes Jun 23, 2018

View reviewed changes

ramya-rao-a added 4 commits June 23, 2018 17:19

Simplify conditions for AddToRecommendations action

6923709

Move comments so that they dont get erased when recommendations are a…

cae228c

…dded programatically

Remove from unwanted before adding to recommendations

a36ea5b

Merge remote-tracking branch 'origin/master' into feat/recommend-ext-…

7a76dec

…action

ramya-rao-a approved these changes Jun 24, 2018

View reviewed changes

reyronald commented Jun 24, 2018

View reviewed changes

Array improvements

9325d54

JacksonKearl approved these changes Jun 24, 2018

View reviewed changes

Friendly error msg

fa81709

ramya-rao-a merged commit 5eea6bc into microsoft:master Jun 24, 2018

reyronald mentioned this pull request Jun 25, 2018

Auto-completion feature inside extensions.json microsoft/vscode-docs#1692

Merged

github-actions bot locked and limited conversation to collaborators Mar 29, 2020

Add "Recommend" action to extension viewlet #50419

Add "Recommend" action to extension viewlet #50419

Conversation

reyronald commented May 24, 2018

msftclas commented May 24, 2018 • edited Loading

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyronald commented May 25, 2018

reyronald commented May 25, 2018

ramya-rao-a commented Jun 12, 2018 • edited Loading

reyronald commented Jun 18, 2018

ramya-rao-a commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyronald commented Jun 19, 2018

reyronald commented Jun 19, 2018 • edited Loading

ramya-rao-a commented Jun 20, 2018

reyronald commented Jun 20, 2018

reyronald commented Jun 20, 2018 • edited Loading

ramya-rao-a commented Jun 21, 2018 • edited Loading

reyronald commented Jun 21, 2018

JacksonKearl commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacksonKearl commented Jun 23, 2018

reyronald commented Jun 23, 2018

ramya-rao-a commented Jun 24, 2018

reyronald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyronald Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyronald commented Jun 24, 2018

JacksonKearl commented Jun 24, 2018

ramya-rao-a commented Jun 24, 2018 • edited Loading

reyronald commented Jun 24, 2018

msftclas commented May 24, 2018 •

edited

Loading

ramya-rao-a commented Jun 12, 2018 •

edited

Loading

reyronald commented Jun 19, 2018 •

edited

Loading

reyronald commented Jun 20, 2018 •

edited

Loading

ramya-rao-a commented Jun 21, 2018 •

edited

Loading

reyronald Jun 24, 2018 •

edited

Loading

ramya-rao-a commented Jun 24, 2018 •

edited

Loading