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

Add option to enable/disable normailze quill delta #2

Open
Kisama opened this issue May 30, 2020 · 8 comments
Open

Add option to enable/disable normailze quill delta #2

Kisama opened this issue May 30, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@Kisama
Copy link

Kisama commented May 30, 2020

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
@Kisama Kisama added the enhancement New feature or request label May 30, 2020
@dmonad
Copy link
Member

dmonad commented May 30, 2020

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
Copy link
Author

Kisama commented Jun 1, 2020

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
Copy link
Member

dmonad commented Jun 1, 2020

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 dmonad closed this as completed Jun 1, 2020
@Kisama
Copy link
Author

Kisama commented Jun 1, 2020

@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
Copy link
Member

dmonad commented Jun 1, 2020

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 dmonad reopened this Jun 1, 2020
@Kisama
Copy link
Author

Kisama commented Jun 1, 2020

@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
Copy link
Member

dmonad commented Jun 1, 2020

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
Copy link
Author

Kisama commented Jun 1, 2020

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

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