Add support for measure_width #26

little-dude · 2019-05-28T16:19:20Z

This is based on #24 so that @Cogitri can try it out if they want. Also note that I haven't tested it out yet because I'm not sure how to make xi-core to send me that specific request.

This is the first time we support requests (as opposed to notifications) from the core.

I could not reuse XiEvent because unlike all the events we've been handling until now, we need to return something to the core. As a result, we cannot use handle_event(). As a result, I added a method to Frontend, and renamed XiEvent and handle_event() to XiNotification and handle_notification() respectively.

One thing that annoys me is that with requests, we'll be reproducing the issue described in #21, because we'll add one method to the Frontend trait for each request with a distinct return type. For instance, here I added:

fn handle_measure_width(&mut self, request: MeasureWidthRequest) -> Box<Future<Item = MeasureWidthResponse, Error = ServerError>>;

One solution would be to only have one method called handle_request, and an enum XiRequest similar to the current XiEvent:

fn handle_measure_width(&mut self, request: XiRequest) -> Box<Future<Item = Value, Error = ServerError>>;

The problem is that we lose the strong typing on the return value, and implementors would have to convert their result into a Value which does not seem very nice.

What do you think @ByteBuddha @Cogitri ?

little-dude · 2019-05-28T16:23:26Z

src/frontend.rs

-        unimplemented!();
+        match method {
+            "measure_width" => match from_value::<MeasureWidthRequest>(params) {
+                Ok(req) => Box::new(


It's kind of a shame that we have to re-box something that was already boxed... If I understand correctly, impl Trait would help here, but it's not supported in trait methods, yet. Not sure if there's something else to try.

Cogitri · 2019-05-28T17:39:11Z

Also note that I haven't tested it out yet because I'm not sure how to make xi-core to send me that specific request.

Enabling word_wrap should cause Xi to send you this upon inserting characters.

little-dude · 2019-05-28T20:04:50Z

Ah nice, I'll try this out with xi-term then. I see you already added modify_user_config, so it should be easy enough.

Cogitri · 2019-05-28T23:55:51Z

Feel free to ping me if you need help with that. I guess I won't be able to test it in gxi before #27 is resolved :/

little-dude · 2019-06-04T17:10:44Z

xi-term builds and runs correctly with this. See: xi-frontend/xi-term#101

Cogitri · 2019-06-04T17:47:09Z

Alright, I'll try to add this to gxi later

Cogitri · 2019-06-04T18:29:40Z

The problem is that we lose the strong typing on the return value, and implementors would have to convert their result into a Value which does not seem very nice.

I've thought about this for a bit, but I can't come up with a prettier solution than using one function per Request either, since using dynamic typing for this doesn't sound good, as you've mentioned.

little-dude · 2019-06-05T07:36:36Z

The measure_width thing is not working currently, don't waste your time. I'll let you know when I get it working.

Cogitri · 2019-06-05T07:37:15Z

Oh, okay, thanks :)

little-dude · 2019-06-05T14:25:28Z

So I got things kind of working: it does not crash, but line wrapping is a bit weird, as shown in the screenshot below

Cogitri · 2019-06-05T14:27:19Z

Does xi-term send resize to Xi?

little-dude · 2019-06-05T14:28:35Z

No it doesn't

little-dude · 2019-06-05T14:29:41Z

I'll try with resize thanks @Cogitri

Cogitri · 2019-06-05T14:29:54Z

Then that explains why it "bugs" out like that. You have to send Xi resize so it knows how big the view is in size so it knows when to break a line :)

little-dude · 2019-06-05T14:31:22Z

src/protocol/message.rs

+    match JsonRpcResult::<Value, Value>::deserialize(deserializer)? {
+
+        JsonRpcResult::Result(value) => {
+        println!("{:?}", value);


note to self: remove that

little-dude · 2019-06-05T14:35:40Z

src/client.rs

-                "changes": changes,
-            }),
-        )
+    pub fn modify_user_config(&self, domain: ConfigDomain, changes: ConfigChanges) -> ClientResult<()> {


@Cogitri I think you use modify_user_config quite a lot already in gxi so this will impact you. If having to deal with this bothers you, I'll move this change to a separate PR that can be merged later, just let me know.

Ah, yes, thanks for the heads up! I do use this a lot in gxi - I don't use the file based config of xi anymore but allow configuration via the UI and GSettings (the GLib integrated way to deal with settings). It's a bit of a bother that I'll have to pass in all of my settings into the function now via ConfigChanges instead of just passing what has actually changed to it, but I guess this works for me.

Right now I only pass the changes to xi: https://github.com/Cogitri/gxi/blob/master/src/gxi/src/main_win.rs#L949

Yeah, I'll remove that change from this PR, we'll see later.

This changes the `Frontend` trait, renaming the too generic `handle_event()` into `handle_notification()`, and `XiEvent` into `XiNotification`.

we just ran `cargo fix --edition-idioms`

responses were not deserialized correctly, so this commit fixes it. it also simplifies (de)serialization with the `flatten` attribute.

- derive Default - skip serializing fields that are None - add `word_wrap`

little-dude · 2019-06-06T11:10:49Z

I dropped the changes to modify_user_config and got line wrapping kind of working in xi-term, so I think this PR is good to go:

Cogitri · 2019-06-06T11:12:57Z

Nice, will add to gxi. Thanks for your work! :)

