helix icon indicating copy to clipboard operation
helix copied to clipboard

Implement window/showMessageRequest

Open ayoub-benali opened this issue 2 years ago • 17 comments

Picker does already a good job I think

image

Todo:

  • [x] Capture window/showMessageRequest and show the choices to the user
  • [x] Render the message
  • [x] Render the MessageType
  • [x] Handle when popup is dismissed

Any pointers regarding how to render the message on top of the input correctly prompt would be really helpful

Fix https://github.com/helix-editor/helix/issues/4699

ayoub-benali avatar Jan 10 '23 22:01 ayoub-benali

A menu would probably be more appropriate for this than a picker: https://github.com/helix-editor/helix/blob/927fa112ec049e5f40309ffdd57c314897e18bbc/helix-term/src/commands/lsp.rs#L627-L654

the-mikedavis avatar Jan 11 '23 00:01 the-mikedavis

I think it is too small as you can see: image

And it might look worse once I add the message coming with the request

ayoub-benali avatar Jan 11 '23 00:01 ayoub-benali

Here is how it looks like when the message is added in the picker

image

I still need to fix the prompt in this example

ayoub-benali avatar Jan 11 '23 00:01 ayoub-benali

I agree with @ayoub-benali here. The menu seems better suited for "inline" actions and less well suited for a large popup like this.

In fact I originally suggested the picker on matrix.

Nvim also handles this as a larger popup:

image

I think reusing the picker here is nice so there are fewer specical cased UI elements (and the PR is easier to review). Altough it's true that the filtering capabilities of the picker don't really fit here. Perhaps it might make more sense to create a new view that is a copy of the picker, rip out any filtering related logic and functionality and reuse the top line to display the message?

pascalkuthe avatar Jan 11 '23 01:01 pascalkuthe

I just noticed that lsp-workspace-command uses the same picker and as you can see the list can be quite long. The filtering capabilities do help because the user can just start typing rather than having to scroll a long list.

image

window/showMessageRequest could contain similar long action list

ayoub-benali avatar Jan 11 '23 08:01 ayoub-benali

After discussion with @pascalkuthe I moved the LSP message to the right. This way we can better handle messages that could span over one line.

Usually LSP messages are short, but I will enable softwrap once https://github.com/helix-editor/helix/pull/5420 is merged.

For metals this how the build message is displayed:

image

And this is for the file creation:

image

ayoub-benali avatar Jan 13 '23 21:01 ayoub-benali

Is there anything still missing? Otherwise you can mark this PR as ready for review.

pascalkuthe avatar Jan 14 '23 16:01 pascalkuthe

almost, I am just working on sending the response to the server when the file picker is dismissed. I had to add a function to the constructor and update all the call sites ^^

ayoub-benali avatar Jan 14 '23 16:01 ayoub-benali

If your favorite language server doesn't implement this request, you can still test this feature by:

  1. Installing Metals https://github.com/helix-editor/helix/wiki/How-to-install-the-default-language-servers#metals
  2. Cloning this example project and opening via Helix in the root
  3. Opening any .sbt or .scala file, the build message should appear.

Once the build is done or you click Don't show again you will have to delete the .metal directory and open Helix again to force the message again.

ayoub-benali avatar Jan 15 '23 21:01 ayoub-benali

Hi @the-mikedavis, sorry to ping you but Is it possible to give this PR a review ? window/showMessageRequest is required in Metals to import the build otherwise nothing works in scala projects. I made sure to often keep this PR conflict free with master but it is getting harder as more core changes have been merged

ayoub-benali avatar Mar 12 '23 19:03 ayoub-benali

Hi @archseer, any chance to give this a review ? :)

ayoub-benali avatar May 04 '23 15:05 ayoub-benali

Any chance we could get this merged? I would personally find this very useful! The PR seems to be ready for merge after one more review and a rebase on master. We're almost there :)

NomisIV avatar Aug 01 '23 09:08 NomisIV

Hi @NomisIV, I rebased this PR multiple time but it doesn't help as long as @archseer doesn't have time to review. This same applies to https://github.com/helix-editor/helix/pull/5535

ayoub-benali avatar Aug 01 '23 10:08 ayoub-benali

I would really love to see this merged. Thank you very much for your contribution @ayoub-benali!

I am also using Metals for working with Scala.

For now I am building and patching Helix locally like I did with https://github.com/helix-editor/helix/pull/5535, but I am struggling with rebasing and resolving conflicts in this PR with master branch or latest 23.05 tag to do the same.

@ayoub-benali Could you or someone else, please, resolve the conflicts, at least with the latest 23.05 branch?

igor-ramazanov avatar Aug 09 '23 08:08 igor-ramazanov

Can we include this into the next release?

igor-ramazanov avatar Sep 07 '23 06:09 igor-ramazanov

Hello @the-mikedavis, @pascalkuthe and @archseer! :slightly_smiling_face: Sorry to ping you like this, but this PR has been sitting reviewed and approved by 2 maintainers for months now. It would be really great to get this merged, since it's part of the LSP specification and some servers (like for example Scala's Metals mentioned in the original issue) use it to provide core features without which usability and user experience are impacted quite heavily. Any chance to see this progress? Is there anything still needed to be done here? :slightly_smiling_face: Thank you for your work on this amazing editor (and big thanks to the author of this PR for the contribution)! :slightly_smiling_face:

filipwiech avatar Jan 30 '24 23:01 filipwiech

I have tried to rebase this to current master and miserably failed. Currently metals (a scala LSP) is unusable without this PR. I am using neovim as a substitute which is pita.

hakan-demirli avatar Apr 02 '24 19:04 hakan-demirli