-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Handle window/showMessage and display it bellow status line #5535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
It's nice that helix now covers this parts of the spec aswell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be separate config for showMessage and progressMessage: progress messages are too verbose and distracting IMO but messages sent through showMessage are usually more important and meant for the user to read.
rust-analyzer
and erlang-ls
are two servers in the wild that behave like this:
- rust-analyzer uses showMessage for things like saying that the config is invalid: https://github.com/rust-lang/rust-analyzer/blob/5306eb06cc2de0566ca789ee3863b78059ee2376/crates/rust-analyzer/src/bin/main.rs#L173-L183
- erlang-ls uses showMessage to say that the config file is missing and how to set one up: https://github.com/erlang-ls/erlang_ls/blob/1a66e8798bb2c33dc1fefa46f054760d8edb767d/apps/els_core/src/els_config.erl#L358-L373
Both use progress messages that are very verbose about internal indexing processes which I don't want to see.
We might also want to consider putting the showMessage contents into a Popup instead of using the statusline so that you have to explicitly clear the notification with C-c
.
What should be the new setting name ? |
Yeah I think you're right about the names. Ideally we should avoid breaking changes but I think progress messages would be better as |
@the-mikedavis So I made the change but I think now a |
@the-mikedavis Is the popup support a must have for this PR or could we added it later depending on the user requests ? |
2d8f044
to
1807e64
Compare
1807e64
to
16d1d88
Compare
Can we include this into the next release? |
(sorry, I thought I was in another PR - still, I really do want this :) would make it a bit easier to use Helix as a client for testing new LSPs 👍🏻) |
This totally fell of my radar for a while - sorry about that! Looking at the most recent changes I think this looks good. We could experiment with a popup separately but I think the statusline is good for now. I've merged master and also added a commit to enable this config option by default. |
…itor#5535) * Handle window/showMessage and display it bellow status line * Enable `editor.lsp.display_messages` by default --------- Co-authored-by: Michael Davis <[email protected]>
…itor#5535) * Handle window/showMessage and display it bellow status line * Enable `editor.lsp.display_messages` by default --------- Co-authored-by: Michael Davis <[email protected]>
This feature is also hidden behind the
display-messages
setting which is used for displaying Progress.I don't think it make sense to create separate setting for this feature as I don't expect a server to send both
Progress
andwindow/showMessage
but I can add one if you prefer.Metals relies on this notification for providing info about the build and is configured here.
Hopefully this will be useful for other language servers.
Here is an example (bottom left) for the message sent by Metals.
Fix #5524