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

Wrong deltas? #98

Closed
benbro opened this issue May 13, 2014 · 7 comments
Closed

Wrong deltas? #98

benbro opened this issue May 13, 2014 · 7 comments

Comments

@benbro
Copy link
Contributor

benbro commented May 13, 2014

I'm testing demo/index.html with FF 29 and IE11.

When writing "aaa" the deltas I'm getting are:

[ InsertOp { value="a" }, RetainOp { start=0, end=1 } ]  // first "a"
[ InsertOp { value="a" }, RetainOp { start=0, end=2 } ] // second "a"
[ InsertOp { value="aaa" }, RetainOp { start=2, end=3 } ] // "third "a"

Why two ops are produced for the first "a"?
Doesn't InsertOp {value="a"} already have all the info we need?
Shouldn't we get RetainOp only when skipping some text?

Why the last step produces an InsertOp {value="aaa"} although I only added a single "a" after the previous op?
In the last RetainOp start=2 and end=3 which is not consistent with previous ops.

When I enter "ab" I'm getting the following deltas:

[ InsertOp { value="a" }, RetainOp { start=0, end=1 } ] // first "a"
[ InsertOp { value="ab" }, RetainOp { start=1, end=2 } ] // second "b"

When entering "ab" I expected to get the same ops like when entering "aa" but for some reason the second ops is different.

Will you consider adding a DeleteOp?
It will make operations much easier to work with.
Did you remove it to save bandwidth when synching?

Thanks

@jhchen
Copy link
Member

jhchen commented May 14, 2014

Deltas in its current form was designed to reduce the number of operations, which right now is just two: insert and retain. It always describes the whole document so it's not the most compact format (though usually its just a two extra retain operations).

The third instruction you're seeing is saying delete the first two letters and insert 'aaa'. This result in the same output but the optimal delta should be insert an 'a' at the 2nd position. This is due to a bad optimization heuristic we are using so we'll plan to fix this in a future release.

The Delta interface is still being finalized so we can still consider adding a delete operation or make deltas easier to read/work with.

@benbro
Copy link
Contributor Author

benbro commented May 15, 2014

The third instruction you're seeing is saying delete the first two letters and insert 'aaa'.

I thought I should process ops in the order received so doesn't it say insert 'aaa', delete the first two letters? Should I first process RetainOp { start=2, end=3 } and only than InsertOp { value="aaa" }?

This result in the same output but the optimal delta should be insert an 'a' at the 2nd position.

Don't you mean that it should be insert an 'a' at the 3rd position?

ot.js uses a very compact ops format
https://github.com/Operational-Transformation/ot.js
Insert op is just the string, retain is the number of chars you skip and delete is the number of chars you delete.

["a"] // insert "a" in empty document
[1, "a"] // retain one char and insert "a"
[1, -1] // retain one char and delete the second char

It also gives you the baseLength so you could make sure two deltas are working on the same initial document.

http://ot.substance.io/visualization/
http://ot.substance.io/demo/

Will you consider using the same format? It seems like a very compact format that is easy to work with in code.

Can you explain how OT should work with the current format quill use?
With ot.js format, I just traverse the document and transform ops of a user against the server.
How do you do it with the following delta?

[ InsertOp { value="aaa" }, RetainOp { start=2, end=3 } ]

The reason I'm asking is I tested the GoInstant quill demo and it seems to work worst than ot.js demo.
I looked at the ops in the console and tried to find why.

@jhchen
Copy link
Member

jhchen commented May 15, 2014

so doesn't it say insert 'aaa', delete the first two letters

Yes this is more specifically correct. I reversed so there's no ambiguity over which two letters you are deleting.

Don't you mean that it should be insert an 'a' at the 3rd position?

Yes.

Will you consider using the same format?

Sure all reasonable ideas are worth considering. It's worth noting that Deltas are objects with useful methods so it will always do worse if we want sensible variable names. Either way the Delta interface is again not finalized and probably not optimal.

How do you do it with the following delta?

Deltas describe the whole document so you start with index 0. You see insert 'aaa' so you insert 'aaa' at index 0. You see retain 2,3 and note 2 > your current index of 0 so you delete characters 0-2 from the original document. You advance to index 3 (keeping one character). With no more instructions if index 3 was not the last index of the document, you delete the rest of the document. You can avoid having to do this yourself most of the time with Delta.apply.

@benbro
Copy link
Contributor Author

benbro commented May 15, 2014

I made another test.
I have "12" in the document.
I'm adding "3" at the end to get "123"

[ InsertOp { value="123" }, RetainOp { start=2, end=3 } ]

Following you instructions:
Start with: "12"
Step 1: InsertOp { value="123" } -> insert "123" at index 0 gives me "12312"
Step 2: RetainOp { start=2, end=3 } -> current index is 0 so I delete characters 0-2 from the original document and get "312" this is clearly wrong.

Can you please explain how it works in this case?

I tested with a longer string "123456789". The last delta always have insert op with the complete string. Is this the non optimal representation you was referring to? In ot.js the last delta will be just [8,"9"].

@jhchen
Copy link
Member

jhchen commented May 15, 2014

Step 2: RetainOp { start=2, end=3 } -> current index is 0 so I delete characters 0-2 from the original document and get "312" this is clearly wrong.

This is why I switched the instructions around earlier. The original character 0-2 is now index 3-5 since a 3 letter string just got inserted. So after step 2 you should have 123.

Is this the non optimal representation you was referring to?

Yes

@benbro
Copy link
Contributor Author

benbro commented May 15, 2014

This is why I switched the instructions around earlier. The original character 0-2 is now index 3-5 since a 3 letter string just got inserted. So after step 2 you should have 123.

I don't understand how it should work.
What are the steps you do to get "123"?
Start with "12"
InsertOp { value="123" } says insert "123" at index 0 so now I have "12312"?
RetainOp { start=2, end=3 } says delete characters 0-2 so I have "312", retain from 2-3 gives me "3".

@jhchen
Copy link
Member

jhchen commented May 15, 2014

Let's start with 'ab' instead of '12' to avoid confusion with inserted text.

  1. Insert '123' should yield 123ab.
  2. Retain (2,3) has two steps
    • Delete characters 0-2 (as explained earlier). This is index 0-2 of the original document which was 'ab' not '123ab'. So we delete 'ab' not '12'.
    • The other step and the rest of this I won't re-explain as there didn't seem to be confusion.

@jhchen jhchen closed this as completed in 336a38e May 15, 2014
edave64 pushed a commit to edave64/quill that referenced this issue Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants