Major core refactor #613

cmyr · 2018-04-16T01:45:20Z

This PR is a major cleanup and refactor of the core's event handling and bookkeeping code, and represents the two last items in #558 (the RPC refactor has been put on hold for the time being).

There is still some work to finish, but I wanted to put this up for preliminary feedback.

A major goal of this work is to make xi-core easier to reason about, especially for new contributors, and to clear the way for expected GSoC projects.

In addition to that it contains a bunch of general cleanup, including:

resolves reconsider editor/view relationship #371: this decouples View and Editor. Events which only modify view state (such as selection and movement events) are now handled directly by the view. Editor no longer has a reference to a single view, but rather a view is passed into the editor as needed.
Introduces a pattern for writing behaviour tests; that is sending a series of client events to a view, and then verifying that view's state.
Simplifies event flow: all events, from the frontend and from plugins, share a common point of entry.
Clears the way for buffer tags and plugin lifetimes, [RFC] Design for plugin lifetimes and syntax binding #374
Moves file i/o to a new file, setting the groundwork for better text encoding/decoding support, as well as async loading of large files. (Open files asynchronously #197)

todo

There is some work left to do before this is ready to merge:

put trace collection back, and add more trace points
fix fuchsia build
reimplement last few outstanding commands
fix not sending available_plugins rpc
fix issue with handling plugin edits
comprehensive testing (help wanted, see below)
better documentation (where, what?)`

removed plugin support

This branch has (temporarily) removed support for running arbitrary plugins. This will come back as my next piece of work, but for the time being we hard-code launching syntect, if it exists, and ignore anything else.

testing (help wanted)

This PR also includes a new way of writing tests against editing behaviour: in short, you can have a series of edit events processed by a view/editor pair, and then verify that the buffer and selection state is as expected; for instance:

    #[test]
    fn smoke_test() {
        let harness = ContextHarness::new("");
        let mut ctx = harness.make_context();
        ctx.do_edit(EditNotification::Insert { chars: "hello".into() });
        ctx.do_edit(EditNotification::Insert { chars: " ".into() });
        ctx.do_edit(EditNotification::Insert { chars: "world".into() });
        ctx.do_edit(EditNotification::Insert { chars: "!".into() });
        assert_eq!(harness.debug_render(),"hello world!|");
        ctx.do_edit(EditNotification::MoveWordLeft);
        ctx.do_edit(EditNotification::InsertNewline);
        assert_eq!(harness.debug_render(),"hello \n|world!");
        ctx.do_edit(EditNotification::MoveWordRightAndModifySelection);
        assert_eq!(harness.debug_render(), "hello \n[world|]!");
        ctx.do_edit(EditNotification::Insert { chars: "friends".into() });
        assert_eq!(harness.debug_render(), "hello \nfriends|!");
    }

Here the | in the text indicates a cursor position, and [..] indicates a selection.

My goal is to include with this PR a set of tests that cover all of the edit commands for which this is appropriate (in particular commands related to find are not currently supported, although I'd like to add that at some point).

If anyone is interested in helping write tests, PRs against this branch are welcome. Currently these tests live in core-lib/src/editing.rs, although I suspect we'll move them to their own file at some point.

This is a large rewrite of much of the core routing and bookkeeping code. - tabs.rs completely rewritten - better separation of concerns between view and editor - move to context-based event handling

raphlinus

Generally looks good to me, a few comments.

raphlinus · 2018-04-16T17:22:32Z

rust/core-lib/src/client.rs

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


Generally I like to have a one-line //!, which I think helps people navigating the high-level structure. How about this?

//! Requests and notifications from the core to front-ends.

raphlinus · 2018-04-16T17:52:35Z

rust/core-lib/src/client.rs

+                                     }));
+    }
+
+    /// Notify the client that a plugin ha started.


raphlinus · 2018-04-16T17:52:44Z

rust/core-lib/src/client.rs

+                                     }));
+    }
+
+    /// Notify the client that a plugin ha stopped.


raphlinus · 2018-04-16T17:53:28Z

rust/core-lib/src/client.rs

+
+    /// Notify the client that a plugin ha stopped.
+    ///
+    /// `code` is not currently used.


I actually don't know what it's for, maybe "In the future, it might be used to identify X"?

raphlinus · 2018-04-16T18:21:41Z

rust/core-lib/src/edit_types.rs

+
+
+/// Events that only modify view state
+pub (crate) enum ViewEvent {


Extremely minor formatting nit, most sources don't indicate a space between pub and (crate)

raphlinus · 2018-04-16T18:28:45Z

rust/core-lib/src/editor.rs

-        Self::with_text(doc_ctx, config, buffer_id,
-                        initial_view_id, "".to_owned())
+    pub fn new(config: BufferConfig) -> Editor {
+        Self::with_text("".to_owned(), config)


Is the to_owned necessary here, looks like the method has Into already. FWIW, I really like that pattern and am in favor of using it more systematically (and using .into() when we don't want to make the methods generic, to signal that it's a straightforward conversion).

roger. One thing I noticed during this is that rope is always constructed from a string slice; thinking of the big-file case, wouldn't it make sense to have a constructor that took an owned string, and just broke it into chunks without allocation? (I haven't looked at how amenable the builder is to this, just wanted to mention it while on my mind)

So that's a deep line of inquiry. In the existing architecture, each leaf in the rope owns its chunk of string. I considered having a Cow, where the chunk would either be owned or a reference, but it increases complexity (especially managing the lifetime of the string it's referencing into), without clear benefits. The tradeoffs would be very different if we could memory-map a read-only snapshot, but for the most part this is not a primitive we have access to.

One small thing that could be done without refactor is to make the rope builder take Into<Cow>, which would let it avoid allocating a new owned string if the string is already suitable as a leaf.

raphlinus · 2018-04-16T18:29:49Z

rust/core-lib/src/editor.rs

-    }
-
-    fn insert(&mut self, s: &str) {
+    fn insert(&mut self, view: &View, s: &str) {


This would be another candidate for an Into<Rope> arg, but less important here.

raphlinus · 2018-04-16T18:32:50Z

rust/core-lib/src/event_context.rs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::cell::RefCell;


Would like a //!

raphlinus · 2018-04-16T18:35:30Z

rust/core-lib/src/file.rs

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


//! Logic for loading and saving files in the file system, and metadata for those actions.

This also removed some redundant tracepoints, to reduce trace file size (and reduce the interval before which trace storage is exhausted). Also fixes a minor logic error.

This also reinstates some tests that had been disabled during the refactor, and adds some annotations to suppress compiler warnings.

nangtrongvuon · 2018-04-18T01:36:10Z

rust/core-lib/src/edit_types.rs

+            Insert { chars } =>
+                BufferEvent::Insert(chars).into(),
+            DeleteForward =>
+                BufferEvent::Delete(Movement::Left).into(),


There is a bug here - DeleteForward should move Right instead of Left.
Also, it doesn't delete the tab character now. Maybe a specific delete forward command?

Nice, good spot. What do you mean about the tab character?

DeleteBackward (backspace) detects and deleted a tab character if there is one. It used to be that DeleteForward also did the same, but it doesn't do so now.

oooooh, interesting. You mean if there was four spaces from auto-indent, a single backspace would delete them all? I didn't think that forward delete ever did that, I thought it used different logic. it's possible I broke that though, will double check tomorrow.

Yes, IIRC this is also how it works in other editors as well.
Edit: Now that I checked again, I don't think this is the case. Delete forward only deletes a character in many other editors.

googlebot · 2018-04-18T15:12:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-04-18T15:12:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

- Fix a bad mapping in edit_types - Fix a few logic errors around file watching

googlebot · 2018-04-19T21:43:16Z

☹️ Sorry, but only Googlers may change the label cla: yes.

googlebot · 2018-04-19T22:08:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2018-04-19T22:08:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

raphlinus

Ok, took another quick pass, didn't see any serious issues. Understand there is a bit more work as discussed on IRC: yank, starting plugins. The debug wrapping stuff need not be preserved.

raphlinus · 2018-04-19T22:30:01Z

rust/core-lib/src/plugins/mod.rs

@@ -15,185 +15,113 @@
 //! Plugins and related functionality.

 pub mod rpc;
-mod manager;
+//mod manager;


Presumably this will be re-enabled as part of cleanup?

googlebot · 2018-04-20T15:09:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-04-20T15:09:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

cmyr added 4 commits April 15, 2018 19:02

Major core rewrite

eb4516f

This is a large rewrite of much of the core routing and bookkeeping code. - tabs.rs completely rewritten - better separation of concerns between view and editor - move to context-based event handling

More modular i/o

f91c99a

Cleanup and docs

79cf679

Add tools for behaviour tests, and an example

2522acd

googlebot added the cla: yes label Apr 16, 2018

raphlinus reviewed Apr 16, 2018

View reviewed changes

cmyr added 3 commits April 16, 2018 17:09

Re-added trace collection, added trace points

c48f6fd

This also removed some redundant tracepoints, to reduce trace file size (and reduce the interval before which trace storage is exhausted). Also fixes a minor logic error.

Implement most remaining missing methods

0a2a8c0

PR feedback and more cleanup

09b63ee

nangtrongvuon mentioned this pull request Apr 17, 2018

Add editing tests cmyr/xi-editor#3

Closed

cmyr added 3 commits April 17, 2018 18:39

Get compiling on fuchsia again

4fa2f96

Add test for gestures and movement

2cf1a99

Fix missing available_plugins notification

a4f227f

This also reinstates some tests that had been disabled during the refactor, and adds some annotations to suppress compiler warnings.

nangtrongvuon reviewed Apr 18, 2018

View reviewed changes

nangtrongvuon mentioned this pull request Apr 18, 2018

Added tests for upcoming refactor #620

Merged

Added tests for indent/outdent

27ae93e

googlebot added cla: no and removed cla: yes labels Apr 18, 2018

cmyr removed the cla: no label Apr 18, 2018

Fix a few last(?) post-refactor bugs

bd43568

- Fix a bad mapping in edit_types - Fix a few logic errors around file watching

googlebot added the cla: no label Apr 18, 2018

cmyr removed the cla: no label Apr 19, 2018

nangtrongvuon added 2 commits April 19, 2018 13:05

Added few more test cases for indent/outdent

e606f77

Added tests for various delete actions

aba0211

googlebot added the cla: no label Apr 19, 2018

cmyr removed the cla: no label Apr 19, 2018

cmyr changed the title ~~Major core refactor (wip)~~ Major core refactor Apr 19, 2018

Cleanup (fix) pristine tracking

d76408d

googlebot added the cla: no label Apr 19, 2018

cmyr mentioned this pull request Apr 19, 2018

adjust indentation when multiple lines are selected #609

Merged

cmyr added cla: yes and removed cla: no labels Apr 19, 2018

googlebot removed the cla: yes label Apr 19, 2018

cmyr force-pushed the refactor/core-0.3 branch from b0a8d5f to d76408d Compare April 19, 2018 21:53

googlebot added cla: no labels Apr 19, 2018

cmyr removed the cla: no label Apr 19, 2018

raphlinus added the cla: yes label Apr 19, 2018

raphlinus approved these changes Apr 19, 2018

View reviewed changes

Reinstate kill_ring

f674e67

googlebot added cla: no and removed cla: yes labels Apr 20, 2018

cmyr added 2 commits April 20, 2018 11:46

Merge remote-tracking branch 'origin/master' into refactor/core-0.3

8405e6b

Fix broken test compilation

9692634

cmyr removed the cla: no label Apr 20, 2018

raphlinus approved these changes Apr 20, 2018

View reviewed changes

cmyr merged commit 5f94e82 into master Apr 20, 2018

cmyr deleted the refactor/core-0.3 branch April 20, 2018 16:36

cmyr mentioned this pull request May 10, 2018

Weird undo behaviour #652

Closed

jansol mentioned this pull request Sep 21, 2018

Open files asynchronously #197

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major core refactor #613

Major core refactor #613

cmyr commented Apr 16, 2018 •

edited

raphlinus left a comment

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

cmyr Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

raphlinus Apr 16, 2018

nangtrongvuon Apr 18, 2018 •

edited

cmyr Apr 18, 2018

nangtrongvuon Apr 18, 2018

cmyr Apr 18, 2018

nangtrongvuon Apr 18, 2018 •

edited

googlebot commented Apr 18, 2018

googlebot commented Apr 18, 2018

googlebot commented Apr 19, 2018

googlebot commented Apr 19, 2018

googlebot commented Apr 19, 2018

raphlinus left a comment

raphlinus Apr 19, 2018

googlebot commented Apr 20, 2018

googlebot commented Apr 20, 2018



		/// Events that only modify view state
		pub (crate) enum ViewEvent {

Major core refactor #613

Major core refactor #613

Conversation

cmyr commented Apr 16, 2018 • edited

todo

removed plugin support

testing (help wanted)

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nangtrongvuon Apr 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nangtrongvuon Apr 18, 2018 • edited

Choose a reason for hiding this comment

googlebot commented Apr 18, 2018

googlebot commented Apr 18, 2018

googlebot commented Apr 19, 2018

googlebot commented Apr 19, 2018

googlebot commented Apr 19, 2018

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Apr 20, 2018

googlebot commented Apr 20, 2018

cmyr commented Apr 16, 2018 •

edited

nangtrongvuon Apr 18, 2018 •

edited

nangtrongvuon Apr 18, 2018 •

edited