yjs / y-quill

Quill Editor binding for Yjs
https://demos.yjs.dev/quill/quill.html
MIT License
80 stars 18 forks source link

Add option to enable/disable normailze quill delta #2

Open Kisama opened 4 years ago

Kisama commented 4 years ago

Checklist

[ ] Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/ [ ] Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

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

Describe the solution you'd like

dmonad commented 4 years ago

What is the use-case for this? Or do you encounter any problems because the newline is removed?

If you disable this behavior, then you will encounter weird behavior, since every clients start with a document that has a newline. Assume you have two clients starting with "\n", then the merged document will be two lines long. So every new client will add a newline item above or below the document.

Another unfortunate behavior is that users often add more newlines to have more space where they can add content. When syncing offline changes, these added newlines will add up to a huge document.

Maybe this behavior can be optimized by only removing the initial newline. I remember that this was my initial plan. I gave up eventually because of some problem. You are welcome to create a PR to yjs/yjs to fix this behavior in Y.Text. Although I would prefer a generic solution.

Kisama commented 4 years ago

In my case,

  1. 2 client(user A, userB) start with some text(ie : text...)
  2. user A add new line with enter at the end of content
  3. user A's editor has 2 lines but user B'editor just one line
  4. user A add new line with enter at the end of content again
  5. user A's editor has 3 lines but user B'editor just 2 lines

and I found in YText's applyDelta function remove last new line cause of quill In my opinion, before call applyDelta in _quillObserver function, normQuillDelta function applied and YText's quill related new line delete logic is removed which is exist only for quill.

I don't konw It can be better solution

applyDelta (delta) {
    if (this.doc !== null) {
      transact(this.doc, transaction => {
        /**
         * @type {ItemListPosition}
         */
        let pos = new ItemListPosition(null, this._start)
        const currentAttributes = new Map()
        for (let i = 0; i < delta.length; i++) {
          const op = delta[i]
          if (op.insert !== undefined) {
            // Quill assumes that the content starts with an empty paragraph.
            // Yjs/Y.Text assumes that it starts empty. We always hide that
            // there is a newline at the end of the content.
            // If we omit this step, clients will see a different number of
            // paragraphs, but nothing bad will happen.
            const ins = (typeof op.insert === 'string' && i === delta.length - 1 && pos.right === null && op.insert.slice(-1) === '\n') ? op.insert.slice(0, -1) : op.insert
            if (typeof ins !== 'string' || ins.length > 0) {
              pos = insertText(transaction, this, pos.left, pos.right, currentAttributes, ins, op.attributes || {})
            }
          } else if (op.retain !== undefined) {
            pos = formatText(transaction, this, pos.left, pos.right, currentAttributes, op.retain, op.attributes || {})
          } else if (op.delete !== undefined) {
            pos = deleteText(transaction, pos.left, pos.right, currentAttributes, op.delete)
          }
        }
      })
    } else {
      /** @type {Array<function>} */ (this._pending).push(() => this.applyDelta(delta))
    }
  }
dmonad commented 4 years ago

Hey @Kisama , I gave a pretty lengthy explanation of the reason why the newline is ignored. You just described again that the quill documents have different lengths. This is the expected behavior.

Kisama commented 4 years ago

@dmonad I'm sorry if you feel bad. I just want to know if i can try to move YText quill related logic to y-quill.

dmonad commented 4 years ago

I think I misunderstood you. So you are trying to use YText without y-quill. That makes sense. Sure, you can create a PR that implements an option to disable that behavior in YText.

Kisama commented 4 years ago

@dmonad Thank you so much. The Idea is quite similar but different usecase , I use yjs and y-quill at once and i need to sync new lines even though it’s duplicated. I’ll try to make 2 PRs first is add option quillbinding to enable/disable remove mewline (default is true) and use option before applyDelta, second remove quill related logic in YText’s applyDelta So I think it works same as now but who can want to do not remove new lone automatically can disable using quillbinding.

I think the result is same as your idea. If you have any concern please tell me, I try to find another solution. If not , I’ll request PR tomorrow

dmonad commented 4 years ago

Unfortunately we can't remove this behavior from YText because other people might rely on it. No breaking changes. We can make a note to remove this behavior in the next major release though.

Kisama commented 4 years ago

Ok, I got it. In short, remove behavior on YText is impossible but add option to disable behavior is possible.