Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tail toggle? #922

Open
sjoshid opened this issue Oct 14, 2018 · 24 comments · May be fixed by #1241 or xi-editor/xi-mac#486
Open

tail toggle? #922

sjoshid opened this issue Oct 14, 2018 · 24 comments · May be fixed by #1241 or xi-editor/xi-mac#486

Comments

@sjoshid
Copy link
Contributor

sjoshid commented Oct 14, 2018

Often times I have felt the need to tail a file for some time and then stop and start again. Would it be possible for Xi core to support tailing a file on demand? Of course there are other ways to do this, like open a terminal within the editor and tail from there. But Im thinking of a tail mode in Xi which can be turned on/off.

@cmyr
Copy link
Member

cmyr commented Oct 15, 2018

This is actually something we've discussed; a very good use case for xi right now would be as a log viewer, and reading long log files is a good stress test.

@jansol
Copy link
Collaborator

jansol commented Oct 15, 2018

How far does our autoreload get us here? I imagine it'd work fine (minus staying scrolled to the bottom) until the file starts taking noticeable time to read.

@cmyr
Copy link
Member

cmyr commented Oct 15, 2018

Yea, our autoreload is doing a whole-file read, so it wouldn't be great in the huge-log case, but it's a reasonable place to start.

I could imagine a 'log mode', which would automatically adjust the scroll as new content arrived, but would disable itself if the user manually scrolled elsewhere (and re-enable itself if the user manually scrolled back to the bottom of the buffer?)

@sjoshid
Copy link
Contributor Author

sjoshid commented Oct 16, 2018

@cmyr Log mode behavior you are talking about is exactly what I had in mind.
I would like to give this a shot.

@cmyr
Copy link
Member

cmyr commented Oct 17, 2018

@sjoshid cool, feel free to ask questions here as they arise. 👍

@sjoshid
Copy link
Contributor Author

sjoshid commented Oct 30, 2018

Just an update. Nothing super important. I was able to setup notify feature and see how current autoreload works. Im still learning concepts widely used like Editor, View, Buffer, Ropes and so on.
Hopefully I will provide something substantial soon.

@cmyr
Copy link
Member

cmyr commented Oct 30, 2018

@sjoshid cool, glad you're making progress! If you have any questions I'm happy to help.

@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 4, 2018

@cmyr Thanks for offering to help.
One area of improvement I see is the method reload in editor.rs. I am wondering why this method reloads the entire text instead of just appending the last line to current buffer? How hard is it to extract last line from Rope (text of file being watched) and append it to current buffer?
Maybe the answer lies in the comment (method handle_open_file_fs_event of tabs.rs)
// this is ugly; we don't map buffer_id -> view_id anywhere
// but we know we must have a view.
but can't make any sense of it. Do you mind explaining a bit more?
As you can tell Im all over the place so any tips/starters would be helpful. This doc CRDT is interesting but pretty heavy for a weekend. Not sure if I need it for this task.

@cmyr
Copy link
Member

cmyr commented Nov 6, 2018

@sjoshid Currently, reload doesn't reload the entire text; it computes the delta between the new text and the old, and reloads just that.

The problem is that we don't necessarily know what part of the buffer has changed, so we have to figure that out.

the way that tail normally works (I believe) is that it keeps track of its position in the buffer, and then when the buffer changes it reads just from the previous offset to EOF, and it assumes that the file is only being appended to. For instance if you open a file with tail -f and then make an edit near the beginning, you will get unexpected results.

@cmyr
Copy link
Member

cmyr commented Nov 6, 2018

So basically, if a file is open in 'tail' mode, you want to note that somewhere, and then you want to have different logic in handle_open_file_fs_event that just reads the new bytes. Although to be honest, I think that the current reload performance should be good, and the behaviour will be more correct.

@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 8, 2018

Great. That helps. Will focus on handle_open_file_fs_event.

sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 13, 2018
@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 20, 2018

@cmyr
I managed to get delta of changes happening when tailing a file. When tail mode is on, I keep changing the location of cursor and read only the changed bytes from the end of the file. I guess I have to append the delta to rope for me to see the entire file and not just the changes. Following gif sums up the issue Im talking about.
tail

@cmyr
Copy link
Member

cmyr commented Nov 20, 2018

yes, you will need to generate a delta with the newly loaded contents and apply that to the engine. If you have a Rope that contains the contents that you'll want to append, you'll want to do something like this:

// in editor.rs
fn tail_append(&mut self, tail: Rope) {
    let buf_end = self.text.len();
    let mut builder = DeltaBuilder::new(buf_end);
    builder.replace(buf_end..buf_end, tail);
    self.apply_delta(builder.build());
}

sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
Tailing a file with notify feature.
@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 25, 2018

