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

Reading ySyncFacet dynamically. #19

Open
a3626a opened this issue Sep 16, 2022 · 2 comments
Open

Reading ySyncFacet dynamically. #19

a3626a opened this issue Sep 16, 2022 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@a3626a
Copy link

a3626a commented Sep 16, 2022

Is your feature request related to a problem? Please describe.

Hi, I am working on this PR jupyterlab/jupyterlab#13093 . There is a need to dynamically update YSyncConfig after the initialization of EditorView. I think ySyncFacet is a nice place to do this. However YSyncPluginValue reads this facet during construction, and caches its value. So changing ySyncFacet after the initialization of EditorView has no effect on YSyncPluginValue.

I am changing ySyncFacet by using CodeMirror compartment.

packages/codemirror/src/editor.ts

...
    const ySyncFacetCompartment = new Compartment();
...
    this._editor = Private.createEditor(
      host,
      fullConfig,
      this._yeditorBinding,
      this._editorConfig,
      [
        this._markField,
        Prec.high(domEventHandlers),
        updateListener,
        translation,
        // might need some precedence setup here.
        ySyncFacetCompartment.of(
          ySyncFacet.of(
            new YSyncConfig(
              this._yeditorBinding?.text,
              this._yeditorBinding?.awareness
            )
          )
        ),
      ]
    );
...
    // every time the model is switched, we need to re-initialize the editor binding
    this.model.sharedModelSwitched.connect(() => {
      this._initializeEditorBinding();

      // it does change ySyncFacet. However YSyncPluginValue already cached old value.
      this._editor.dispatch({
        effects: ySyncFacetCompartment.reconfigure(
          ySyncFacet.of(
            new YSyncConfig(
              this._yeditorBinding?.text,
              this._yeditorBinding?.awareness
            )
          )
        )
      });
    }, this);
...

Describe the solution you'd like

I think just not caching the facet value can solve the problem.

Describe alternatives you've considered

No

Additional context

jupyterlab/jupyterlab#13093

@a3626a a3626a added the enhancement New feature or request label Sep 16, 2022
@a3626a a3626a changed the title Reading ySyncFacet more dynamic. Reading ySyncFacet dynamically. Sep 16, 2022
@dmonad
Copy link
Member

dmonad commented Sep 16, 2022

This should work when you create a compartment of all the y-codemirror plugins, right? Just a quick fix.

Ideally, it should be possible to create a compartment of only the facet. I'll investigate how much effort this would take.

@a3626a
Copy link
Author

a3626a commented Sep 19, 2022

This should work when you create a compartment of all the y-codemirror plugins, right? Just a quick fix.

I think so. Jupyterlab uses compartment to dynamically change codemirror plugins, including configurations like language, them, tab size and so on.

Additionally, in the documentaion:

Facet values are only recomputed when necessary, so you can use an object or array identity test to cheaply check whether a facet changed.

So it is safe (no performance degradation) not to cache the facet output value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants