Show changes relative to VCS in diagnostics

[luctius]

Because I'm lazy it is for git only for now, and it is only updated when the file is saved.

WIP because my children wake too early and I'm tired.

[kirawi]

It would be better to have it as a plugin: #227
Maybe you could look at #455 to help figure out the API needed for this kind of thing.

[pickfire]

I think we should just use the ▎ for add and modify and ▁ for blocks removed. UTF-8 characters will look better. And we can make use of colors, like green for added, yellow/orange for modified, red for removed.

[archseer]

I personally prefer:

let g:gitgutter_sign_added = '▌'
let g:gitgutter_sign_removed = '▖'
let g:gitgutter_sign_removed_first_line = '▘'
let g:gitgutter_sign_modified = '▐'
let g:gitgutter_sign_modified_removed = '▞'

with
diff.plus #35BF86
diff.minus #F22C86
diff.delta #6F44F0

[luctius]

While I like those glyphs, I'm not sure if they are very discoverable to unfamiliar users. That said, I am planning to make them them theme configurable in a separate PR.

[pickfire]

▞ is confusing, I prefer - or line like for removed, at least it's clearer.

[archseer]

You need to see how it looks visually. ▞ is specifically when an area is both deleted and then modified.

[archseer]

In the first purple area, one line was deleted and replaced with a single line. In the second area, multiple lines were deleted and replaced with a single line.

[pickfire]

Without you explaning, I won't understand what that means.

[pickfire]

It would be good as a plugin but I think this plugin should be bundled by default, since git is very widely used.

[archseer]

I wonder if https://github.com/Byron/gitoxide might be a better fit to avoid needing to link against openssl/libssh/libz.

[pickfire]

Can gitoxide do a line diff for file yet? Oh, maybe it can. https://docs.rs/git-diff/0.3.0/git_diff/tree/index.html

[luctius]

I will look at gitoxide later; I hadn't realized it provided a goo diff alternative yet. That said; we probably won't need openssh since we don't need to make connections. (I did however forget to disable it... ;-))

[luctius]

I took a look at gitoxide, but that is still heavily in development; case in point, if I wanted to use git-repository, I had to use the git repo, and I had a conflict with libc when using git-repository, git-diff and chrono.

My suggestion is to take another look after a couple of months and see if things are more stable.

[pickfire]

Why are all of them blue by default?

[pickfire]

Tested, seemed to show something but default colors seemed to be off. All of them is blue.

The diff also showed out too late, it will only show the current diff after two saves rather than instant after save.

[pickfire]

I think the bug that the diff showed out too late is probably because save_impl is asynchronous and this does not triggers render after it is finished.

[luctius]

Any suggestions on how to fix this?

[luctius]

Ok, I see now that the diff occurs before the async save call, which means it occurs before the file is actually written.
I'm not sure how to fix it in an elegant way yet, but there is progress.

[archseer]

Sorry, forgot to comment here, I think the way to go would be to .and_then the save future, in that closure schedule another callback (using something similar as cx.callback) and modify the document in there.

I'm planning on making this a bit easier to do by introducing hooks, so saving would trigger a hook::DOCUMENT_SAVED and schedule jobs like these.

[luctius]

I had some time to look at it again.

From what I can see, we need some way to have a job queue with access to the document and which the document can access.

I'm thinking of giving document a jobs queue. This would be similar to how Compositor polls it's layers, now Editor will poll the documents for any jobs.

This would allow us to move several other functions to async, like possibly open and format.

[archseer]

I think we could maybe make the existing jobs queue accessible globally, and in the callback you can simply use a passed in id to find the document (editor.documents[id])

[archseer]

Format already uses the callback system after some of the recent PRs, it receives the changes in a job then applies them to the doc: #285

[archseer]

The blue is because the scope names don't match anymore I guess. The code will use #0000FF if a scope is missing so you can see something is wrong:

helix/helix-view/src/theme.rs

Line 223 in d754c72

.unwrap_or_else(|| Style::default().fg(Color::Rgb(0, 0, 255)))

(Maybe we should make that bright yellow/red/pink to make it more obvious)

[gbaranski]

wouldn't it be better to make another column for displaying git changes? diagnostics column is very wide. here's how it could look like:

source: https://github.com/lewis6991/gitsigns.nvim

[pickfire]

There are color now but the issue that the displayed vcs is still one save old is still there.

[kirawi]

To address #405, maybe here we could abstract it to simply be doc.line_changes() . It doesn't need to be done in this PR however.

[luctius]

Good point, though I agree that it is probably outside of the scope of this PR.

[luctius]

There are color now but the issue that the displayed vcs is still one save old is still there.

As I mentioned above, I am still working this out.

I had some time to look at it again.

From what I can see, we need some way to have a job queue with access to the document and which the document can access.

I'm thinking of giving document a jobs queue. This would be similar to how Compositor polls it's layers, now Editor will poll the documents for any jobs.

This would allow us to move several other functions to async, like possibly open and format.

[zetashift]

I also find the column a bit too wide for my taste, however I'm glad the functionality is being worked on!

[kirawi]

There are color now but the issue that the displayed vcs is still one save old is still there.

As I mentioned above, I am still working this out.

I had some time to look at it again.
From what I can see, we need some way to have a job queue with access to the document and which the document can access.
I'm thinking of giving document a jobs queue. This would be similar to how Compositor polls it's layers, now Editor will poll the documents for any jobs.
This would allow us to move several other functions to async, like possibly open and format.

I don't know if you found out or not, but we indeed already have this functionality:

helix/helix-term/src/application.rs

Lines 200 to 212 in 2f8ad7f

	Some(callback) = self.jobs.futures.next() => {
	self.jobs.handle_callback(&mut self.editor, &mut self.compositor, callback);
	self.render();
	}
	Some(callback) = self.jobs.wait_futures.next() => {
	self.jobs.handle_callback(&mut self.editor, &mut self.compositor, callback);
	self.render();
	}
	_ = &mut self.editor.idle_timer => {
	// idle timeout
	self.editor.clear_idle_timer();
	self.handle_idle_timeout();
	}

We already use it for opening and maybe formatting, e.g.

helix/helix-term/src/commands.rs

Lines 1563 to 1591 in 2f8ad7f

	fn write_impl<P: AsRef<Path>>(
	cx: &mut compositor::Context,
	path: Option<P>,
	) -> anyhow::Result<()> {
	let jobs = &mut cx.jobs;
	let (_, doc) = current!(cx.editor);
	if let Some(path) = path {
	doc.set_path(Some(path.as_ref()))
	.context("invalid filepath")?;
	}
	if doc.path().is_none() {
	bail!("cannot write a buffer without a filename");
	}
	let fmt = doc.auto_format().map(|fmt| {
	let shared = fmt.shared();
	let callback = make_format_callback(
	doc.id(),
	doc.version(),
	Modified::SetUnmodified,
	shared.clone(),
	);
	jobs.callback(callback);
	shared
	});
	let future = doc.format_and_save(fmt);
	cx.jobs.add(Job::new(future).wait_before_exiting());
	Ok(())
	}

[kirawi]

It also seems like gitoxide has become a lot more featureful, so we may be able to switch to it in the relatively soon future.

[Nehliin]

Anyone working on this? I'd really like this functionality pre plugins since the plugin system seems a bit far off. I'd be happy to help on this.

[pickfire]

@Nehliin Seemed like @luctius is not so active lately. Maybe you can take his current patch and rebase it to master and fix the bugs that mentioned here, that can help on this patch.

[Nehliin]

I'll take a look then 👍

[the-mikedavis]

Should we close in favor of #1623?