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

Merge view - jumping onChange #681

Open
natocTo opened this issue Aug 30, 2024 · 15 comments
Open

Merge view - jumping onChange #681

natocTo opened this issue Aug 30, 2024 · 15 comments

Comments

@natocTo
Copy link

natocTo commented Aug 30, 2024

Hello,

we are trying to implement merge view into our app and we came across this behaviour.

There is a simple sandbox: https://codesandbox.io/p/sandbox/competent-tom-8zpsy7

Please try to enter severals new lines (for example 50) into second editor (I just keep presed enter for while). When it hits certain point the view "jump" up over active cursor. And anytime you enter another char there (on some latest lines usually), it behave same (jump a little bit up). So it is really hard to write something there.

Did I miss some setting I have to use? Thank you very much for any help.

jaywcjlove added a commit that referenced this issue Sep 5, 2024
@jaywcjlove
Copy link
Member

@natocTo upgrade v4.23.1

import React, { useState } from 'react';
import CodeMirrorMerge from 'react-codemirror-merge';

const Original = CodeMirrorMerge.Original;
const Modified = CodeMirrorMerge.Modified;
let doc = `one
two
three
four
five`;

export default function App() {
  const [value, setValue] = useState(doc);
  const [valueModified, setValueModified] = useState(doc);
  return (
    <div>
      <CodeMirrorMerge destroyRerender={false}>
        <Original
          onChange={(val) => {
            console.log('~~:1', val);
            setValue(val);
          }}
          value={value}
        />
        <Modified
          onChange={(val) => {
            console.log('~~:2', val);
            setValueModified(val);
          }}
          value={valueModified}
        />
      </CodeMirrorMerge>
      <div style={{ display: 'flex', marginTop: 10 }}>
        <pre style={{ flex: 1 }}>{value} </pre>
        <pre style={{ backgroundColor: '#fff', flex: 1 }}>{valueModified} </pre>
      </div>
    </div>
  );
}

github-actions bot pushed a commit that referenced this issue Sep 5, 2024
@cjayyy
Copy link

cjayyy commented Sep 10, 2024

@jaywcjlove Thank you for the update!
The destroyRerender={false} seems to solve the glitch for us. But with that the custom theme and extensions no longer work. Do you know why by any chance?

jaywcjlove added a commit that referenced this issue Sep 10, 2024
github-actions bot pushed a commit that referenced this issue Sep 10, 2024
@jaywcjlove
Copy link
Member

@cjayyy After the MergeView is initialized, it doesn't update the extensions. I guess it might be that your extensions are loaded asynchronously.

useEffect(() => {
if (!view.current && editor.current && originalExtension && modifiedExtension) {
view.current = new MergeView({
a: {
...original,
extensions: [
...(originalExtension?.extension || []),
...getDefaultExtensions({ ...originalExtension?.option, theme }),
originalUpdateListener,
],
},
b: {
...modified,
extensions: [
...(modifiedExtension?.extension || []),
...getDefaultExtensions({ ...modifiedExtension?.option, theme }),
modifiedUpdateListener,
],
},
parent: editor.current,
...opts,
});
}
}, [view, editor, originalExtension, modifiedExtension]);

If your extensions are asynchronous, add them to the editor.

const handle1 = () => {
$ref.current?.view?.a.dispatch({
effects: StateEffect.appendConfig.of([log2UpdatePlugin]),
});
};

Implementing extensions updates inside the editor causes errors, and I haven't found a solution for this. It may require support from the CodeMirror team, and you can seek answers from their community.

// effects: StateEffect.reconfigure.of([
// ...(originalExtension?.extension || []),
// ...getDefaultExtensions({ ...originalExtension?.option, theme }),
// ])

@cjayyy
Copy link

cjayyy commented Sep 10, 2024

Not using any async extensions. Even if I use only minimal setup like below, destroyRerender={false} completely disables the functionality of those two extensions (both editors are editable). When setting destroyRerender={true}, extensions work.

<CodeMirrorMerge destroyRerender={false}>
   <CodeMirrorMerge.Original
      value="first"
      extensions={[EditorView.editable.of(false), EditorState.readOnly.of(true)]}
   />
   <CodeMirrorMerge.Modified value="second" />
</CodeMirrorMerge>

@jaywcjlove
Copy link
Member

@cjayyy My example test didn't have any issues -> https://uiwjs.github.io/react-codemirror/#/examples/681

<CodeMirrorMerge ref={$ref} destroyRerender={false}>
<Original
value={value}
extensions={[log1UpdatePlugin]}
onChange={(val) => {
setValue(val);
}}
/>

@cjayyy
Copy link

cjayyy commented Sep 10, 2024

Oh I overlooked you published version 4.23.2 - with that it works! (4.23.1. didn't work for me).
Thank you very much. Will test it more thoroughly tomorrow. 🤞

@cjayyy
Copy link

cjayyy commented Sep 11, 2024

Thank you. It looks like the original issue with lines jumping is fixed now.

We are still struggling with another (probably) unrelated issue though;
When changing the doc value fast enough the change handlers (our outside onChange handler` and your internal change dispatching) get mixed up and cycled in and infinite loop causing last change undoing and redoing forever leaving the editor stuck unusable.
My guesses:

Could please point me into the right direction? 🙏

@natocTo
Copy link
Author

natocTo commented Oct 31, 2024

@jaywcjlove it looks like after update to v4.23.1 (and newer) the onChange is actually not called at all.

Check this sandbox - https://codesandbox.io/p/sandbox/gracious-chandrasekhar-zqmsy9?file=%2Fsrc%2FApp.js it should show alert when updating a value. But it is not called.

@jaywcjlove
Copy link
Member

@natocTo I suggest using the approach in the example below to handle the onChange event.

<Modified
  onChange={(val) => {
    console.log('~~:2', val);
    setValueModified(val);
  }}
  value={valueModified}
/>

@natocTo
Copy link
Author

natocTo commented Nov 1, 2024

Hi @jaywcjlove I did not realize the sandbox may change after I send a link, I used onChange originally. Please check sandbox again. There is onChange but it is never called.

https://codesandbox.io/p/sandbox/gracious-chandrasekhar-zqmsy9?file=%2Fsrc%2FApp.js%3A1%2C1-39%2C1

@jaywcjlove
Copy link
Member

jaywcjlove commented Nov 1, 2024

@natocTo https://codesandbox.io/p/sandbox/nostalgic-leakey-3frkm2?workspaceId=dca383b2-9fa2-4898-b548-3527e348e0e1

2024-11-01.15.33.03.2024-11-01.15.33.19.mp4
import React, { useState } from "react";

import CodeMirrorMerge from "react-codemirror-merge";
import { EditorView } from "codemirror";
import { EditorState } from "@codemirror/state";

const Original = CodeMirrorMerge.Original;
const Modified = CodeMirrorMerge.Modified;
let doc = `one
two
three
four
five`;

export default function App() {
  const [value, setValue] = useState(doc);
  const [valueModified, setValueModified] = useState(doc);
  return (
    <div>
      <CodeMirrorMerge destroyRerender={false}>
        <Original
          extensions={[
            EditorView.editable.of(false),
            EditorState.readOnly.of(true),
          ]}
          onChange={(val) => {
            console.log("~~:1", val);
            setValue(val);
          }}
          value={value}
        />
        <Modified
          extensions={[
            EditorView.editable.of(true),
            EditorState.readOnly.of(false),
          ]}
          onChange={(val) => {
            console.log("~~:2", val);
            setValueModified(val);
          }}
          value={valueModified}
        />
      </CodeMirrorMerge>
      <div style={{ display: "flex", marginTop: 10 }}>
        <pre style={{ flex: 1 }}>{value} </pre>
        <pre style={{ backgroundColor: "#fff", flex: 1 }}>{valueModified} </pre>
      </div>
    </div>
  );
}

@natocTo
Copy link
Author

natocTo commented Nov 1, 2024

@jaywcjlove thank you! Can you elaborate more why it is working for you? It is because of destroyRerender?

It looks like it is false by default https://github.com/uiwjs/react-codemirror/blob/v4.23.6/merge/src/index.tsx#L12

@jaywcjlove
Copy link
Member

if (destroyRerender && view.current) {
const originalFrom = view.current.a.state.selection.ranges[0].from;
const modifiedFrom = view.current.b.state.selection.ranges[0].from;
view.current.destroy();
view.current = new MergeView({
a: {
...original,
extensions: [
...(originalExtension?.extension || []),
...getDefaultExtensions({ ...originalExtension?.option, theme }),
],
},
b: {
...modified,
extensions: [
...(modifiedExtension?.extension || []),
...getDefaultExtensions({ ...modifiedExtension?.option, theme }),
],
},
parent: editor.current!,
...opts,
});
if (originalFrom) {
view.current.a.focus();
view.current.a.dispatch({
selection: {
anchor: originalFrom,
head: originalFrom,
},
});
}
if (modifiedFrom) {
view.current.b.focus();
view.current.b.dispatch({
selection: {
anchor: modifiedFrom,
head: modifiedFrom,
},
});
}
}
}, [view, theme, editor.current, original, modified, originalExtension, modifiedExtension, destroyRerender]);

@natocTo destroyRerender is designed to address the issue of extensions failing to refresh updates.

@kyleqian
Copy link

kyleqian commented Nov 9, 2024

@jaywcjlove I'm having the same issue as @natocTo on 4.23.6. onChange is never called unless destroyRerender is false. This seems to be a bug, no?

@jaywcjlove
Copy link
Member

// effects: StateEffect.reconfigure.of([
// ...(originalExtension?.extension || []),
// ...getDefaultExtensions({ ...originalExtension?.option, theme }),
// ])

@kyleqian This is a temporary workaround for when the extension update fails.

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

No branches or pull requests

4 participants