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

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

Merged

Conversation

@ParkourKarthik
Copy link
Contributor

commented Aug 9, 2018

This fixes #55114

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

@ramya-rao-a
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

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

@ramya-rao-a

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

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

This comment has been minimized.

Copy link

commented Aug 14, 2018

CLA assistant check
All CLA requirements met.

@ParkourKarthik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

@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)

This comment has been minimized.

Copy link
@ramya-rao-a

ramya-rao-a Aug 14, 2018

Member

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 {

This comment has been minimized.

Copy link
@ramya-rao-a

ramya-rao-a Aug 14, 2018

Member

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

This comment has been minimized.

Copy link
@ramya-rao-a

ramya-rao-a Aug 14, 2018

Member

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

This comment has been minimized.

Copy link
@ParkourKarthik

ParkourKarthik Aug 15, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

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

2 checks passed

VSTS: VS Code 20180815.59 succeeded with issues
Details
license/cla All CLA requirements met.
Details

@ParkourKarthik ParkourKarthik deleted the ParkourKarthik:update-extensions-feedback branch Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.