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

Implement support for $/progress #176

Closed
silvanshade opened this issue Apr 6, 2020 · 13 comments
Closed

Implement support for $/progress #176

silvanshade opened this issue Apr 6, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@silvanshade
Copy link
Contributor

It would be nice to be able to support progress notifications. I haven't looked into what sort of modifications would be needed for this but it seems like it should have an issue for the sake of completeness.

@ebkalderon ebkalderon added the enhancement New feature or request label Apr 6, 2020
@ebkalderon
Copy link
Owner

Thanks for opening this issue! It's a good idea to track this for the sake of completeness, regardless of priority.

@ebkalderon
Copy link
Owner

ebkalderon commented May 20, 2021

I think it should be pretty trivial to expose a server-to-client progress notification API in the form of a new Client method which uses ProgressTokens to report progress. However, I wonder whether we should also design higher-level API abstractions with better ergonomics, perhaps like this?

#[derive(Default)]
pub struct ProgressDetails {
    pub title: String,
    pub message: Option<String>,
    pub cancellable: Option<bool>,
}

impl Client {
    /// Wraps a stream and reports `$/progress` notifications to the client, using `count` to calculate percentage.
    pub fn progress_count<T, S>(&self, stream: S, count: u64, details: ProgressDetails) -> impl Stream<Item = T> + Send + Unpin + '_
    where
        S: Stream<Item = T> + Send + Unpin;
}

EDIT: Learned some more about the API, and I realize that this snippet isn't really suitable for all use cases. It would have to be redesigned to be like this instead:

#[derive(Default)]
pub struct ProgressDetails {
    pub title: String,
    pub message: Option<String>,
    pub cancellable: Option<bool>,
}

impl Client {
    /// Wraps a stream and reports `$/progress` notifications to the client, using `count` to calculate percentage.
    pub fn progress_count<T, S, F>(&self, stream: S, count: u64, details: F) -> impl Stream<Item = T> + Send + Unpin + '_
    where
        S: Stream<Item = T> + Send + Unpin,
        F: FnMut(&T) -> ProgressDetails;
}

@wiseman
Copy link

wiseman commented Nov 17, 2021

Just mentioning that this would be super helpful for my project.

@ebkalderon
Copy link
Owner

ebkalderon commented Mar 4, 2022

In addition, perhaps we could return a more manual API like this:

impl Client {
    pub fn progress<T, M>(&self, title: T, message: M) -> Progress
    where
        T: Into<String>,
        M: Into<Option<String>>;
}

pub struct Progress<P = Infinite, C = NotCancelable> { ... }

impl<C> Progress<Infinite, C> {
    pub fn bounded(self, start_percentage: u32) -> Progress<Bounded, C>;
}

impl<P> Progress<P, NotCancelable> {
    pub fn cancelable(self, show_cancel_btn: bool) -> Progress<P, Cancelable>;
}

impl<P, C> Progress<P, C> {
    pub async fn begin(self) -> ProgressStarted<P, C>;
}

pub struct ProgressStarted<P, C> { ... }

impl ProgressStarted<Infinite, NotCancelable> {
    pub async fn report<M>(&self, message: M)
    where
        M: Into<Option<String>>;
}

impl ProgressStarted<Infinite, Cancelable> {
    pub async fn report<M>(&self, message: M, show_cancel_btn: bool)
    where
        M: Into<Option<String>>;
}

impl ProgressStarted<Bounded, NotCancelable> {
    pub async fn report<M>(&self, percentage: u32, message: M)
    where
        M: Into<Option<String>>;
}

impl ProgressStarted<Bounded, Cancellable> {
    pub async fn report<M>(&self, percentage: u32, message: M, show_cancel_btn: bool)
    where
        M: Into<Option<String>>;
}

impl<P, C> ProgressStarted<P, C> {
    pub fn token(&self) -> &ProgressToken;

    pub async fn finish(self, message: M)
    where
        M: Into<Option<String>>;
}

The final usage would be quite ergonomic, and also type safe:

