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 support to deactivate auto-updating disabled extensions #113155

Merged
merged 9 commits into from
Mar 31, 2021

Conversation

plainerman
Copy link
Contributor

This PR fixes #76879

With the current version, disabled extensions are updated in the background. When manually triggering an update process, the user is explicitly asked if disabled extensions should be updated.

This PR transfers this desired behavior (as stated in issue #76879) to auto-updating extensions. Users can now set extensions.autoUpdateDisabledExtensions and decide whether disabled extensions should be updated or not (the default is false, meaning to not update disabled extensions). Further, this PR includes the required code to ignoring disabled extensions in the update process.

To test this fix, you can install an outdated extension, disable it and see that it won't be updated (unless you set the correct configuration).

@plainerman
Copy link
Contributor Author

plainerman commented Dec 19, 2020

Could you take a look, why the Mac OS build has timed out and maybe restart it?
I saw that multiple other Mac OS builds in other PRs have also failed due to a timeout.

@plainerman
Copy link
Contributor Author

@sandy081 is there anything I can do to help getting the PR reviewed and merged?

@plainerman
Copy link
Contributor Author

plainerman commented Jan 14, 2021

@sandy081 I was wondering whether you already had a chance to look at the PR (and the failing build) and if there is anything I can do to help you merging the PR?

@plainerman
Copy link
Contributor Author

@lszomoru, as you were also involved in this issue and @sandy081 is not responding, I was wondering whether you could take a look at this PR?

@sandy081 sandy081 self-requested a review January 27, 2021 16:37
@sandy081
Copy link
Member

@plainerman Sorry for not responding soon. I will take a look at this in Feb milestone and get back to you with feedback.

@plainerman
Copy link
Contributor Author

@sandy081 I have just resolved merge conflicts that came up and the PR is now ready again to be reviewed.
Can you give me an estimate on when you will have time to look at this?

Thanks for getting back to me.

@plainerman plainerman changed the base branch from master to main February 15, 2021 07:22
@sandy081 sandy081 modified the milestones: February 2021, March 2021 Feb 25, 2021
@plainerman
Copy link
Contributor Author

Hey @sandy081,

I saw that you have pushed this issue back to the march milestone. I was hoping that we could look at this issue soon.
Any updates from your side? Or when you will have time to look at it?

Thanks

@sandy081
Copy link
Member

sandy081 commented Mar 3, 2021

Will take this up in this milestone.

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Also please provide action to toggle this flag in extensions ... menu here:

image

@plainerman
Copy link
Contributor Author

Thank you for the review! I will take a look at it in the next few days and will get back to you.

@plainerman
Copy link
Contributor Author

@sandy081 I have implemented some of the requested changes, but after rebasing my branch against the latest main branch, I have difficulties with node-gyp and cannot compile the project as of now. It will probably take until the following week, until I have time to resolve the remaining issues and look into those errors.

In the meantime, could you point me in the right direction on how I can check when an extension was enabled globally? And possibly, where the best place is to call checkForUpdates() in this case?

In the meantime, I will revert this PR into a draft, and will come back to you.

Thank you

@plainerman plainerman marked this pull request as draft March 13, 2021 12:59
@sandy081
Copy link
Member

To know if extension was enabled globally you can check the extension's enablement state. Eg:

&& (this.extension.enablementState === EnablementState.EnabledGlobally || this.extension.enablementState === EnablementState.EnabledWorkspace)

Listen on enablement changed event (extensionEnablementService.onEnablementChanged) and check if extension is enabled then call checkForUpdates. For eg:

@plainerman
Copy link
Contributor Author

Thank you for the code pointer! I have now resolved all of your suggestions. If there are any further changes, just let me know.

@plainerman plainerman marked this pull request as ready for review March 18, 2021 10:09
@sandy081
Copy link
Member

@plainerman Thanks for the changes. After trying out I felt having two settings for disabling auto updates and disabling auto updates of disabled extensions seems confusing for the user.

Instead I think having one setting that defines what to update would be simple. Change the existing extensions.autoUpdate setting to take enum value "all", "enabled", "none" and defaults to "all". For backward compatibility we can still accept boolean values.

"extensions.autoUpdate": true | false | "all" | "enabled" | "none"

Action in the UI can also be changed to choose among above 3 values.

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

@plainerman Thanks for the changes. After trying out I felt having two settings for disabling auto updates and disabling auto updates of disabled extensions seems confusing for the user.

Instead I think having one setting that defines what to update would be simple. Change the existing extensions.autoUpdate setting to take enum value "all", "enabled", "none" and defaults to "all". For backward compatibility we can still accept boolean values.

"extensions.autoUpdate": true | false | "all" | "enabled" | "none"

Action in the UI can also be changed to choose among above 3 values.

@plainerman
Copy link
Contributor Author

plainerman commented Mar 22, 2021

Thanks @sandy081, I will try to get it ready by tomorrow.

@plainerman
Copy link
Contributor Author

plainerman commented Mar 22, 2021

I have implemented most changes, apart from the UI controls.

I imagine the new change auto update behavior to show a submenu like here:
image

but was not able to find out how to properly create such a menu (as of now). Further, I could not find an instance where only one option can be selected.

And I am not quite sure how to change this negate, to check for === 'none' (similarly in another line for enabled)

}, {
id: MenuId.ViewContainerTitle,
when: ContextKeyAndExpr.create([ContextKeyEqualsExpr.create('viewContainer', VIEWLET_ID), ContextKeyDefinedExpr.create(`config.${AutoUpdateConfigurationKey}`).negate()]),
group: '1_updates',
order: 2

Any suggestions are appreciated but I will look more into it in the next few days.

@sandy081 sandy081 modified the milestones: March 2021, April 2021 Mar 24, 2021
@plainerman
Copy link
Contributor Author

I have now adopted the code to to this three-options approach. A few points I would like to mention:

  1. I believe that the code could be simplified if we remove this backwards compatibility true | false. Is there a way to "ugprade" the users' configurations?
  2. I could not manage to create a submenu like I described in the previous message. As of now it shows the two missing options as individual buttons (see image below). If you would prefer the other option, I would need a pointer into the right direction :)
    image

Thank you @sandy081

@sandy081
Copy link
Member

sandy081 commented Mar 31, 2021

@plainerman Thanks a lot for the changes. You are right that supporting both boolean and string (all, enabled, none) values is complicated. We can remove boolean values and migrate to new values on users machine automatically. But, if settings sync is enabled then it might sync new values to user's other vs code instance which might be still on old version. Even though we do not support old clients I would like to try as much as possible for not making this happen.

Hence I simplified the current proposal by just adding one string value onlyEnabledExtensions to the existing boolean values

image

Supported this with improved UI:

image

image

Added above changes on top of your changes and merged the PR

@sandy081 sandy081 merged commit f775378 into microsoft:main Mar 31, 2021
@plainerman
Copy link
Contributor Author

Wonderful, thank you very much!

@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not update disabled extensions
2 participants