Reusing the same op object for each iteration can result in very weird
behaviors because previously yielded op objects will get a surprise
mutation.
It is unclear why the code was written to reuse the same object. There
was no comment, nor is there a commit message providing rationale (it
has behaved this way since the very first commit). Perhaps the objects
were reused to improve performance (fewer object allocations that need
to be garbage collected). I do expect this change to reduce
performance somewhat, but not enough to warrant reverting this commit.
* code tidy up: always evaluates
* tidy up: is always true
* tidy up: remove unused code
* always true/false variables
* unused variable
* tidy up: remove unused code in caretPosition.js
* for squash: Revert "tidy up: remove unused code in caretPosition.js"
The `if` condition was previously always true, so the body should be
preserved. If the body is preserved, other logic can be deleted. I
opened PR #4845 to clean it all up.
This reverts commit 75b03e5a7d.
* for squash: simplify
* for squash: Explain that the getter is used for its side effects
It's very weird to call a getter without using its return value. Add a
comment explaining why this is done so that the reader doesn't get
confused.
* for squash: Revert "tidy up: remove unused code"
The exception test was the purpose of the code.
This reverts commit 85153b1676.
* for squash: Log the tsort results
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Clearing the authorship colors of a document with at least two authors, and then
undoing that action caused a disconnect from the pad.
This change disallows undoing clearing authorship colors in order to prevent
the problem from affecting users, and adds the relative test coverage.
This is a change of behaviour, and is documented in the changelog.
Fixes#2802 (sidestepping it).
THE BUG - HIGH LEVEL:
- When client A sends out an attribute change, client B applies that change to itself but
also thinks that it made the change itself, which is incorrect. This means that when client B
next makes a change, he will send out that he made the attrib change that A actually made.
- Ex: Have 2 clients on the same pad. Have A apply bold on some text. Next, have B type a character.
B will broadcast that it both added a character AND applied bold, when in reality it did NOT
apply bold at all, that change was done by the other client and this client incorrectly adopted it as its own.
- This root bug behavior results in clients continuing to think that they each made the other client's change,
thus resulting in an infinite loop of changeset spamming that bogs down clients and harms server stability.
THE BUG - IN DEPTH:
- The root issue is in the way that Changesets are combined in Changeset.follow(). Specifically, in the case of a
changeset with ONLY new attrib changes (no text changes) being merged with an identity changeset (has no ops).
- In this case, Changeset.follow() copies the ops of the new CS and fully overrides the other CS.
- applyChangesToBase invokes Changeset.follow to determine the final client document CS state after applying the new CS.
If the final client document CS state is NOT the identity CS, then the client broadcasts that it made a change.
- When client A changes just attribs, client B's applyChangesToBase calls Changeset.follow() and passes client A's
changeset (attrib change) and Client B's current changeset state (identity).
- As per the noted bug, Changeset.follow() returns client A's changeset as a result, causing client B to adopt
client A's changeset as its own document state. Thus, client A ends up thinking it has made client B's changes.
THE FIX:
- Changeset.follow() should NOT copy the ops of the new CS passed in if those changes are only attrib changes.
This allows applyChangesToBase to properly set the client's CS back to the identity after applying an
external attrib change, instead of incorrectly adopting the external client's changes.