Page MenuHomePhabricator

[clangd] Expose FileStatus in LSP.
ClosedPublic

Authored by hokein on Dec 6 2018, 4:45 AM.

Details

Summary

Add an LSP extension "textDocument/fileStatus" to emit file-status information.

Diff Detail

Event Timeline

hokein created this revision.Dec 6 2018, 4:45 AM
hokein updated this revision to Diff 176983.Dec 6 2018, 8:24 AM

Update.

Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.

simark added a subscriber: simark.Dec 14 2018, 9:13 AM

Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.

I agree about not necessarily needing to expose clang internals, but I think the end goal of providing some feedback to the user (still thinking/done) is worthy.

If this is a clangd-specific event, shouldn't the name of the event reflect it? As-is, it looks like an official LSP feature and can be confusing.

Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like clangd:$methodName, i.e. the current extensions would be clangd:textDocument/fileStatus.
WDYT?

hokein updated this revision to Diff 178455.Dec 17 2018, 5:49 AM

Update:

  • avoid using terms that C++ programmers are unfamiliar with.
  • textDocument/fileStatus => textDocument/clangd.fileStatus

Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.

Leaking some details to users is fine -- it can help users build the model of how clangd work, even though they don't understand all details, users will workout "building file" will take a while before "idle", and when the file is idle, clangd is responsive for upcoming requests.
I also agree that we should try to avoid using terms that C++ programmers are unfamiliar with.

Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like clangd:$methodName, i.e. the current extensions would be clangd:textDocument/fileStatus.

Adding clang-specific words to clangd-extension methods sounds good to me (to avoid confusion, the extension textDocument/switchFromHeader confused me before). I'd vote for textDocument/clangd.FileStatus.

Could we add a capability to the init request to check for this extension? We don't want to send those notifications to clients that don't have the code to handle them.

Another observation: when I played around with the previous version of the patch, RunningAction and BuildingAST where constantly "blinking", i.e. it changes faster than I can read it.
This lead to them being irritating and not providing any actual value.

Maybe consider sending an update after some period of time, e.g. 0.5s? (I expect this particular strategy to be a bit complicated and out of scope of this patch, so another alternative is to simply not send them to the clients in the first version). WDYT?

clangd/ClangdLSPServer.cpp
806

It's somewhat easy to miss the clangd in the middle, would personally prefer the clangd.textDocument/fileStatus. But up to you.

clangd/Protocol.h
1002

Why not vector<string>? What's the use of the MessageType?

clangd/TUScheduler.cpp
741

Maybe start with a lower case?
Arguably clangd: queued looks more natural than clangd: Queued

Also consider maybe change this to file is queued to make it clear it's not clangd that's queuing up somewhere.

744

Maybe make it running ${action name}, e.g. running find references?

hokein updated this revision to Diff 179041.Dec 20 2018, 4:36 AM
hokein marked 3 inline comments as done.

address review comments.

Could we add a capability to the init request to check for this extension? We don't want to send those notifications to clients that don't have the code to handle them.

yes, I think we should add it to initializationOptions, since LSP clients don't provide interfaces to customize TextDocumentClientCapabilities.

Another observation: when I played around with the previous version of the patch, RunningAction and BuildingAST where constantly "blinking", i.e. it changes faster than I can read it.
This lead to them being irritating and not providing any actual value.

This depends on the file, for some huge files, BuildingAST RunningAction may take a while, but I agree that for most small files, these two are fast :).

Maybe consider sending an update after some period of time, e.g. 0.5s? (I expect this particular strategy to be a bit complicated and out of scope of this patch, so another alternative is to simply not send them to the clients in the first version). WDYT?

Sounds fair, keeping status bar blinking seems annony to users. Added a FIXME.

clangd/Protocol.h
1002

MessageType indicates the importance of the message. Editors might choose different UI for showing different message (e.g. in vscode, error message are prompted with a red icon; while Info are prompted with a yellow icon).

ilya-biryukov added inline comments.Dec 20 2018, 5:12 AM
clangd/Protocol.cpp
761

Ah, there's a non-zero chance of name clash here in case the protocol implements something similar.
Maybe name the JSON field "clangdFileStatus" to be completely sure this won't clash later.

clangd/Protocol.h
1002

As discussed offline, maybe leave out the 'details' field in the first version?
It does not seem to provide much value as of now. Could be totally useful if we showed a fallback compile command used or similar.

clangd/index/Merge.cpp
137 ↗(On Diff #179041)

Accidental change?

unittests/clangd/IndexTests.cpp
222 ↗(On Diff #179041)

Accidental change?

hokein updated this revision to Diff 179046.Dec 20 2018, 5:25 AM
hokein marked 4 inline comments as done.

Remove details.

hokein added inline comments.Dec 20 2018, 5:26 AM
clangd/index/Merge.cpp
137 ↗(On Diff #179041)

oops, I rebased this branch on top of my another branch. Fixed.

This revision is now accepted and ready to land.Dec 20 2018, 7:30 AM
This revision was automatically updated to reflect the committed changes.

Maybe consider sending an update after some period of time, e.g. 0.5s? (I expect this particular strategy to be a bit complicated and out of scope of this patch, so another alternative is to simply not send them to the clients in the first version). WDYT?

Sounds fair, keeping status bar blinking seems annony to users. Added a FIXME.

I am a bit late to the discussion but isn't this something to be dealt with on the client's side?

I am a bit late to the discussion but isn't this something to be dealt with on the client's side?

That would mean more complex code in every client and more communication between the server and the client, which is not particularly useful.
Doing it on the server means keeping the complexity in one place, this looks strictly better, unless there are actual use-cases for high-frequency updates.