Implement window/showMessageRequest #755
Conversation
src/language_server_protocol.rs
Outdated
| let mut v = Value::Null; | ||
| let result: Option<i64> = self.vim()?.rpcclient.call("s:inputlist", options)?; | ||
| if let Some(answer) = result { | ||
| if answer > 0 && answer < (n_actions - 1) as i64 { |
There was a problem hiding this comment.
The second portion should be something like <= n_actions, right?
Say there are 2 actions, 2 should be a legitimate answer.
There was a problem hiding this comment.
The mapping between actions and parameters to inputlist isn't 1 to 1, they're actually shifted to accommodate the prompt. params.message is actually the index 0, so when the user types 1, they're actually selecting 0.
See screenshot:
We're instructing the user to type from 1 to 3, but the user could actually type 0 and get an invalid option. Vim also supports mouse click, so that's why we can't simply say that the second option is index 0.
There was a problem hiding this comment.
Yes, I get that part.
What I meant is, say in this screenshot, if I select or click 3, it should still be an acceptable answer, right? But in your code, 3 < (3 - 1), which is returning false.
There was a problem hiding this comment.
Oh shoot you're right I got this wrong, my bad.
src/language_server_protocol.rs
Outdated
| let result: Option<i64> = self.vim()?.rpcclient.call("s:inputlist", options)?; | ||
| if let Some(answer) = result { | ||
| if answer > 0 && answer < (n_actions - 1) as i64 { | ||
| let raw_actions: Vec<Value> = try_get("actions", ¶ms)?.unwrap_or_default(); |
There was a problem hiding this comment.
Parse action is not necessary since there is already deserialized instance before hand. Around L1833, you can use iter() instead of into_iter() so you can continue use the instance here.
There was a problem hiding this comment.
Yeah I thought the struct in lsp-type wouldn't contain everything that's required, but I just checked the spec and it does. Will fix the code.
There was a problem hiding this comment.
Question: what's the easiest way to convert MessageActionItem into Value? 🤔
src/language_server_protocol.rs
Outdated
| if let Some(answer) = result { | ||
| if answer > 0 && answer < (n_actions - 1) as i64 { | ||
| let raw_actions: Vec<Value> = try_get("actions", ¶ms)?.unwrap_or_default(); | ||
| v = raw_actions[(answer - 1) as usize].clone(); |
There was a problem hiding this comment.
Please try not indexing from an array. If the index is out of range, it crashes the whole program. use get() instead.
There was a problem hiding this comment.
We're doing a bounds check two lines before 😬 I'll update the code though.
|
Thanks for contributing. Please see my comments inline. |
|
Thanks! |
|
@autozimu hey I didn't get a chance to address all the feedback, sorry about that. I can look into them later today or tomorrow. Thanks for all the feedback! |
|
I will create a commit to clean it up. |
|
Oh cool, thank you and sorry about that: :) |
2 participants
I probably did a lot of things wrong, feedback is appreciated :)
I was able to test this with Scala Metals and everything seems to work fine (see fsouza/dotfiles@914bb5d).