@cmyr
Latest commit (will remove info!s before PR) will give you a brief idea of how Im doing this. If I have not followed Rust/xi rules please point them out.

One issue Im still working on: Xi tails even when I run without notify. This condition if cfg!(feature = "notify") is always true no matter I run xi with or without notify. Looks like some compile magic I dont know but Im looking into this.

@jansol
Copy link
Collaborator

jansol commented Nov 25, 2018

I'm somewhat surprised that even compiles as I've only ever seen cfg() in attributes. But yes, that refers to cargo features which are enabled or disabled at build time, not xi's configuration file.

@cmyr
Copy link
Member

cmyr commented Nov 25, 2018

@sjoshid Can you open a PR with this, and then I'll take a look?

sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
Notify shouldn't be here. It was already under core-lib.
sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 25, 2018
@sjoshid sjoshid mentioned this issue Nov 25, 2018
@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 26, 2018

I created a PR but it is failing because of rustfmt. So I tried formatting using that but ran into another issue.
Error: Decoding config file failed:
invalid type: string "Max", expected a boolean for key use_small_heuristics
Please check your config file.

Rustfmt config documentation is correct. Looks like an issue with rustfmt.

Anyone else facing this error?

@cmyr
Copy link
Member

cmyr commented Nov 26, 2018

I suspect this is related to the version of rust you're running; we run 1.29.2 in CI, you can set this for the project with rustup override set 1.29.2.

edit: and thanks for the PR, i'll take a look!

sjoshid added a commit to sjoshid/xi-editor that referenced this issue Nov 26, 2018
@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 26, 2018

That was it. Thanks.

@cmyr
Copy link
Member

cmyr commented Nov 26, 2018

@sjoshid Okay, so I think that for now it makes more sense to have 'tail' be something that we can turn on for already-open files; this should be simpler to implement and test; I'd just add a debug_toggle_tail RPC, and use that to enable/disable the feature.

I'm not sure what's going on with notify, however I wouldn't use that feature to determine whether this is enabled; notify should be set all the time, the only reason it is behind a feature is because there's no support for filesystem events on fuchsia.

Another question here is whether or not 'tail' implies that a file is read-only. Realistically, I think it does; we can't reload changes if we've changed the file ourselves, so I don't think it makes sense for us to allow editing in this case.

Adding 'read-only' support would be a feature in itself, (and there's an old issue at #196) so it's out of scope for this PR, but something to keep in mind.

In any case, I think a good first goal for this work is to have a 'Tail Toggle' item in the Debug menu of xi-mac that will start tailing the currently open file. I think once that is working we can think more seriously about what it will take to get this merged.

Sound reasonable?

@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 26, 2018

Sounds good.
Some responses below:

I'm not sure what's going on with notify, however I wouldn't use that feature to determine whether this is enabled; notify should be set all the time, the only reason it is behind a feature is because there's no support for filesystem events on fuchsia.

I did fiddle with notify initially, but later realized notify is turned on by default. Main reason why im checking if it's on/off is to make sure I don't break things I dont know about (like fuchsia). Im referring to open method in file.rs where if I dont do the check will give unexpected results in fuchsia.

Another question here is whether or not 'tail' implies that a file is read-only. Realistically, I think it does; we can't reload changes if we've changed the file ourselves, so I don't think it makes sense for us to allow editing in this case.

Valid point. Im on Linux using xi-gtk so behavior I see might be different from xi on mac. When tailing a file if I make changes....xi stops tailing! It not just stops tailing it wont allow me to save the file either. This behavior is similar to vscode and atom. They do allow editing a file being tailed just like xi does. So I guess what you are saying here is the current behavior is fine and we dont have to worry about read-only for this PR?

The main intention of this PR was to improve performance of reload. This PR, instead of loading entire file on every change, will only load the delta.

@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 26, 2018

Ohh and I just remembered, I already had something similar to debug option you are talking about. It's in struct TailDetails called is_tail_mode_on .

@cmyr
Copy link
Member

cmyr commented Nov 26, 2018

Ah okay I didn't realize you were on xi-gtk. You're correct that if you edit the file tail will get 'cancel'ed (it will still try to reload but will abort when it sees there are unsaved changes) so maybe this is reasonable behaviour.

When I talk about a 'debug' setting I mean an RPC, prefixed with 'debug', that we can test the feature with. This is an internal convention for things that are still in flux. On xi-mac, we have a 'debug' menu where we cram a bunch of stuff:

debug menu

And if you were working on this feature on xi-mac I'd suggest you add a 'tail' command there.

In any case, it would be nice to have a command that is exposed from a menu that lets you toggle this feature; we'll need that eventually and it makes testing much easier.

@sjoshid
Copy link
Contributor Author

sjoshid commented Nov 26, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants