Add blocking UI feedback to goto_definition, goto_references.
#6436
Conversation
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
added
C-enhancement
There was a problem hiding this comment.
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
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-statusenabled. 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-statusset, 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:
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?