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

Language Status indicator #1547

Merged
merged 3 commits into from
Jul 28, 2023
Merged

Language Status indicator #1547

merged 3 commits into from
Jul 28, 2023

Conversation

jpogran
Copy link
Contributor

@jpogran jpogran commented Jul 25, 2023

This sets a busy status when finding out the terraform version, which approximates a loading status for the extension because the request for status happens after all the other requests happen "in the background". So, we get a loading bar for the extension in the lower right hand corner.

Screen.Recording.2023-07-20.at.1.23.37.PM.mov

This sets a busy status when finding out the terraform version, which approximates a loading status for the extension because the request for status happens after all the other requests happen "in the background". So, we get a loading bar for the extension in the lower right hand corner.

We could make this a statusbarItem instead for better visibility and/or discoverability.
@jpogran jpogran self-assigned this Jul 25, 2023
@jpogran
Copy link
Contributor Author

jpogran commented Jul 25, 2023

Repointed to main instead of experimental #1540

@jpogran jpogran marked this pull request as ready for review July 25, 2023 18:13
@jpogran jpogran requested a review from a team as a code owner July 25, 2023 18:13
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Nice work! Just two small suggestions.

And one nit: we have three loading indicators, one per version, but currently all are spinning until we fetch all versions.
2023-07-27 16 58 13
If there is no easy fix for that, we can keep it that way.

src/status/installedVersion.ts Outdated Show resolved Hide resolved
src/status/installedVersion.ts Outdated Show resolved Hide resolved
src/features/terraformVersion.ts Outdated Show resolved Hide resolved
@jpogran
Copy link
Contributor Author

jpogran commented Jul 27, 2023

And one nit: we have three loading indicators, one per version, but currently all are spinning until we fetch all versions.

That why I said this approximates not is a loading status indicator.

We have a busy indicator for terraform-ls by using client.onDidChangeState, but since terraform-ls returns initialized almost immediately the indicator goes from busy to running faster than the eye can see. However, we are still processing jobs in the background that prevent new requests from being processed, so we aren't truly 'ready'. So relying just on the server to say it's ready doesn't actually tell us it is.

The request for the terraform version resolution is queued behind all the other work we initiate at start and returns when most of that is finished. This simulates terraform-ls still being 'busy' by having the busy indicator set for both terraform-ls status and the terraform version. This means all three are busy at the same time, but more accurately portrays the current status of terraform-ls.

I could have not set the terraform version as busy and left it blank until it's filled, and only set terraform-ls to busy. This would be only one busy icon, so would be less moving parts. But, I think this leaves the state of the terraform version confusing, is it blank because we couldn't find it or because we're still waiting?

A more accurate implementation could have terraform-ls not return initialized until it has finished the background jobs, which would make the client.onDidChangeState show busy until it was ready. This would require some significant changes to initialization of terraform-ls.

Another accurate implementation would utilize the custom request functionality to have the client send a request to terraform-ls asking about it's status. The scheduler can return how many jobs are pending and we can use that to update the busy indicator. This would require less changes than changing initialization, but still is a bigger lift than this PR.

We had time for this approximation right now.

jpogran and others added 2 commits July 27, 2023 11:35
Co-authored-by: Daniel Banck <dbanck@users.noreply.github.com>
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

👍 That makes sense. Appreciate the additional context!

@jpogran jpogran merged commit 78ff237 into main Jul 28, 2023
@jpogran jpogran deleted the show_language_status branch July 28, 2023 15:35
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants