Added hover support by betterclever · Pull Request #753 · xi-editor/xi-editor

betterclever · 2018-07-18T23:43:28Z

Tracking issues/ PRs: #717 #693 #746

This PR adds support for Hover via Language Server only. It is currently based on work from #750 but will be rebased to master once that is resolved and merged.

Meanwhile this is working with the Client work done by @nangtrongvuon with these changes. The RPC for client has been updated. See the relevant files in this PR.

The changes required in Xi-mac client PR is:

document.sendRpcAsync("request_hover", params: ["request_id": hoverRequestID, "position": ["line": hoverPosition.line, "column": hoverPosition.column]])

PS: I am yet to test out the Range conversion successfully yet since RLS sends it as null and we are not using it on the client either.

cmyr · 2018-07-19T14:36:02Z

+    }
+}
+
+/// Counts the number of utf-16 code units in the given string.


can we delete all this now?

Actually, I currently handled the conversion for Hover only. This is still being used for the update methods. I plan to address them in a separate PR to keep it clean.

cmyr · 2018-07-19T14:40:44Z

Is this ready for review?

cmyr · 2018-07-19T14:52:51Z

+        self.send_request("textDocument/hover", params, Box::new(on_result))
+    }
+
+    pub fn request_definition<CB>(&mut self, view_id: ViewId, position: Position, on_result: CB)


these two functions probably don't belong here?

Removing this bit.

cmyr

Only some nits. Haven't run this version too much yet, will do that shortly.

cmyr · 2018-07-20T14:00:35Z

 use tabs::ViewId;
 use config::Table;
 use styles::ThemeSettings;
-use plugins::rpc::ClientPluginInfo;


cmyr · 2018-07-20T14:02:00Z


 use config::{Table, ConfigDomainExternal};
-use plugins::PlaceholderRpc;
+use plugins::{PlaceholderRpc};


braces not needed

cmyr · 2018-07-20T14:04:57Z

    UpdateStatusItem { key: String, value: String  },
-    RemoveStatusItem { key: String }
+    RemoveStatusItem { key: String },
+    ShowHover { request_id: usize, result: Result<Hover, LanguageResponseError>, rev: u64 }


nit: we can add a trailing , here

cmyr · 2018-07-20T14:20:53Z

 }

+#[derive(Serialize, Deserialize, PartialEq, Eq, Debug)]
+pub struct Position {


I'd remove this, and just include line and col params like we do with the gesture RPC, if only to keep the protocol somewhat consistent.

(in fact, it might make sense for us to be just adding a hover GestureType?)

I added this Position struct since the whole parameter i.e. line/col is optional, it uses caret position as a fallback (this makes more sense for terminal clients). However, adding this as a gesture type is a good possibility but how do terminal client invoke this then?

Okay, good point.

cmyr · 2018-07-20T14:29:05Z

        eprintln!("close view {}", view.get_id());
+
+        if let Some(view_info) = self.view_info.get(&view.get_id()) {
+            let ls_client = self


I'm noticing you have to write these ~4 lines a lot..

I added a helper function that do take rest of the code in a closure to clean this up (see below). Will use that here as well.

cmyr · 2018-07-20T14:40:34Z

+
+#[derive(Serialize, Deserialize, Debug, Clone)]
+#[serde(rename_all = "snake_case")]
+pub struct Position {


These two types will be confusing for people. I've been trying to come up with better names without a lot of luck. maybe CorePosition and ClientPosition?

cmyr · 2018-07-20T14:41:07Z

 }

+#[derive(Serialize, Deserialize)]
+pub struct Range {


What is this used for, again? As in, if this is present what is the client supposed to do with that information?

The client is supposed to highlight this area while hover is active. Thinking again, it makes sense to convert this to Line/Col according to View which client can use to have a tint over that area. What do you think?

@cmyr Views on this?

cmyr · 2018-07-20T14:44:10Z

+        .map(|offset| self.get_position(offset))
+    }
+
+    fn get_position(&mut self, utf8_offset: usize) -> Position {


I would call this something like offset_to_plugin_position.

cmyr · 2018-07-20T14:45:35Z

+        }
+    }
+
+    fn get_utf8_offset_from_position_enum(&mut self, position: PositionEnum) -> usize {


maybe offset_from_plugin_position? I appreciate that this is a different type than above, but at least this fn name communicates what we're doing.

betterclever · 2018-07-20T17:02:40Z

@cmyr I fixed most of the things. @nangtrongvuon PR does not have the above change in RPC request yet so be sure to make a change if running now.

betterclever · 2018-07-23T15:54:41Z

@cmyr this failed due to a storage full error, I think it is unrelated to this work. Maybe restart the build?

cmyr

Okay that's my last batch of nits I think!

cmyr · 2018-07-25T17:00:24Z

 }

+#[derive(Debug, Serialize, Deserialize)]
+pub struct Range {


let's not send the range to the client.

cmyr · 2018-07-25T17:10:22Z

        f(&mut view, editor.get_buffer())
    }

+    fn with_rope<R, F>(&mut self, f: F) -> R


I'd either just use with_view or rename this to with_text.

cmyr · 2018-07-25T17:10:49Z

+        f(editor.get_buffer())
+    }
+
+    fn with_rope_rev<R, F>(&mut self, rev: u64, f: F) -> Option<R>


"with_rev"?

cmyr · 2018-07-25T17:11:21Z

                                                        self.view_id, &key, &value),
-            RemoveStatusItem { key } => self.client.remove_status_item(self.view_id, &key)
+            RemoveStatusItem { key } => self.client.remove_status_item(self.view_id, &key),
+            ShowHover { request_id, result, rev } => {


I would put all the login in something like do_show_hover.

cmyr · 2018-07-25T17:12:14Z

+        }
+    }
+
+    // Gives the requested position in UTF-8 offset format to be sent to plugin


three /// ?

cmyr · 2018-07-25T17:13:12Z

+
+#[derive(Serialize, Deserialize, Debug, Clone)]
+#[serde(rename_all = "snake_case")]
+pub enum LanguageResponseError {


let's use xi_rpc::RemoteError

cmyr · 2018-07-25T17:13:59Z

+#[derive(Serialize, Deserialize, Debug, Clone)]
+#[serde(rename_all = "snake_case")]
+pub struct CorePosition {
+    pub utf8_offset: usize,


maybe just offset? utf8 offsets are our default representation. You could mention that it's utf8 in a doc comment.

cmyr

okay just took a look through and I think this is solid. I'll build and play around tonight, but looks good code-wise.

cmyr

Okay I think this is a good checkpoint. There are still some open questions, but let's get this in and we can iterate from there.

I'm not going to have time to update my autocomplete stuff tonight, but first thing tomorrow I'll merge this and rebase my stuff and we can start working from there.

googlebot added the cla: yes label Jul 18, 2018

nangtrongvuon mentioned this pull request Jul 19, 2018

Added hover UI for front end xi-editor/xi-mac#228

Merged

cmyr reviewed Jul 19, 2018

View reviewed changes

cmyr reviewed Jul 20, 2018

View reviewed changes

betterclever force-pushed the hover-new branch 2 times, most recently from 3712683 to 0a8da37 Compare July 20, 2018 17:01

betterclever force-pushed the hover-new branch from 0a8da37 to e3a7e10 Compare July 21, 2018 16:07

Added hover support

d1e28bf

betterclever force-pushed the hover-new branch from e3a7e10 to e8bf865 Compare July 23, 2018 15:26

betterclever mentioned this pull request Jul 23, 2018

Add Hover + Go-to Definition Request Support #717

Closed

cmyr reviewed Jul 25, 2018

View reviewed changes

betterclever force-pushed the hover-new branch 4 times, most recently from dd60d30 to a2b5ea6 Compare July 25, 2018 19:24

Cleanup

b134832

betterclever force-pushed the hover-new branch from a2b5ea6 to b134832 Compare July 25, 2018 19:45

cmyr reviewed Jul 25, 2018

View reviewed changes

cmyr approved these changes Jul 26, 2018

View reviewed changes

cmyr merged commit 10f4c28 into xi-editor:master Jul 26, 2018

cmyr mentioned this pull request Oct 17, 2018

Roadmap (October 2018) #937

Open

12 tasks

Conversation

betterclever commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betterclever commented Jul 20, 2018

Uh oh!

betterclever commented Jul 23, 2018

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

betterclever commented Jul 18, 2018 •

edited

Loading