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

First attempt at sync formatting. #285

Merged
merged 5 commits into from
Jun 30, 2021
Merged

Conversation

jneem
Copy link
Contributor

@jneem jneem commented Jun 17, 2021

This is a first stab at #53.

There are definitely some rough edges still:

  • I wanted commands to be able to provide LspCallbacks, so I made them take a compositor::Context instead of an Editor. I don't know whether that's reasonable.
  • I'm not sure if I'm keeping the language-server's changes in sync correctly
  • Transaction isn't Sync, so lots of Arc<Mutex<>>...

@jneem jneem marked this pull request as draft June 17, 2021 15:13
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the initiative on this!

I'm assuming the issue with Transaction is that it carries a Tendril? I've been pondering replacing it with something simpler like smol_str, but I like some of the properties (being able to mutate it, reusing the underlying buffer on clone)

helix-view/Cargo.toml Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
Box::new(move |editor: &mut Editor, compositor: &mut Compositor| {
let view_id = editor.view().id;
if let Some(doc) = editor.document_mut(doc_id) {
if doc.version() == doc_version {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the version changed, we could later on use Transaction::map. It's currently unimplemented because I didn't need it yet:

/// Given another change set starting in the same document, maps this
/// change set over the other, producing a new change set that can be
/// applied to the document produced by applying `other`. When
/// `before` is `true`, order changes as if `this` comes before
/// `other`, otherwise (the default) treat `other` as coming first.
///
/// Given two changes `A` and `B`, `A.compose(B.map(A))` and
/// `B.compose(A.map(B, true))` will produce the same document. This
/// provides a basic form of [operational
/// transformation](https://en.wikipedia.org/wiki/Operational_transformation),
/// and can be used for collaborative editing.
pub fn map(self, _other: Self) -> Self {
unimplemented!()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds useful, yes! Will there be a way to check if the two transactions "interact/conflict"? I worry a little about messing up the user's edits, and given that we'll reformat anyway at the next save I think it's usually fine to just throw away the reformatting.

helix-view/src/document.rs Outdated Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
@jneem
Copy link
Contributor Author

jneem commented Jun 23, 2021

Thanks for the feedback on the draft! I think this one's in better shape now.

@jneem jneem marked this pull request as ready for review June 23, 2021 18:42
}
helix_lsp::block_on(tokio::spawn(doc.save()));
let fmt = if autofmt {
doc.format().map(|fmt| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get format() to return Future without Option since that only checks for language server.

Maybe autofmt should take it into account whether language server is available so we don't have to do the first branch or can just unwrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the return type of format(), I can see these options:

  1. Option<impl Future>
  2. impl Future, and panic if the language server isn't there
  3. impl Future, and return a boxed future with no changes if the language server isn't there.

My preference is for (1).

One alternative that would make this usage nicer would be to add a Document::auto_format method that's the same as Document::format but returns None if autoformatting is off.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning Option is fine, it's what I've done elsewhere. Returning a future means we need to box values, it's an unnecessary cost if we're just trying to signal no formatting can be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding a Document::auto_format method. See what you think...

} else {
None
};
tokio::spawn(doc.format_and_save(fmt));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block_on was necessary here. It's a bit of a hack but without it the editor could quit before a file save completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a problem. Because if we're blocking until the save is done, then we're also blocking until the formatting is done and so this PR won't achieve anything...

What if we were to store the JoinHandles from spawning the save job, and make sure to block on them before exit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that a more general version of the LspCallbacks would be useful (not limited to LSP). We could spawn the futures there, then receive the values. If we get a Error return we can also log it to the Editor via set_error. This would solve the problem of not being able to report errors from these utility tasks.

The shutdown sequence would then fully drain this queue (or time out) before dropping the runtime.

Do you want to make the change in this PR? I can probably look into it otherwise.

(By the way, feel free to join us on Matrix! we were just discussing your teddy library earlier)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can take a stab at this. I'm not 100% sure what you mean by "receive the values", because I would expect that different jobs would want to return different things, and then how would the editor know how to handle them? I guess what I'm trying to say is that I don't see a way to completely avoid the callback...

I was imagining a AsyncJob struct (or maybe trait?) that contains

  • a Future returning an anyhow::Result<Callback>
  • a JoinHandle for cancelling that future
  • some additional metadata, like timeouts, and whether we need to wait on the future before exiting

Does that sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was imagining something similar, but simpler:

lsp_callbacks is currently defined as FuturesUnordered<LspCallback>. I'd turn that into a callbacks: FuturesUnordered<Result<(), anyhow::Error>>. The lsp futures will then need to be wrapped in another future to work as before.

cx.spawn(doc.save) would push the future onto the callbacks. Since the callback results are received on in the main event loop here:

Some(callback) = &mut self.callbacks.next() => {
self.handle_language_server_callback(callback)
}

We could catch any Errors here and use editor.set_error to show them ("save failed" etc).

When quitting we just need to drain the callbacks stream until empty or a timeout is reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(By the way, feel free to join us on Matrix! we were just discussing your teddy library earlier)

Sure, I'll join! It's really BurntSushi's library, though. I just extracted it into a crate so I could play with it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd turn that into a callbacks: FuturesUnordered<Result<(), anyhow::Error>>. The lsp futures will then need to be wrapped in another future to work as before.

How would you get access to the &mut Editor in the wrapper callback, though? I thought that borrowing issue was the reason for the whole callback pattern.

@jneem
Copy link
Contributor Author

jneem commented Jun 28, 2021

This adds some very basic infrastructure for async jobs. The API feels a little over-engineered right now, but I wrote it with the idea of eventually having things like:

jobs.spawn(
    Job::new(fut)
        .timeout(Duration::from_secs(1))
        .cancel_on_edit()
        .show_some_specific_kind_of_ui_feedback()
);

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I finally had some time to test this out. LGTM! 👍🏻

@archseer archseer merged commit b39e452 into helix-editor:master Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants