Implement window/showMessageRequest #5492

ayoub-benali · 2023-01-10T22:09:01Z

Picker does already a good job I think

Todo:

Capture window/showMessageRequest and show the choices to the user
Render the message
Render the MessageType
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 #4699

the-mikedavis · 2023-01-11T00:19:43Z

A menu would probably be more appropriate for this than a picker:

helix/helix-term/src/commands/lsp.rs

Lines 627 to 654 in 927fa11

    
           let mut picker = ui::Menu::new(actions, (), move |editor, code_action, event| { 
        
               if event != PromptEvent::Validate { 
        
                   return; 
        
               } 
        
               // always present here 
        
               let code_action = code_action.unwrap(); 
        
               match code_action { 
        
                   lsp::CodeActionOrCommand::Command(command) => { 
        
                       log::debug!("code action command: {:?}", command); 
        
                       execute_lsp_command(editor, command.clone()); 
        
                   } 
        
                   lsp::CodeActionOrCommand::CodeAction(code_action) => { 
        
                       log::debug!("code action: {:?}", code_action); 
        
                       if let Some(ref workspace_edit) = code_action.edit { 
        
                           log::debug!("edit: {:?}", workspace_edit); 
        
                           apply_workspace_edit(editor, offset_encoding, workspace_edit); 
        
                       } 
        
                       // if code action provides both edit and command first the edit 
        
                       // should be applied and then the command 
        
                       if let Some(command) = &code_action.command { 
        
                           execute_lsp_command(editor, command.clone()); 
        
                       } 
        
                   } 
        
               } 
        
           });

ayoub-benali · 2023-01-11T00:29:50Z

I think it is too small as you can see:

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

ayoub-benali · 2023-01-11T00:33:20Z

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

I still need to fix the prompt in this example

pascalkuthe · 2023-01-11T01:09:11Z

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:

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?

ayoub-benali · 2023-01-11T08:45:01Z

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.

window/showMessageRequest could contain similar long action list

ayoub-benali · 2023-01-13T21:25:50Z

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 #5420 is merged.

For metals this how the build message is displayed:

And this is for the file creation:

pascalkuthe · 2023-01-14T16:10:59Z

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

ayoub-benali · 2023-01-14T16:20:48Z

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 · 2023-01-14T16:41:22Z