impl LanguageServer for Backend {
    async fn completion(&self, _: CompletionParams) -> Result<Option<CompletionResponse>> {
        let progress = self
            .client
            .progress("retrieving completions", "starting work...".into())
            .bounded(0)
            .begin()
            .await;

        for x in something {
            // Do some work...
            let percentage = ...
            progress.report(percentage, "reticulating splines".into()).await;
        }

        progress.finish("done!".into()).await;

        // ...
    }

I'm sure we could bikeshed whether the Into<_> magic above is really necessary. But the core idea is sound, I think.

Any feedback, @silvanshade @wiseman?

@ebkalderon
Copy link
Owner

ebkalderon commented Mar 4, 2022

Now, according to the specification, it's technically possible for a WorkDoneProgressBegin or WorkDoneProgressReport notification to start out with a percentage and/or cancellable field, but as soon as either or both of these are omitted by the server, the client will ignore them and switch to treating the progress as infinite or not cancellable from then on.

As such, maybe we could also add methods to the impl ProgressStarted<..., ...> blocks like:

impl<C> ProgressStarted<Bounded, C> {
    pub fn infinite(self) -> ProgressStarted<Infinite, C>;
}

impl<P> ProgressStarted<P, Cancellable> {
    pub fn not_cancellable(self) -> ProgressStarted<P, NotCancellable>;
}

These methods would allow the user to switch an active progress bar over to being infinite and/or not cancellable at any point, and this would downgrade the ProgressStarted type, changing the type signature of the report() method to match.

@TimJentzsch
Copy link

If you're looking for contributions, I'd be interested in taking a stab at implementing this.
However I have to say that I am rather new at LSP and async Rust.

@ratmice
Copy link

ratmice commented May 4, 2022

FWIW, I implemented it in the following commit just using send_request/send_notification + lsp-types API.
So it doesn't technically seem to require further support methods above and beyond what tower already implements it seems.

ratmice/nimbleparse_lsp@c403506

@oxalica
Copy link

oxalica commented Feb 23, 2023

Is there any progress to this? This functionality is quite necessary when the loading costs more than seconds.

@ratmice
Copy link

ratmice commented Feb 23, 2023

@oxalica I assume it might not be a high priority since you can implement it in terms of the following without the additional ergonomics/friendlier API, as in the commit referenced in my previous comment here.

So if it is really necessary it isn't too difficult to implement on top of what is already working today without the details discussed above.

@dalance
Copy link
Contributor

dalance commented Feb 27, 2023

I implemented progress by reference to nimbleparse_lsp. ( Thanks @ratmice ! )

veryl-lang/veryl@3445583

The implementation is very simple.

  • Add async functions ( progress_start, progress_report and progress_done ) to your backend.
  • Call the functions where you want to.

@ebkalderon
Copy link
Owner

ebkalderon commented Mar 2, 2023

This ticket is currently conflating two different types of progress described by the LSP specification:

  1. Client initiated progress
    1. The client includes a .work_done_progress_params.work_done_token field in the "params" given to the server.
    2. The server sends back $/progress begin/report/end notifications to the client using this work_done_token value.
  2. Server initiated progress
    1. The server sends a window/workDoneProgress/create request (with a server-generated WorkDoneToken) to the client.
    2. If it succeeds, the server sends $/progress begin/report/end notifications to the client using the same WorkDoneToken.
    3. If it fails, the server must not send progress notifications using that WorkDoneToken at all.
    4. The client may send a window/workDoneProgress/cancel notification to the server, requesting the $/progress notification stream with the given WorkDoneToken be canceled.

I'm thinking this issue should be split into two separate tickets for better tracking:

  1. Support client initiated progress
    • Introduce an API resembling this proposal. This lets the server send a stream of $/progress notifications to the client using a client-issued WorkDoneToken.
  2. Support server initiated progress
    • Add a Client::server_progress() method which performs the following steps.
      1. Generates a new unique WorkDoneToken.
      2. Sends a window/workDoneProgress/create request to the client.
      3. If it succeeds, it calls into Client::progress() with the server-generated WorkDoneToken, reusing the API from the previous ticket.
      4. If it fails, it short-circuits with an Err(_) and no $/progress stream may be initiated.
    • Add a LanguageServer::work_done_progress_cancel() trait method (corresponding to window/workDoneProgress/cancel), from which the server may cancel ongoing progress.

Does this sound reasonable to you folks?

@ratmice
Copy link

ratmice commented Mar 2, 2023

Seems good to me, I wasn't actually aware of client side progress!

@ebkalderon
Copy link
Owner

Okay! I've broken this ticket down into #380 and #381 to track both these efforts separately. Feel free to continue further discussion into either of those.

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

No branches or pull requests

7 participants