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

Extension Badge represents number of outdated and reloadrequired extensions #161046

Merged
merged 16 commits into from Sep 21, 2022

Conversation

benibenj
Copy link
Contributor

Whatever style of extension "notification center" we decide to go for, we'll want the badge number to represent also reload required extensions.
I'm making this a seperate PR in advance such that it is easier to review.

@benibenj benibenj self-assigned this Sep 16, 2022
@benibenj benibenj requested review from alexr00 and sandy081 and removed request for alexr00 September 16, 2022 07:48
@benibenj benibenj added this to the September 2022 milestone Sep 16, 2022
@sandy081
Copy link
Member

Another idea would be to add reloadStatus property to IExtension

like outdated property. Then we do not need the two new APIs you added.

@benibenj
Copy link
Contributor Author

The reason why I did not add a reloadRequired property to IExtension is because we need the running extensions which has to be fetched asynchronously. This means the getter would have to be async and for every extension that we check if a reload is required, an async call is made. Due to this, it's better to have two API calls. One that is sync if the client already has the running extensions, and one that is async, for which the async call has to be performed only once instead of for each extension which would be slow.

@sandy081
Copy link
Member

That's right and this is the reason ReloadAction maintains a state of runtime extensions so that they can be accessed sync. ExtensionWorkbenchService can do the same and maintain a state of runtime extensions. Doing so, the service can return the reload state of each extension using the existing state of runtime extension when asked for.

Ideally IExtensionService shall provide sync access to runtime extensions given that it has a change event when they are changed. Then, we do not need to maintain this state in our services. I will talk to AlexD about this later and see if we can get the sync API, until then lets move runtime extensions state to ExtensionWorkbenchService and add reloadStatus property to IExtension

@benibenj
Copy link
Contributor Author

I pushed some changes, but I would prefer to wait until we have sync access to the running extensions before merging. Otherwise, there is either the possibility for a race condition (current situation) or we have to add events which I prefer not to. I think it should be possible to make the running extensions accessible synchronously. Let's discuss with Alex.

@benibenj benibenj merged commit 1fba6ec into main Sep 21, 2022
@benibenj benibenj deleted the benibenj/extensionBadgeNumber branch September 21, 2022 18:40
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2022
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.

None yet

2 participants