codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.75k stars 366 forks source link

[autcomplete] need more information about changes to the document #1347

Closed bradymadden97 closed 6 months ago

bradymadden97 commented 7 months ago

Describe the issue

When trying to implement LSP-backed completion sources, we run into some difficulties with the current @codemirror/autocomplete API.

I think, at a minimum,

  1. Having access to the view inside CompletionSource callback, or having some way of knowing when a completion has started (startCompletionEffect is not currently exported from the package)
  2. Having access to the transaction inside the CompletionResult update callback would allow us to continue what we've currently set up, in a less hacky way

But I added the extra context in case you had ideas for a more holistic approach to some of these LSP issues, given the support you just added for commitCharacters.

Thanks so much, as always!

Browser and platform

No response

Reproduction link

No response

marijnh commented 7 months ago

Note that the update method is only called for typing or backspacing anyway, and it's not something that will give you all editor updates.

Would adding support for a map method ((CompletionResult, Transaction) -> CompletionResult) that is called for every transaction help you here? Completion result maintenance happens on the state level, so you still wouldn't get access to a view. But that doesn't seem like it should be necessary to begin with.

bradymadden97 commented 7 months ago

I think a map method would solve problem (2) in a good way, yes!

As for problem (1), I don't think it would completely solve it, as we also need to know about any changes that happened during transactions while the async completion source is inflight (before it's responded with its completions).

A concrete example of this would be a multiplayer scenario, where:

This would be prior to changes that happen while the completions are available, which I think would be solved by your solution above.

Let me know if you have any questions about this.

marijnh commented 7 months ago

The autocompletion plugin is already tracking transactions that happen while the completion is being fetched, it would call map for those as well.

bradymadden97 commented 7 months ago

if that's the case, I think a map method sounds like it could work. for a given completion source, provided via override, we could track any changes applied during the pending state via the map method, then apply them to the results once the source returns, and then continue to apply changes from the map method while the state is active.

marijnh commented 6 months ago

Does attached patch look like it would work for you? I had to use ChangeDesc, rather than the full transaction, to also make this work correctly for mapped setActiveEffect effects.

bradymadden97 commented 6 months ago

Yeah I just tried this out and I think it should work for what I need. I noticed in a situation where a user is typing: "foo" and a slow completion source is triggered after "f", and again after "oo", when the results for the "f" request are returned, map is immediately called with the change that includes "oo", which seems like it will work perfectly.

If you're able to tag a release for this that would be great!

marijnh commented 6 months ago

I've tagged 6.14.0

bradymadden97 commented 6 months ago

Hm I just realized I might be more stumped by the ChangeDesc than I originally thought. I can compose the ChangeDescs togther to keep a "missed changes" desc, but I realized I need to do the following steps at the point where I want to apply one of the completion items:

  1. Invert the ChangeDesc
  2. Use the inverted desc to get the original document state, at the starting point in time that the completion request was initiated
  3. Convert the LSP position to the document offset (LSP position is encoded as {line: number, column: number})
  4. Map that offset over the original ChangeDesc

I realized that step 2 might not be possible with a ChangeDesc and that I may need a ChangeSet after all. Unless I'm just missing a series of manipulations that could let me get to the same end state...

bradymadden97 commented 6 months ago

Ok i may have found a workaround by storing a copy of context.state.doc before the request was initiated. That allows me to get past step #2.

bradymadden97 commented 5 months ago

@marijnh would it be possible to include EditorState or CompletionContext as a third parameter in map, similar to update? I think that would help me solve my mapping problem (as I need the document to translate from LSP positions which are line/col and have no concept of absolute offset, to CM positions which are absolute and have no concept of line/col in change descs without the document itself)

I would try my hand at a PR but figured I'd ask if you think it's possible first - happy to try a PR if so.

marijnh commented 5 months ago

These are also called from a state effect map function, which itself has no effect to any editor state, so this would be difficult to do. Would it be possible to work in flat positions at this level by doing the conversion from line/col positions to flat positions at at an earlier point?

bradymadden97 commented 5 months ago

Oh. Wait yeah I have access to the state at request time via the AutoCompletion Context, which means I have the starting doc, so I can transform the LSP response to the absolute offset in the starting doc, then map it over the change descs. Sorry I missed that idea yesterday. That should work. Will update if it doesn't. Thanks!

bradymadden97 commented 5 months ago

Ok one more request - it would be nice if CompletionResult was generic on Completion. I've got some extra properties hanging off my Completion objects:

class MyCompletionWithExtraProperties implements Completion {....}

It would be awesome if I could have those types in update and map methods for a completion source:

type CompletionSource = (context: CompletionContext) => CompletionResult | null | Promise<CompletionResult | null>;

something like

type CompletionSource = <C extends Completion = Completion>(context: CompletionContext) => CompletionResult<C> | null | Promise<CompletionResult<C> | null>;
async function completionSource(context) {
  const options = [new MyCompletionWithExtraProperties(...)]

  return {
    from: context.pos,
    options,
    update: (current: CompletionResult, ...) => {
      current.options; /* Completion[] */
      // would be nice if `current.options` was typed as MyCompletionWithExtraProperties[]
      ...
    },
    map: (current: CompletionResult, ...) => {
      // same...
    },
  }
}
marijnh commented 5 months ago

I see what you mean but I'm not a fan of how complex this makes the already rather messy types, for a rather obscure use case. I think you'll just have to use casts there for the time being.

bradymadden97 commented 5 months ago

No worries thanks for working with me on some of these :)