Skip to content

Added status bar rpcs to send to front end#677

Merged
cmyr merged 4 commits intoxi-editor:masterfrom
nangtrongvuon:statusbar_rpc
Jun 6, 2018
Merged

Added status bar rpcs to send to front end#677
cmyr merged 4 commits intoxi-editor:masterfrom
nangtrongvuon:statusbar_rpc

Conversation

@nangtrongvuon
Copy link
Copy Markdown
Member

This PR is a companion PR to xi-mac's #183. This PR adds necessary RPC items and commands to support status bar items, along with a sample plugin that makes use of these features.

Copy link
Copy Markdown
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good aside from a few little nits/documentation requests :)

Comment thread rust/core-lib/src/event_context.rs Outdated
Edit { edit } => self.with_editor(
|ed, _, _| ed.apply_plugin_edit(edit)),
Alert { msg } => self.client.alert(&msg),
AddStatusItem { key, value, alignment } => self.client.add_status_item(viewID, &key, &value, &alignment),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd break these lines up by inserting a newline after the =>.

self.peer.request_is_pending()
}

pub fn add_status_item(&self, key: &str, value: &str, alignment: &str) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three methods should be added to the docs, alongside the other core->frontend methods.

Comment thread rust/sample-plugin/src/main.rs Outdated
}
}
self.0 += 1;
view.update_status_item("my_key", &format!("hello {}", self.0));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after you get to 100 and remove "my_key", this stops doing anything. Overall I like the example, but maybe it could be tweaked a bit?

@nangtrongvuon nangtrongvuon force-pushed the statusbar_rpc branch 2 times, most recently from 82eb0b7 to 2ccbff9 Compare May 31, 2018 10:59
Copy link
Copy Markdown
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very close to being ready, just a few things.

Comment thread docs/docs/frontend-protocol.md Outdated

#### update_status_item

`update_status_item { key: "my_key", value: "hello"}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ` is never closed.

Comment thread rust/core-lib/src/event_context.rs Outdated
cmd: PluginNotification) {
use self::PluginNotification::*;

let viewID = self.view.borrow().view_id;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust convention is to give identifiers snake_case names. (You should get a warning about this when you compile; in general warnings should be addressed unless there's a good reason.)

Comment thread rust/core-lib/src/event_context.rs Outdated
AddStatusItem { key, value, alignment } =>
self.client.add_status_item(viewID, &key, &value, &alignment),
UpdateStatusItem { key, value }
=> self.client.update_status_item(viewID, &key, &value),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rust style nit: the => should be on the same line as the match case; and if the statement for that case is on a newline it should be indented.

UpdateSpans { start: usize, len: usize, spans: Vec<ScopeSpan>, rev: u64 },
Edit { edit: PluginEdit },
Alert { msg: String },
AddStatusItem { key: String, value: String, alignment: String },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an argument that there should be a new Alignment type used here, but I don't think that needs to be blocking.

Comment thread rust/sample-plugin/src/main.rs Outdated
/// and the user inserts an exclamation mark, the plugin will capitalize the
/// preceding word.
struct SamplePlugin;
struct SamplePlugin(usize);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'd like to include this update to the sample plugin in this PR. I'm not exactly sure what the role of the sample-plugin project is, and I do imagine updating that project to show examples of various plugin features, but this particular addition doesn't feel like it is a very useful illustration.

What might make the most sense for things like this, where we want some simple plugin just to demo a new feature, would be to copy sample-plugin into a new folder (out side of xi-editor) and then make a new repo for your new example, put that on your github, or something like this?

Copy link
Copy Markdown
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, lets get this in.

@raphlinus
Copy link
Copy Markdown
Member

Thanks!

@cmyr cmyr merged commit 0cea1bb into xi-editor:master Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants