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

Add support for getting current active document and current cursor position #718

Closed
tinaschrepfer opened this issue Apr 10, 2019 · 18 comments
Labels

Comments

@tinaschrepfer
Copy link
Member

Proposal to add support to get the currently active document and cursor position in LSP. Set is not necessary, but get would be good. Usage scenario to come later.

@dbaeumer dbaeumer added feature-request Request for new features or functionality discussion labels Apr 11, 2019
@LukaszMendakiewicz
Copy link

LukaszMendakiewicz commented Apr 11, 2019

LS for languages that are expensive to calculate results for might use this information to prioritize computing results for the text that is on the user screen to make the perceived experience snappier.
The range of text in view currently in the editor should be added to this request.

With information like:

The user has file.ext open, viewing lines 37-75, with caret in (49,5).

The LS could prioritize updating diagnostics as follows:

  1. In the range covering lines 37-75 so they will appear to the user almost immediately.
  2. In the ranges covering lines 1-36 and 76-999 as they may appear with slight delay as the user will be focusing on the active view first.
  3. In files bg1.ext, bg2.ext, etc. as the user will be focusing on the file in the foreground first.

Whenever semantic colorization becomes part of LSP, it would benefit with the same rationale.

The caret position might be used by some LS to infer the current "edit interest" from the user, and pre-fetch parts of the language model appropriately.

@alanz
Copy link

alanz commented Apr 11, 2019

This could probably be piggy backed on to the hover request

@dbaeumer
Copy link
Member

I am not sure if a server can gain a lot having such a request. To my expierence completion and hover requests are so frequently send from the client to a server that they serve as a good indication of where the user currently edits or navigates the code.

This of course requires that client cancel requests when they don't need the informaiton anymore. VS Code does this. For example if VS Code issues a hover request and the user moves the mouse VS Code cancel the first hover request and sends a second one for the new position (see also https://microsoft.github.io/language-server-protocol/specification#cancelRequest).

The are requests where a view port is useful and sematic colorization might be one of them. Another example is folding ranges.

In general we think that a pull model (where the client ask for information instead of the server pushing it) together with a cancel model allows for efficient server implementations (for semantic colorization an editor should cancel a color request if the editor becomes invisible).

@LukaszMendakiewicz I do see the problem that diagnostics are a push model. Since they are usually triggerd on document changes it might be useful to add view port information to the document change notification. Would this together with a pull / cancel model address your concerns?

@LukaszMendakiewicz
Copy link

Completion and hover are indeed approximations of the editor view, but they are not necessarily precise. For example, a user can be scrolling the document with the mouse wheel -- so that viewport changes, but caret remains in the old position, and hover/completion are not invoked; or even browsing through the document with PgUp/PgDn where the completion will not be called at all, and reasonable editor may not be calling hover either.

I don't think this proposal conflicts with the pull model -- the client will still be asking for specific information to retrieve -- however the server will have the ability to prefetch/precook some data so that actual pull requests are served quicker.

I am guessing that you are thinking about having pull model for semantic colorization as well, where the client will be asking for color in specific range (which probably matches the editor viewport)? I suppose that if this is being realized, then it would be a more reasonable approximation of the view information the server can key off than the hover/completion is, and might be sufficient for the server to base of? On the other hand that would form an implicit protocol that clients would need to abide to for some language servers using such optimization, which can get messy in the long term. So considering that, my preference would still be to have an explicit protocol for the editor viewport.

I do not think that adding viewport information to document changes will be much helpful for diagnostics. It is not always the case that diagnostics are only invalidated in the current view where the edits happen, so prioritizing just this chunk of file will degrade to the old behavior (waiting for diagnostics to be computed in the entire file from top to bottom) once the user moves the view.

@dbaeumer
Copy link
Member

I see the scrolling problem without moving the cursor however I couldn't give a clear advice when the server should send such a request. If we think that the viewport information is important for a server I would rather add a notification send from the client to the server when the viewport changes.

@LukaszMendakiewicz
Copy link

I agree that it makes sense to be client -> server notification when the viewport changes. Honestly I thought that this direction is what was being asked for the whole time, until I read your most recent comment and then re-read Tina original message :).

Perhaps @tinaschrepfer could clarify if she sees the need for this to be server -> client request/response, or was it just slight miscommunication.

@nicofuccella
Copy link

Any news on this?
The currently suggested approach is still to use hover and completion approximations of the editor view with all the known limitations?

@dbaeumer
Copy link
Member

dbaeumer commented Aug 5, 2022

We have introduced pull diagnostics which gives you the information about the active file. If necessary, we can add a range to the https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#documentDiagnosticParams to denote the visible range. I think this is still better than exposing UI state to the server.

@ejgallego
Copy link

@dbaeumer as I mentioned here having range support would be great for some uses cases as the ones discussed in #1414

@dbaeumer
Copy link
Member

We had quite some discussion about this when implementing pull diagnostics and we came to the conclusion that sending this information in requests where that information is relevant is the better approach. We do include the information for semantic token and I am open to add it to other requests were necessary.

I will therefore close this request since we don't plan to provide it.

@ejgallego
Copy link

Thanks for the feedback @dbaeumer , IIUC, I've thus opened #1610 to track the possibility of adding a range paramater to document/diagnostics

@dbaeumer dbaeumer removed this from the Backlog milestone Dec 19, 2022
@nayeemrmn
Copy link

we came to the conclusion that sending this information in requests where that information is relevant is the better approach

@dbaeumer Our use case is defining commands server-side that can be used on the client's command palette (these invocations can't include parameters), which are pertinent to the currently active document.

Currently these commands must be defined on the client plugin just to know what the active document is.

  • Being able to define more commands server-side deduplicates work between plugins for various editors.
  • Apparently there's a niche (or movement?) amongst newish editors where there's no extension or plugin system at all, other than an LSP client. They're surprisingly capable except for odd things like this.

The concept of an active document is pretty universal across language clients, so why not?

@dbaeumer
Copy link
Member

I am still not a fan of this although I see the use case. Reasons are:

  • the async nature of the client / server communication. A get of this data might be already out of date when hitting the server
  • pushing UI state to the server feels wrong as well since it is hard to say were to stop (e.g. if we start with the active document, why not the other visibel documents, tabs, ...)

@davidhalter
Copy link

Our use case is defining commands server-side that can be used on the client's command palette (these invocations can't include parameters),

Why can't these invocations include parameters? I assume you are talking about workspace/executeCommand. Couldn't we add the information you are asking for to that endpoint? I feel like that's a much more sensible solution than having a separate endpoint.

@nayeemrmn
Copy link

I am still not a fan of this although I see the use case. Reasons are:

  • the async nature of the client / server communication. A get of this data might be already out of date when hitting the server
  • pushing UI state to the server feels wrong as well since it is hard to say were to stop (e.g. if we start with the active document, why not the other visibel documents, tabs, ...)

@dbaeumer That does make sense to me. How about a client-to-server didChangeActiveDocument notification? It would be less misleading wrt TOCTOU. Though you already mentioned pushing to the server feels wrong... we do kinda have this for tabs via didOpen and didClose.

Why can't these invocations include parameters? I assume you are talking about workspace/executeCommand.

@davidhalter I'm talking about the editor user triggering workspace/executeCommand on their command palette (ctrl+shift+p on vscode), while being able to define said command on the server.

@rwols
Copy link
Contributor

rwols commented Oct 17, 2023

we do kinda have this for tabs via didOpen and didClose.

No we don't. That's an implementation of clients whether they send did{Open,Close} when focusing an open tab.

@myitcv
Copy link

myitcv commented Oct 17, 2023

@dbaeumer

  • the async nature of the client / server communication. A get of this data might be already out of date when hitting the server

Does the same not apply to everything the uses/interacts with the LSP protocol?

AFAICT there is nothing in this proposal that requires anything more than being eventually correct, as is the case with everything to do with the LSP protocol.

  • pushing UI state to the server feels wrong as well since it is hard to say were to stop (e.g. if we start with the active document, why not the other visibel documents, tabs, ...)

What specifically gives you cause to think that we are more likely to open the door to unintended consequences here?

@dbaeumer
Copy link
Member

Does the same not apply to everything the uses/interacts with the LSP protocol?

For all the communication that goes from the client to the server the client needs to ensure that it synchronizes data (e.g. document change first). If the server send a response and the client state has changed it usually discards the request. Some client try to still use the response and adjust it accordingly to the state the client is in.

What specifically gives you cause to think that we are more likely to open the door to unintended consequences here?

The whole discussion around pull versus push diagnostics. We could have fixed the push diagnostics with synching more UI state (tabs, editor layout, ...). However moving over to pull provided a clearer model without having UI state in LSP (and just for clarification: the open/close event are not signaling editor open or close. It is a owner transfers ownership of documents between client and server).

I am not saying that a UI protocol would not be useful. What I am trying to say is that such a model shouldn't go into LSP.

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

No branches or pull requests

10 participants