Finish documentation after OT refactor #1509
Conversation
Docs: Added docs for code related to OT.
|
Coverage decreased? ;> |
|
🤷♂️ |
|
Don't worry, I will get CC to 100% in next task :). But yeah, code changed a little little, but it shouldn't have an impact on CC. Code was changed to make some parts more clear. |
src/model/liverange.js
Outdated
| * @param {module:engine/model/position~Position} data.sourcePosition Source position for move, remove and reinsert change types. | ||
| * @param {Object} data Object with additional information about the change. | ||
| * @param {null} data.deletionPosition Due to the nature of this event, this property is always set to `null`. It is passed | ||
| * for compatibility with {@link module:engine/model/liverange~LiveRange#event:change:range} event. |
| // Below we take an operation, check its type, then use its parameters in marking (private) methods. | ||
| // The general rule is to not mark elements inside inserted element. All inserted elements are re-rendered. | ||
| // Marking changes in them would cause a "double" changing then. | ||
| // |
There was a problem hiding this comment.
I do a lot of those to make comments more readable and separated from code, to make more breathing room so it is easier to read it all. These are not by mistake.
src/model/liverange.js
Outdated
| /** | ||
| * Checks whether given operation changes something inside the range (even if it does not change boundaries). | ||
| * | ||
| * @ignore |
There was a problem hiding this comment.
Usually we use // comments instead of @ignore.
There was a problem hiding this comment.
I changed all @ignore occurrences in the engine.
| @@ -34,8 +34,7 @@ export default class LivePosition extends Position { | |||
| * @see module:engine/model/position~Position | |||
| * @param {module:engine/model/rootelement~RootElement} root | |||
| * @param {Array.<Number>} path | |||
| * @param {module:engine/model/position~PositionStickiness} [stickiness] Position stickiness. Defaults to `'toNone'`. | |||
| * See {@link module:engine/model/liveposition~LivePosition#stickiness}. | |||
| * @param {module:engine/model/position~PositionStickiness} [stickiness] | |||
There was a problem hiding this comment.
Any docs?
[stickiness='toNone'] Position stickiness. ?
There was a problem hiding this comment.
Stickiness is described in typedef. I could go with something like See {@link ... } but this is a bit stupid cause in generated docs you will have a link to the typedef next to the parameter.
| * | ||
| * For example, if `n` nodes are inserted before the position, the returned position {@link ~Position#offset} will be | ||
| * increased by `n`. If the position was in a merged element, it will be accordingly moved to the new element, etc. | ||
| * |
There was a problem hiding this comment.
I believe that this method is also safe to use it on non-existing positions. I you put such information for the previous method maybe all "safe" methods should get this warning?
| * **Note:** transformation may break one range into multiple ranges (e.g. when a part of the range is | ||
| * moved to a different part of document tree). For this reason, an array is returned by this method and it | ||
| * may contain one or more `Range` instances. | ||
| * |
There was a problem hiding this comment.
One or more ranges may be returned as a result of this transformation.?
src/model/operation/transform.js
Outdated
| * both transformed `operationsA` and transformed `operationsB` are returned. | ||
| * | ||
| * Note, that the first operation in each set should base on the same context. A simplified version of context is | ||
| * {@link module:engine/model/document~Document#version document version}, so the |
There was a problem hiding this comment.
A simplified version of context is document version.
It might be too abstract.
Can you just say:
Note, that the first operation in each set should base on the same document state ({@link module:engine/model/document~Document#version document version}).
?
src/model/operation/transform.js
Outdated
| * Base versions of the transformed operations sets are updated accordingly. For example, if base versions start from `4` | ||
| * and there are three operations in `operationsA` and five operations in `operationsB`, then transformed `operationsA` | ||
| * will start from base version `9` while transformed `operationsB` will start from base version `7`. If no operation was | ||
| * broken into two during transformation, then both sets will end up with an operation that bases on version `11`. |
There was a problem hiding this comment.
I had to read this 3 times to get it. What can be improved: three -> 3, five -> 5. Some "math helpers":
9 (4 base + 5 operations A),7 (4 base + 3 operations B),11 (4 base + 5 operations A + 3 operation B trandormed by A = 4 base + 3 operations B + 3 operation A trandormed by B = 12, what means that the base of the last operation is 11),
There was a problem hiding this comment.
As a note, I just say that describing base versions is difficult, because it is a little abstract concept and the reader won't understand that a version, say, 10 at one client is something different than version 10 on the other client.
So there is a problem, cause if you write too little, the reader will wonder why even care about versions and if you write too much it is too complicated and abstract and too long to comprehend :).
But I tried to clarify this with your suggestions.
| * @param {Boolean} [options.useContext=false] Whether during transformation additional context information should be gathered and used. | ||
| * @param {Boolean} [options.padWithNoOps=false] Whether additional {@link module:engine/model/operation/nooperation~NoOperation}s | ||
| * should be added to the transformation results to force the same last base version for both transformed sets (in case | ||
| * if some operations got broken into multiple operations during transformation). |
There was a problem hiding this comment.
Is there any case where we do not want to pad with no-ops? Undo?
src/model/operation/transform.js
Outdated
| // | ||
| // So, suppose we have sets of two operations each: `operationsA` = `[ a1, a2 ]`, `operationsB` = `[ b1, b2 ]`. | ||
| // | ||
| // Remember, that we can only transform operations that base on the same context. We assert that `a1` and `a2` base on |
There was a problem hiding this comment.
🎉 somebody read it!
| // by `a11'`. Similarly, `a12'` is only transformed by `b1`. This leads to a conclusion that we need to start transforming `a12'` | ||
| // from the moment just after it was broken. So, `a12'` is transformed by `b2`. Now, "the whole" `a1` is transformed | ||
| // by `operationsB`, while all `operationsB` are transformed by "the whole" `a1`. This means that we can continue with | ||
| // following `operationsA` (in our case it is just `a2`). |
There was a problem hiding this comment.
Maybe some ASCII arts?
* -a1-- * -a2- *
| |
b1 b1'
| |
* -a1'- *
|
b2
|
*
Or:
*
/ \
a1 b1
/ \
* *
/ \ / \
a2 b1' a1' b2
/ \ / \
* * *
There was a problem hiding this comment.
Especially some arts for operations splitting would be useful.
src/model/operation/transform.js
Outdated
| }; | ||
|
|
||
| // Initialize context to use during transformations. |
There was a problem hiding this comment.
Not sure if you need to add comments here to EVERYTHING ;)
src/model/operation/transform.js
Outdated
| const opA = operationsA[ i ]; | ||
|
|
||
| // For the "current" operation `a`, get the index of the next operation `b` to transform by. | ||
| const transformByIndex = nextTransformIndex.get( opA ); |
There was a problem hiding this comment.
I am not a big fan of this method. transformByIndex sounds more like a function than property.
src/model/operation/transform.js
Outdated
| nextTransformIndex.set( newOpA, transformByIndex + newOpsB.length ); | ||
| } | ||
|
|
||
| // Update `operationsA` and `operationsB` so that in next loops transformed operations are processed. |
There was a problem hiding this comment.
Maybe it is late, but I have no idea what is going on here.
| context.relations = new Map(); | ||
|
|
||
| // Returns whether given operation `op` has already been undone. | ||
| // | ||
| // This is only used when additional context mode is on (options.useContext == true). |
There was a problem hiding this comment.
Aren't all context.* used only when additional context mode is on?
There was a problem hiding this comment.
context.isStrong is used always
src/model/operation/transform.js
Outdated
| // attributes on the inserted nodes in a way which would make the original operation correct: | ||
| // | ||
| // <p>Fo[z{<$text highlight="red">}x</$text>b]ar</p> <--- Change range `{}` from `red` to `null`. | ||
| // <p>Fo[zxb]ar</p> <--- Now change from `null` to `yellow` is completely fine. |
src/model/operation/transform.js
Outdated
| @@ -500,10 +806,8 @@ function _getComplementaryAttributeOperations( insertOperation, key, newValue ) | |||
| setTransformation( AttributeOperation, MergeOperation, ( a, b ) => { | |||
| const ranges = []; | |||
|
|
|||
| // Case 1: Attribute change on the merged element. In this case, the merged element was moved to graveyard. | |||
| // Case 1: Attribute change on the merged element. In this case, the merged element was moved to the graveyard. | |||
| // An additional attribute operation that will change the (re)moved element needs to be generated. | |||
There was a problem hiding this comment.
Wrong indentation, use spaces instead of tabs.
There was a problem hiding this comment.
It's the same everywhere in the file. I'll change them all.
src/model/operation/transform.js
Outdated
| * Helper function for `AttributeOperation` x `MoveOperation` transformation. | ||
| * | ||
| * Takes the passed `range` and transforms it by move operation `moveOp` in a specific way. Only top-level nodes of `range` | ||
| * are considered to be in the range. If move operation moves nodes from deep from inside of the range, those nodes won't |
There was a problem hiding this comment.
If move operation moves nodes from deep from inside of the range
Is this sentence correct?
src/model/operation/transform.js
Outdated
| * @returns {module:engine/model/operation/moveoperation~MoveOperation} | ||
| * @ignore | ||
| */ | ||
| function _makeMoveOperation( range, targetPosition ) { |
There was a problem hiding this comment.
Maybe move constructor should look like this?
There was a problem hiding this comment.
Yeah, well, maybe but probably nope. Because then you'd be able to pass non-flat range and you'd have to check it. But OTOH it would simplify places where transformation follows this scheme:
- Change
sourcePosition+howManyinto range. - Transform that range.
- Read new
sourcePositionandhowManyfrom the transformed range and change them in the operation.
There was a problem hiding this comment.
Okay I removed this function completely.
Suggested merge commit message (convention)
Other: Updated OT documentation. Closes ckeditor/ckeditor5#4380.