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 blocking UI feedback to goto_definition, goto_references. #6436

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

askreet
Copy link
Contributor

@askreet askreet commented Mar 25, 2023

Based on some initial work by @pascalkuthe for general blocking UI components.

The main feature added here is user feedback for goto_definition and goto_references. Once initiated, they can be canceled with C-c if the user no longer wishes to wait.

I didn't want to create too much change in one PR, but if accepted this pattern should probably be applied to nearly all LSP commands and some DAP commands as well. I'd be happy to chase that down in a follow-up series of PRs.

Some thoughts on the implementation are below.

Handling layered editor status.

The way editor status is implemented, it's easy to stomp on another async message. In fact, the handler for LSP incoming progress updates (see application.rs:909) previously cleared the status line regardless of whether the user had lsp.display-status enabled. This is likely just a simple bugfix (which enabled my added message not to be cleared prematurely), but it highlights the brittle nature of the current solution.

I'm wondering if longer term something like the LSP status struct that tracks all current background tasks should be leveled up to a general editor concept which can track multiple active activities and perhaps a priority rating of each, so that we can show the most urgent one (your currently requested action is blocked and waiting) vs. lower priority ones (other background LSP status update messages).

Because of all this, if you do have lsp.display-status set, the LSP message I set gets trampled and you can just be left with a dim window for a bit while you wait (or cancel).

Use of editor status vs. new layer.

I didn't pursue this option, but it occurred to me that since we're dimming and freezing the main UI until we can react to the user's request, we have a lot of real estate to play with. We could place a message here for example:

image

Impact on LSP message handling.

One thing I don't understand well is how this impacts the LSP or the message handling. When the cancel message is sent, we de-allocate all the futures (I believe) waiting for the response. Any downside to that? Any LSP commands we should send to ignore the previous request?

pascalkuthe and others added 3 commits March 22, 2023 03:09
Right now job futures are required to be `Send`. However this
requirement is actually unnecessary as the job futures are polled in
the UI loop and not send to another thread with `tokio::spawn`.

The reason this requirement was likely added is so that some utilities
from the `futures` crated (`BoxedFuture`, `boxed()`) could be used.
However these utilities are just trivial convenience functions that
are easily replaced and only contain the `Send` bound for convenience
as most futures are polled with `await` and potentially send across
threads.

The bound was removed so that UI components could be returned as a
future instead of being build synchronously on the main thread.
Currently, there are two ways to deal with a blocking computation needed
to display a UI component: Performing the computation directly in the
UI loop while responding to user feedback (so usually in `commands.rs`).
Or creating a job that polls an async future and displays the UI
component when ready. Both approaches are not a good fit for this
use case. Blocking the UI thread will freeze the editor if the
computation takes longer than expected. Meanwhile, asynchronous jobs
have the opposite problem: When the future takes a long time to
complete there is no direct response. The user may attempt to
repeat the same command or keep editing. At a later point the UI
component will appear and interrupt the workflow. Furthermore, if the
computation consumes lots of resources it will continue doing so in the
background with no way to terminate it.

To address this use case a new job is introduced to the editor: A
`BlockingJob`. A blocking job acts mostly like a normal job: It is an
asynchronous future that is polled in the UI loop yielding a callback.

While a blocking job is running, the editor is softlocked.
The core UI loop is not blocked, and therefore the editor will continue
to respond to external events like signals, keyboard input and screen
size changes. However, all input by the user is ignored, and all UI
elements are dimmed. The user is prompted to press `c-c` to abort the
running job which will cancel the blocking job through a `Cancelation`.

The `Cancelation` can be efficiently checked synchronously
(compiles down to an atomic read) while also acting as a future.
It is up to the caller to ensure that the constructed future responds
quickly to being canceled to avoid any UI delay.
@pascalkuthe pascalkuthe added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 28, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Sorry I kind of forgot about this. Thanks for picking up my work!

One thing I don't understand well is how this impacts the LSP or the message handling. When the cancel message is sent, we de-allocate all the futures (I believe) waiting for the response. Any downside to that? Any LSP commands we should send to ignore the previous request?

That's not an issue. We already set timeouts to lsp commands, we just stop waiting for the servers reply.

I didn't pursue this option, but it occurred to me that since we're dimming and freezing the main UI until we can react to the user's request, we have a lot of real estate to play with. We could place a message here for example

The reason I used the status message is that in most cases the wait time should be pretty short and it would be distracting to have a big popup open everytime IMO. We could definitly experiment here but for an initial version reusing the statusline seems like a pretty decent option.

Multiple status messages conflicting is actually an existing problem that I noticed. I am not sure how to best address that. IMO it would be good to address that seperatly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants