Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Additional improvements in OT #1498

Merged
merged 14 commits into from
Aug 9, 2018
Merged

Additional improvements in OT #1498

merged 14 commits into from
Aug 9, 2018

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Aug 9, 2018

Suggested merge commit message (convention)

Other: Added MergeOperation#howMany and SplitOperation#howMany. Improvements in Position and Range transforming functions. Improvements in OT algorithms. Closes ckeditor/ckeditor5#4394. Closes ckeditor/ckeditor5#4396. Closes ckeditor/ckeditor5#4398.

@coveralls
Copy link

coveralls commented Aug 9, 2018

Coverage Status

Coverage increased (+0.007%) to 98.633% when pulling da9b4ac on t/1488 into 74f00e9 on master.

@@ -30,12 +30,13 @@ export default class MergeOperation extends Operation {
*
* @param {module:engine/model/position~Position} sourcePosition Position inside the merged element. All nodes from that
* element after that position will be moved to {@link ~#targetPosition}.
* @param {Number} howMany
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs?


updateRelations( context, opA, opB );
}
for ( let j = transformFrom; j < operationsB.length; ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked it could be simplified.

//
// So, the final solution is to just expand the original range, but "fix" all the nodes that have "incorrect" `oldValue`.
// For example, in the highlight case above, we would need an operation to change `highlight="red"` to `null` so that
// the original attribute operation `null` -> `yellow` would be correct.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example is needed here.

// Unwrap:
// <p>Foo</p><p>Bar</p><p>Xyz</p>
//
// Wrap with stronger wrap (`a`):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"(a)" can be removed or replaced with "(strong operation A)".

@@ -77,6 +79,10 @@ export function transformSets( operationsA, operationsB, options ) {
return { operationsA, operationsB };
}

for ( const op of operationsA ) {
op.transformBy = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeakMap?

@pjasiun pjasiun merged commit e334195 into master Aug 9, 2018
@pjasiun pjasiun deleted the t/1488 branch August 9, 2018 15:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants