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

clear diagnostics on close document #1135

Merged
merged 4 commits into from
May 3, 2024

Conversation

bzy-debug
Copy link
Contributor

Currently, bash-language-server does not clear diagnostics when user close an document. This causes bad user experience.

This PR fixes this problem, it makes sure that server clear diagnostics when document was closed

@skovhus skovhus enabled auto-merge April 4, 2024 08:51
Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@bzy-debug bzy-debug requested a review from skovhus April 13, 2024 10:50
@bzy-debug
Copy link
Contributor Author

@skovhus

It seems that CI is blocking merge. Can you take a look at it? thanks!

Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.19%. Comparing base (9d5d784) to head (5e540ac).
Report is 1 commits behind head on main.

❗ Current head 5e540ac differs from pull request most recent head fe73990. Consider uploading reports for the commit fe73990 to get more accurate results

Files Patch % Lines
server/src/server.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   81.25%   81.19%   -0.06%     
==========================================
  Files          29       29              
  Lines        1408     1409       +1     
  Branches      334      334              
==========================================
  Hits         1144     1144              
- Misses        217      218       +1     
  Partials       47       47              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skovhus skovhus merged commit 4106b56 into bash-lsp:main May 3, 2024
4 checks passed
@Shane-XB-Qian
Copy link
Contributor

curious to ask: how such 'clear' helped?
just send a empty [] to client? what's the different if just do nothing when closing?
or was it helpless but adding a bit burden?

@bzy-debug
Copy link
Contributor Author

Quote from lsp specification

Diagnostics are “owned” by the server so it is the server’s responsibility to clear them if necessary. The following rule is used for VS Code servers that generate diagnostics:

  • if a language is single file only (for example HTML) then diagnostics are cleared by the server when the file is closed. Please note that open / close events don’t necessarily reflect what the user sees in the user interface. These events are ownership events. So with the current version of the specification it is possible that problems are not cleared although the file is not visible in the user interface since the client has not closed the file yet.
  • if a language has a project system (for example C#) diagnostics are not cleared when a file closes. When a project is opened all diagnostics for all files are recomputed (or read from a cache).

clear here usually means send empty diagnostics (i.e. []) to client, so that client's UI won't show any diagnostics on this file.

AFAIK, bash-lsp currently does not have very much project system concept, so it's resonable to clear diagnostics.

If server does not clear diagnostics, the diagnostics will live forever in client's UI, even after user has deleted the file, util user restart the extension host. This is kind of annoying, in my opinon.

Here's a video illustration:

Screen.Recording.2024-05-03.at.23.29.52.mov

@Shane-XB-Qian
Copy link
Contributor

Shane-XB-Qian commented May 3, 2024 via email

@bzy-debug
Copy link
Contributor Author

Does sending [] to client not clear diagnostics on your side? That works well on my side.

Here's a illustration: (I'm using vscode version 1.89.0 with bash-lsp on main 81125b8)

Screen.Recording.2024-05-04.at.00.45.41.mov

P.S. this clear diagnostics behavior might be different among different editors, since they implement language client differently. If there are any cross-client compatible methods to clear diagnostics, i'd like to hear.

@Shane-XB-Qian
Copy link
Contributor

i am seeing your video now, i think it's probably not a problem,
i am not a vsc user, but i guess it's its same behavior for others langs.
// sending a fake [] to client i supposed it is not a clear, or if you really like to hide it then it maybe a just client option.

@Shane-XB-Qian
Copy link
Contributor

@skovhus so did you plan to keep this, or actually it should be a vscode option to hide it (if there was one) ?

@skovhus
Copy link
Collaborator

skovhus commented May 15, 2024

I haven’t seen this causing any harm. Have you?

@Shane-XB-Qian
Copy link
Contributor

i felt it sent fake [] to client would make it add a bit burden to ACK it,
// or maybe a bit mess thoughg it depends on how client impl it.
and such actually didnot clear anything, but just hide it.
but yea, the impact seems was tiny, so if you're ok, then let it be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants