yjs/y-quill

Add option to enable/disable normailze quill delta

Opened this issue · 8 comments

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.

  • y-quill alway remove last new line

Describe the solution you'd like

  • add option to enable/disable normalize quill data in normQuillDelta function

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.

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))
    }
  }
    

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.

@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.

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.

@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

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.

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