Skip to content

Notify number of outdated extensions on Check for Extension updates action#56053

Merged
ramya-rao-a merged 7 commits intomicrosoft:masterfrom
ParkourKarthik:update-extensions-feedback
Aug 16, 2018
Merged

Notify number of outdated extensions on Check for Extension updates action#56053
ramya-rao-a merged 7 commits intomicrosoft:masterfrom
ParkourKarthik:update-extensions-feedback

Conversation

@ParkourKarthik
Copy link
Contributor

This fixes #55114

@ramya-rao-a ramya-rao-a self-assigned this Aug 9, 2018
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

This will only show the extensions that have already been deemed outdated. You need to bring back the deleted code that checks for updates. When the promise returned by checkForUpdates is resolved, you can then use the viewlet service

@ramya-rao-a ramya-rao-a added this to the August 2018 milestone Aug 9, 2018
@ParkourKarthik
Copy link
Contributor Author

Thanks for the patience 😄. Hope it works now as expected.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Aug 10, 2018

I tried out the changes. Works very well when I run the Check for Extension Updates from the command pallet and the extensions view is hidden to begin with.

But when the extensions view is open, and I then run the Check for Extension Updates either from command pallet or from the menu, the extensions get the Update to ... label in existing view (which is for enabled extensions) and then the @outdated view is shown. This causes a flicker like perception which is not very nice.

How about the below?

  • Instead of using @outdated as the search term, lets use empty string. That will show the default view which will contain the enabled extensions and the outdated ones will have the Update to ... label.
  • We shouldnt do the above if there are no updates available.
  • We should show the number of extension updates in a notification.

@ParkourKarthik
Copy link
Contributor Author

I agree with your suggestion.
But it would be better if we also show a notification if no updates are available, otherwise, there would be no feedback to the end user on the action as the default view would also be not shown as per your suggestion.

@ParkourKarthik
Copy link
Contributor Author

Also, with what you've said, we need to show the number of extension updates.
But the extensionsWorkbenchService.checkForUpdates() (calls syncWithGallery()) returns a void promise; do we need to count the available extension updates and return as Promise<int> then make use of it?

In order to show the notification, I believe that we might need to add @IDialogService param (hope this would be served automatically) to this ExtensionAction and import vs/base/common/severity. Correct me if I am wrong.

@ramya-rao-a
Copy link
Contributor

But it would be better if we also show a notification if no updates are available,

Yes definitely, my third point (We should show the number of extension updates in a notification) holds even if the number is 0 :)

do we need to count the available extension updates

Yes, once extensionsWorkbenchService.checkForUpdates is done, you can use extensionsWorkbenchService.queryLocal() which will return a list of installed extensions. These extensions have a boolean property called outdated that you can use.

In order to show the notification, I believe that we might need to add @IDialogService param(hope this would be served automatically)

Use the INotificationService instead. Yes, it will be available automatically.

@msftclas
Copy link

msftclas commented Aug 14, 2018

CLA assistant check
All CLA requirements met.

@ParkourKarthik
Copy link
Contributor Author

@ramya-rao-a
I've made the changes as you requested. Though I couldn't test the outdated extensions in real-time, I believe it should work fine.

I've shown a different message when no extensions are outdated - All Extensions are up to date. Let me know if you still think this needs to show the same outdated message with 0 Extensions.

this.notificationService.notify(
{
severity: severity.Info,
message: localize('updatesAvailable', "{0} extensions are outdated.", outdatedCount)
Copy link
Contributor

Choose a reason for hiding this comment

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

extensions updates found sounds more positive than extensions are outdated :)
Also account for the case where only 1 extension is outdated. You will have to use 2 separate msgs.

super(id, label, '', true);
}

private onUpdateNotAvailable(): void {
Copy link
Contributor

@ramya-rao-a ramya-rao-a Aug 14, 2018

Choose a reason for hiding this comment

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

For brevity purposes, I would suggest to combine the onUpdateNotAvailable and onUpdateAvailable functions to a single function notifyUpdateAvailablility and make it take the outdated count as a parameter.
Then based on the count, you can first determine the msg to show and then use a single call to notificationService.notify

Copy link
Contributor

@ramya-rao-a ramya-rao-a Aug 14, 2018

Choose a reason for hiding this comment

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

Also, I just realized that it would be cool if we present an option to Update all extensions in this notification. Then we can skip the change of focus to the extension viewlet.

To do this, instead of notify you will need to use the prompt method. We already have the action UpdateAllAction, you will need to instantiate it using the instantiation service. For reference see https://github.com/Microsoft/vscode/blob/121bb10fd176fc29e790581449518554d53228b2/src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts#L57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I just realized that it would be cool if we present an option to Update all extensions in this notification. Then we can skip the change of focus to the extension viewlet.

I also thought of the same prompt while coding and redirecting to Update all extensions. Would be a nice to have option 👍

@ramya-rao-a
Copy link
Contributor

I got carried away with all the features we kept adding to this PR :)
Below is the case where our idea of having the button to update in the notification fails to be a good experience

  • User clicks on the Check extension updates from the menu in the extensions view
  • The outdated extensions get the Update to .. label, and the notification shows up as well
  • If the user clicks on the Update to .. label on the extension
    At this point our notification is still open asking user to update the extension

I'll push a commit to remove the option. Apologies for the churn.

@ramya-rao-a
Copy link
Contributor

@ParkourKarthik I've made some changes to the PR. Take a look. If you are fine by it as well, I'll merge the PR.

@ramya-rao-a
Copy link
Contributor

Also, since you now know everything about outdated extensions, #22461 would be a good next item if you are interested in picking another item in VS Code :)

@ParkourKarthik
Copy link
Contributor Author

I'm ok with your changes, you can merge it :)

@ramya-rao-a ramya-rao-a changed the title show outdated extension for Check for Extension updates action Notify number of outdated extensions on Check for Extension updates action Aug 16, 2018
@ramya-rao-a ramya-rao-a merged commit 5440820 into microsoft:master Aug 16, 2018
@ParkourKarthik ParkourKarthik deleted the update-extensions-feedback branch August 16, 2018 11:18
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

Unify/clarify "Check for updates" for main code and extensions

3 participants