Tail toggle v2 #1241

sjoshid · 2019-11-23T21:58:12Z

Closes #922

Review Checklist

I have responded to reviews and made changes where appropriate.
I have tested the code with cargo test --all / ./rust/run_all_checks.
I have updated comments / documentation related to the changes I made.
I have rebased my PR branch onto xi-editor/master.

2) Notify client about tail enable/disable.

…e dont scroll.

sjoshid · 2019-11-23T22:04:59Z

@cmyr
Tailing works with this branch.
This PR is still not cleaned up. I have a bunch of custom todos which I'll clean up. Also clippy fails with

error: value assigned to `is_tail_event` is never read
   --> core-lib/src/tabs.rs:737:17
    |
737 |         let mut is_tail_event = false;
    |                 ^^^^^^^^^^^^^
    |
    = note: `-D unused-assignments` implied by `-D warnings`
    = help: maybe it is overwritten before being read?

But these arent issues that will stop you from reviewing.

If there is something you want me to do make it easier for you to review...let me know.

# Conflicts: # rust/core-lib/src/watcher.rs

…tail_toggle_v2 � Conflicts: � rust/core-lib/src/watcher.rs

2) Added a check to ignore rpc tail calls for files that dont exist.

…into tail_toggle_v2

rust/core-lib/src/tabs.rs

sjoshid · 2019-12-14T21:35:30Z

rust/core-lib/src/tabs.rs

+            EventKind::Modify(ModifyKind::Any) if event.flag().is_some() => {
+                let flag_kind = event.flag().unwrap();
+
+                if Flag::Ongoing.eq(flag_kind) {


Ongoing events should not be handled here.

sjoshid · 2019-12-14T21:39:26Z

rust/core-lib/src/view.rs

@@ -869,8 +882,30 @@ impl View {
        let height = self.line_of_offset(text, text.len()) + 1;
        let plan = RenderPlan::create(height, self.first_line, self.height);
        self.send_update_for_plan(text, client, styles, style_spans, &plan, pristine);
-        if let Some(new_scroll_pos) = self.scroll_to.take() {
-            let (line, col) = self.offset_to_line_col(text, new_scroll_pos);
+        if !self.is_tail_enabled {


This check makes sure that when user scrolls a tailing file, we dont auto scroll to cursor location. Mimics tail command.

I suppose it'd be worth adding a comment to the source file about this.

sjoshid · 2019-12-14T21:41:15Z

rust/core-lib/src/watcher.rs

+        let mut inner =
+            watcher(tx_event, Duration::from_millis(100)).expect("watcher should spawn");
+        inner
+            .configure(Config::OngoingEvents(Some(Duration::from_millis(50))))


Any mills less than 100 would made sense here.

rust/core-lib/src/client.rs

Cogitri · 2019-12-23T12:36:17Z

rust/core-lib/src/file.rs

+            return Ok(Rope::from(""));
+        }
+
+        let existing_file_info = self.file_info.get(&id).expect("File info must be present");


Using expect here is kind of ugly.

rust/core-lib/src/file.rs

Cogitri · 2019-12-23T12:43:23Z

rust/core-lib/src/file.rs

+        f.seek(SeekFrom::End(0)).map_err(|e| FileError::Io(e, path.as_ref().to_owned()))?;
+
+    let current_position = file_info.tail_position.expect(
+        "tail_position is None. Since we are tailing a file, tail_position cannot be None.",


Again, expect here isn't nice

Cogitri · 2019-12-23T12:44:14Z

rust/core-lib/src/file.rs

+    );
+    let diff = end_position - current_position;
+    let mut buf = vec![0; diff as usize];
+    f.seek(SeekFrom::Current(-(buf.len() as i64))).unwrap();


Isn't buf.len() == diff here?

Also, it'd be nice if we could avoid unwrap

Isn't buf.len() == diff here?

yes.

So it'd make more sense to just use diff here directly I think

Ok. Will make the change.

rust/core-lib/src/file.rs

rust/core-lib/src/tabs.rs

Cogitri · 2019-12-23T12:55:38Z

rust/core-lib/src/tabs.rs

+        };
+
+        let has_changes = self.file_manager.check_file(path, buffer_id);
+        let is_pristine = self.editors.get(&buffer_id).map(|ed| ed.borrow().is_pristine()).unwrap();


Again, unwrapping isn't nice (unless None is a programmer error in which case the called code should probably panic! anyway)

Agree but this code is copied as is from original.

Hm, I guess this returning None doesn't really happen then? @cmyr

Cogitri · 2019-12-23T12:56:57Z

rust/core-lib/src/view.rs

@@ -869,8 +882,30 @@ impl View {
        let height = self.line_of_offset(text, text.len()) + 1;
        let plan = RenderPlan::create(height, self.first_line, self.height);
        self.send_update_for_plan(text, client, styles, style_spans, &plan, pristine);
-        if let Some(new_scroll_pos) = self.scroll_to.take() {
-            let (line, col) = self.offset_to_line_col(text, new_scroll_pos);
+        if !self.is_tail_enabled {


I suppose it'd be worth adding a comment to the source file about this.

Cogitri · 2019-12-23T21:59:34Z

Could you please rebase this on master?

rust/core-lib/src/file.rs

sjoshid · 2019-12-24T03:26:40Z

I have already rebased this on master. You shouldnt get any conflicts.

sjoshid added 4 commits November 10, 2019 09:56

Second attempt at tailing file.

4f437a3

Fixing build error.

511a5d6

1) Appending delta rope to existing rope.

11784cd

2) Notify client about tail enable/disable.

1) When tailing a file, scroll to eof only if it's visible. If not, w…

e9110c0

…e dont scroll.

sjoshid added 7 commits December 7, 2019 10:10

Merge remote-tracking branch 'upstream/master' into tail_toggle_v2

e9d2145

# Conflicts: # rust/core-lib/src/watcher.rs

1) Merging with upstream master.

8bb3d9e

Merge branch 'master' of https://github.com/xi-editor/xi-editor into …

c6385a0

…tail_toggle_v2 � Conflicts: � rust/core-lib/src/watcher.rs

1) Added a new event token used for tailing a file.

93ba5b4

2) Added a check to ignore rpc tail calls for files that dont exist.

Merge branch 'tail_toggle_v2' of https://github.com/sjoshid/xi-editor …

fbce37f

…into tail_toggle_v2

1) Formatting only.

ebd26d2

1) Formatting only. Intellij messing files before commit.

15b5ceb

sjoshid commented Dec 14, 2019

View reviewed changes

rust/core-lib/src/tabs.rs Show resolved Hide resolved

sjoshid commented Dec 14, 2019

View reviewed changes

sjoshid mentioned this pull request Dec 16, 2019

Add tail config xi-editor/xi-mac#484

Closed

4 tasks

Cogitri requested changes Dec 23, 2019

View reviewed changes

rust/core-lib/src/file.rs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tail toggle v2 #1241

Tail toggle v2 #1241

sjoshid commented Nov 23, 2019 •

edited

sjoshid commented Nov 23, 2019 •

edited

sjoshid Dec 14, 2019

sjoshid Dec 14, 2019

Cogitri Dec 23, 2019

sjoshid Dec 23, 2019

sjoshid Dec 14, 2019

Cogitri Dec 23, 2019

Cogitri Dec 23, 2019

Cogitri Dec 23, 2019

Cogitri Dec 23, 2019

sjoshid Dec 23, 2019

Cogitri Dec 23, 2019

sjoshid Dec 24, 2019

Cogitri Dec 23, 2019

sjoshid Dec 23, 2019

Cogitri Dec 23, 2019

Cogitri Dec 23, 2019

Cogitri commented Dec 23, 2019

sjoshid commented Dec 24, 2019 •

edited

Tail toggle v2 #1241

Are you sure you want to change the base?

Tail toggle v2 #1241

Conversation

sjoshid commented Nov 23, 2019 • edited

Review Checklist

sjoshid commented Nov 23, 2019 • 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

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

Cogitri commented Dec 23, 2019

sjoshid commented Dec 24, 2019 • edited

sjoshid commented Nov 23, 2019 •

edited

sjoshid commented Nov 23, 2019 •

edited

sjoshid commented Dec 24, 2019 •

edited