Will look at #29 in a few minutes I suppose.

little-dude · 2019-06-06T12:24:07Z

Yeah I'll look this afternoon (in ~2h probably).

little-dude requested a review from wendivoid May 28, 2019 16:19

little-dude commented May 28, 2019

View reviewed changes

little-dude force-pushed the issue-25 branch 4 times, most recently from b2c69e7 to b37f6d2 Compare June 4, 2019 14:55

little-dude force-pushed the issue-25 branch 2 times, most recently from e0adaaf to 9c70346 Compare June 4, 2019 20:19

little-dude force-pushed the issue-25 branch from 9c70346 to 167d0a5 Compare June 5, 2019 14:19

little-dude mentioned this pull request Jun 5, 2019

responses are not handled correctly #28

Closed

little-dude commented Jun 5, 2019

View reviewed changes

little-dude added 5 commits June 6, 2019 13:07

handle measure_width requests

62175b4

This changes the `Frontend` trait, renaming the too generic `handle_event()` into `handle_notification()`, and `XiEvent` into `XiNotification`.

add logging to help debugging the line cache

21f2e47

fix 2018 edition idiom warnings

55db7df

we just ran `cargo fix --edition-idioms`

refactor message serialization

81734aa

responses were not deserialized correctly, so this commit fixes it. it also simplifies (de)serialization with the `flatten` attribute.

improve ConfigChanges

e4a78e1

- derive Default - skip serializing fields that are None - add `word_wrap`

little-dude force-pushed the issue-25 branch from 167d0a5 to e4a78e1 Compare June 6, 2019 11:08

little-dude merged commit ca3549d into xi-frontend:master Jun 6, 2019

little-dude deleted the issue-25 branch June 6, 2019 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for measure_width #26

Add support for measure_width #26

little-dude commented May 28, 2019 •

edited

Loading

little-dude May 28, 2019

Cogitri commented May 28, 2019

little-dude commented May 28, 2019

Cogitri commented May 28, 2019

little-dude commented Jun 4, 2019

Cogitri commented Jun 4, 2019

Cogitri commented Jun 4, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude commented Jun 5, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude Jun 5, 2019 •

edited

Loading

little-dude Jun 5, 2019

Cogitri Jun 5, 2019

Cogitri Jun 5, 2019

little-dude Jun 5, 2019

little-dude commented Jun 6, 2019

Cogitri commented Jun 6, 2019

little-dude commented Jun 6, 2019

Add support for measure_width #26

Add support for measure_width #26

Conversation

little-dude commented May 28, 2019 • edited Loading

little-dude May 28, 2019

Choose a reason for hiding this comment

Cogitri commented May 28, 2019

little-dude commented May 28, 2019

Cogitri commented May 28, 2019

little-dude commented Jun 4, 2019

Cogitri commented Jun 4, 2019

Cogitri commented Jun 4, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude commented Jun 5, 2019

little-dude commented Jun 5, 2019

Cogitri commented Jun 5, 2019

little-dude Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

little-dude Jun 5, 2019

Choose a reason for hiding this comment

Cogitri Jun 5, 2019

Choose a reason for hiding this comment

Cogitri Jun 5, 2019

Choose a reason for hiding this comment

little-dude Jun 5, 2019

Choose a reason for hiding this comment

little-dude commented Jun 6, 2019

Cogitri commented Jun 6, 2019

little-dude commented Jun 6, 2019

little-dude commented May 28, 2019 •

edited

Loading

little-dude Jun 5, 2019 •

edited

Loading