helix-term/src/ui/picker.rs

 }

 impl<T: Item> Picker<T> {
    pub fn new(
        options: Vec<T>,
        editor_data: T::Data,
        callback_fn: impl Fn(&mut Context, &T, Action) + 'static,
+        on_close_fn: impl Fn(&mut Context) + 'static,


I added this function so that we can run it when the picker is dismissed but since it is not needed in the other context, I don't know if there is a better way

I think this mostly works fine but I think you could merge these two callbacks here and pass None when the picker is closed:

callback_fn: impl Fn(&mut Context, &T, Option<Action>) + 'static,

So callback_fn should be invoked here with None just before close_fn ?

helix/helix-term/src/ui/picker.rs

Lines 619 to 621 in b633139

key!(Esc) | ctrl!('c') => {

return close_fn;

}

EDIT: done in 267af0d and made the item &T optional rather than the action

ayoub-benali · 2023-01-14T16:41:58Z

helix-term/src/ui/picker.rs

@@ -36,6 +36,7 @@ pub const MAX_FILE_SIZE_FOR_PREVIEW: u64 = 10 * 1024 * 1024;
 pub enum PathOrId {
    Id(DocumentId),
    Path(PathBuf),
+    Text(Rope),


This to show the same message in the "preview" part of the file picker

ayoub-benali · 2023-01-15T21:03:02Z

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

Installing Metals https://github.com/helix-editor/helix/wiki/How-to-install-the-default-language-servers#metals
Cloning this example project and opening via Helix in the root
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 · 2023-01-15T22:28:38Z

helix-term/src/ui/picker.rs

@@ -114,7 +125,7 @@ impl<T: Item> FilePicker<T> {
    pub fn new(
        options: Vec<T>,
        editor_data: T::Data,
-        callback_fn: impl Fn(&mut Context, &T, Action) + 'static,
+        callback_fn: impl Fn(&mut Context, Option<&T>, Action) + 'static,


The None case allows us to represent when the picker is dismissed, otherwise we have to define a separate on_close method, ~~you can see the attempt here: 34f1555~~
EDIT: rebasing on master was a mess because I had several commits for commands.rs and picker.rs so I squashed my commits to make my life easier.

ayoub-benali · 2023-01-17T10:49:54Z

helix-term/src/ui/picker.rs

                return close_fn;
            }
            alt!(Enter) => {
-                if let Some(option) = self.selection() {


This isn't needed anymore since the None case is handled in the callback function

- Opening untrusted_files in NewWindow is not supported, yet? - VS Code empty_window equivalent is a scratch buffer, does it have a cwd? - Manage trusted workspaces as a simple list of paths for now? - Plan for local/remote trusted folders and workspaces - Plan for plugin extensions support (disabled, limited) TODO: - [x] Disable LSP and ? in restricted mode - [x] User configuration (new TOML entry) - [ ] Persist user preferences (trusted folder choices) - [x] Show current workspace status (badge?) - [ ] Add a command to quickly toggle workspace status (Manage Workspace Trust) - [x] Use picker as prompt: helix-editor#5492

ayoub-benali · 2023-03-12T19:59:21Z

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

helix-lsp/src/client.rs

helix-term/src/application.rs

helix-term/src/commands.rs

helix-term/src/ui/picker.rs

This is done by taking advantage of the preview panel in the file picker to show the request message I changed the Picker callback function to account for when the picker is dismissed

helix-term/src/commands.rs

the-mikedavis

I left some minor comments but otherwise I think this looks good.

In the long run I think I would prefer a custom component for this kind of UI. Using the picker's preview to show a message feels inconsistent to me: the picker preview is otherwise always used for showing supporting information about the currently selected option instead of context about why the picker is open. When I open a picker my eyes are always on the options first and then the preview if needed. This is a relatively small change though to add a feature so I think we can leave that off for future implementation/discussion

helix-term/src/application.rs

helix-term/src/commands/dap.rs

helix-term/src/ui/mod.rs

helix-term/src/ui/picker.rs

Co-authored-by: Michael Davis <[email protected]>

ayoub-benali · 2023-05-04T15:40:57Z

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

NomisIV · 2023-08-01T09:25:09Z

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 :)

ayoub-benali · 2023-08-01T10:18:41Z

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 #5535

igor-ramazanov · 2023-08-09T08:59:39Z

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 #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 · 2023-09-07T06:43:00Z

Can we include this into the next release?

filipwiech · 2024-01-30T23:17:19Z

Hello @the-mikedavis, @pascalkuthe and @archseer! 🙂
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? 🙂
Thank you for your work on this amazing editor (and big thanks to the author of this PR for the contribution)! 🙂

hakan-demirli · 2024-04-02T19:04:49Z

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.

ayoub-benali marked this pull request as draft January 10, 2023 23:24

the-mikedavis added A-language-server Area: Language server client A-helix-term Area: Helix term improvements labels Jan 11, 2023

ayoub-benali force-pushed the show-message-request branch from 9e05bd3 to a67c281 Compare January 13, 2023 21:22

ayoub-benali marked this pull request as ready for review January 14, 2023 16:34

ayoub-benali commented Jan 14, 2023

View reviewed changes

ayoub-benali commented Jan 15, 2023

View reviewed changes

pascalkuthe mentioned this pull request Jan 16, 2023

Enable http server by default in Metals config #5551

Merged

pascalkuthe added the S-waiting-on-review Status: Awaiting review from a maintainer. label Jan 17, 2023

ayoub-benali force-pushed the show-message-request branch from 267af0d to 3d6a2d3 Compare January 17, 2023 10:42

ayoub-benali commented Jan 17, 2023

View reviewed changes

ayoub-benali force-pushed the show-message-request branch 2 times, most recently from d3e8b0e to ea0abce Compare January 22, 2023 14:50

ayoub-benali force-pushed the show-message-request branch from ea0abce to 567a3d6 Compare January 26, 2023 16:20

pascalkuthe mentioned this pull request Feb 11, 2023

refactor: Merge Picker and FilePicker #5801

Closed

ayoub-benali force-pushed the show-message-request branch from 567a3d6 to 1b47fa3 Compare March 12, 2023 19:51

the-mikedavis reviewed Mar 15, 2023

View reviewed changes

helix-lsp/src/client.rs Outdated Show resolved Hide resolved

helix-term/src/application.rs Outdated Show resolved Hide resolved

helix-term/src/commands.rs Outdated Show resolved Hide resolved

helix-term/src/ui/picker.rs Show resolved Hide resolved

Implement window/showMessageRequest

43708df

This is done by taking advantage of the preview panel in the file picker to show the request message I changed the Picker callback function to account for when the picker is dismissed

exit earlier

d259860

ayoub-benali force-pushed the show-message-request branch from 0d398e3 to d259860 Compare March 19, 2023 13:55

ayoub-benali requested review from the-mikedavis and removed request for pascalkuthe March 19, 2023 14:07

the-mikedavis reviewed Mar 19, 2023

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

Address comment

060667b

ayoub-benali force-pushed the show-message-request branch from 185cc6b to 060667b Compare March 19, 2023 18:37

ayoub-benali requested a review from the-mikedavis March 20, 2023 07:36

the-mikedavis reviewed Mar 21, 2023

View reviewed changes

ayoub-benali and others added 6 commits March 21, 2023 09:57

Update helix-term/src/application.rs

3be3a7a

Co-authored-by: Michael Davis <[email protected]>

Update helix-term/src/commands/dap.rs

3bad5e5

Co-authored-by: Michael Davis <[email protected]>

Update helix-term/src/commands/dap.rs

9745ebc

Co-authored-by: Michael Davis <[email protected]>

Update helix-term/src/commands/dap.rs

c75f3f2

Co-authored-by: Michael Davis <[email protected]>

Update helix-term/src/ui/mod.rs

3e0fa9c

Co-authored-by: Michael Davis <[email protected]>

Address PR comment

33a611f

ayoub-benali requested a review from the-mikedavis March 21, 2023 10:15

the-mikedavis approved these changes Mar 21, 2023

View reviewed changes

the-mikedavis requested a review from archseer March 21, 2023 21:56

pascalkuthe approved these changes May 4, 2023

View reviewed changes

jbrudvik mentioned this pull request Jan 17, 2024

Support PureScript's new spago.yaml configs #9362

Merged

smortime pushed a commit to smortime/helix that referenced this pull request Jul 2, 2024

apply helix-editor#5492 to latest

34837a4

smortime mentioned this pull request Jul 2, 2024

Implement window/showMessageRequest [applies PR 5492 to latest] #11081

Closed

smortime pushed a commit to smortime/helix that referenced this pull request Jul 9, 2024

apply helix-editor#5492 to latest

877c332

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement window/showMessageRequest #5492

Implement window/showMessageRequest #5492

ayoub-benali commented Jan 10, 2023 •

edited

Loading

the-mikedavis commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

pascalkuthe commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

ayoub-benali commented Jan 13, 2023 •

edited

Loading

pascalkuthe commented Jan 14, 2023

ayoub-benali commented Jan 14, 2023

ayoub-benali Jan 14, 2023

pascalkuthe Jan 15, 2023

ayoub-benali Jan 15, 2023 •

edited

Loading

ayoub-benali Jan 14, 2023

ayoub-benali commented Jan 15, 2023

ayoub-benali Jan 15, 2023 •

edited

Loading

ayoub-benali Jan 17, 2023

ayoub-benali commented Mar 12, 2023

the-mikedavis left a comment

ayoub-benali commented May 4, 2023

NomisIV commented Aug 1, 2023 •

edited

Loading

ayoub-benali commented Aug 1, 2023

igor-ramazanov commented Aug 9, 2023

igor-ramazanov commented Sep 7, 2023

filipwiech commented Jan 30, 2024

hakan-demirli commented Apr 2, 2024

Implement window/showMessageRequest #5492

Are you sure you want to change the base?

Implement window/showMessageRequest #5492

Conversation

ayoub-benali commented Jan 10, 2023 • edited Loading

the-mikedavis commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

pascalkuthe commented Jan 11, 2023

ayoub-benali commented Jan 11, 2023

ayoub-benali commented Jan 13, 2023 • edited Loading

pascalkuthe commented Jan 14, 2023

ayoub-benali commented Jan 14, 2023

ayoub-benali Jan 14, 2023

Choose a reason for hiding this comment

pascalkuthe Jan 15, 2023

Choose a reason for hiding this comment

ayoub-benali Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

ayoub-benali Jan 14, 2023

Choose a reason for hiding this comment

ayoub-benali commented Jan 15, 2023

ayoub-benali Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

ayoub-benali Jan 17, 2023

Choose a reason for hiding this comment

ayoub-benali commented Mar 12, 2023

the-mikedavis left a comment

Choose a reason for hiding this comment

ayoub-benali commented May 4, 2023

NomisIV commented Aug 1, 2023 • edited Loading

ayoub-benali commented Aug 1, 2023

igor-ramazanov commented Aug 9, 2023

igor-ramazanov commented Sep 7, 2023

filipwiech commented Jan 30, 2024

hakan-demirli commented Apr 2, 2024

ayoub-benali commented Jan 10, 2023 •

edited

Loading

ayoub-benali commented Jan 13, 2023 •

edited

Loading

ayoub-benali Jan 15, 2023 •

edited

Loading

ayoub-benali Jan 15, 2023 •

edited

Loading

NomisIV commented Aug 1, 2023 •

edited

Loading