Path related issue with inverted operations #1
Comments
|
Hi there, I appreciate your feedback – thank you! You're right, that does seem like a bug, and I can't really explain why I wrote if (equal(path, opParentPath)) {
if ("li" in op) offset++;
if ("ld" in op) offset--;
}back then. After all, it's been more than a year since I last looked at the code. However, when removing that, tests begin to fail, so that's something I'll have to look into before submitting a fix. Of course, if you already have a fix at hand – or if you're willing to spend a little time on it – pull requests are always welcome. |
|
I'll give it a shot. The offset is used to update the path of operations after 'li' or 'ld' operations occur. For example: <body>
<p id='1'></p>
<p id='2'></p>
<p id='3'></p>
</body>Deleting the second and third 'p' elements would result in the following operations:
This happens because you wrote the code using a top-down approach when handling the generation of the operations. No need for offset if a bottom-up approach would have been chosen for calculating the diffs but there would have probably been other issues :) EDIT 1: Added 'id' to the 2 operations used in the example (forgot to add them initially) |
|
I came up with 2 (+1) solutions: For this repository, building the ops using a bottom-up approach would fix the issue. This means dropping the offset variable and creating the ops from the last element to the first: for (var i = l; i >= 0; --i) {
var newOps = diff(input[i], output[i], [...path, i]);
ops.concat(newOps);
}
return ops;This solution was great, however it does not mix well with inverted 'sd/si' operations (Fixable, however I do not have the time to do it). The second solution (The one I actually used) was not applying the offset if there was no list delete operation within the generated ops. The solution works great for me, however there are scenarios which will render it useless (e.g. ld, li, li, li - the third li would have a bad path). For this, I've kept a path map which was reset after before every optimizeDiff function call. The pathMap remembers the offset and the total number of ops for a certain parent path. It looks like this: var pathMap = {};
[...]
// within the 'li' OR 'ld' op creation code (no reason to do it in the list replace op code)
var offsetKey = JSON.stringify(op.p.slice(0, -1));
var totalOpsKey = offsetKey + "_ops";
// for 'li' - TODO: optimize
pathMap[offsetKey] = typeof pathMap[offsetKey] == "number" ? pathMap[offsetKey] + 1 : 1;
pathMap[totalOpsKey] = typeof pathMap[totalOpsKey] == "number" ? pathMap[totalOpsKey] + 1 : 1;
[...]
// for 'ld' - TODO: optimize
pathMap[offsetKey] = typeof pathMap[offsetKey] == "number" ? pathMap[offsetKey] - 1 : -1;
pathMap[totalOpsKey] = typeof pathMap[totalOpsKey] == "number" ? pathMap[totalOpsKey] + 1 : 1;
[...]
// when creating the 'li' op
if(pathMap[offsetKey] == pathMap[totalOpsKey]) {
op.p[op.p.length - 1] -= pathMap[offsetKey] - 1;
}This solution works great for my use case and as far as I've seen, for this repository as well. However there are these rare scenarios which can cause some mess. The third solution is pretty straight forward. It only adds one function to the code: [...]
{
[...]
for (var i=0; i < l; ++i) {
var newOps = diff(input[i], output[i], [...path, i + offset]);
newOps.forEach(function(op) {
var opParentPath = op.p.slice(0, -1);
if (equal(path, opParentPath)) {
if ("li" in op && mustIncrementOffset(path, op, input, output, ...)) {
offset++;
}
if ("ld" in op) offset--;
}
ops.push(op);
});
}
return ops;
}
[...]
function mustIncrementOffset(/*metadata params*/) {
[...]
return /*boolean*/;
}This is the best solution, however after spending a lot of time on it trying to 'calculate' when to increment the offset with no success, I've decided to go with the second one (I mean, the second solution could be a deciding factor for the mustIncrementOffset function - if there's no list delete op generated, don't increment the offset). I will not be doing a PR since I did not offer any viable solution (100% working for every scenario, including future 'si' / 'sd' operation support (loominc's fork)). |
Hey @kbadk,
The issue I am having is with lines 87-90. The code works fine for every other scenario I've tested, however when trying to apply inverted operations it simply does not work because of the final path value change done at those lines.
Let's look at the following scenario. I want to bold the 'b' letter in the following HTML:
Obtaining:
I'm doing this by running the json0-ot-diff code in the browser and retrieving the diffs between an older version of the JsonML (obtained from the body element) and the newer one (after the bold operation).
The following operations are obtained:
The inverted ops are the following (applied in this exact order):
What is the reason for increasing / decreasing the final element of the path array by 1 for 'li' / 'ld' operations when
if (equal(path, opParentPath))?EDIT 1:
I've just understood the logic of the code, however I still think that there's a bug regarding the scenario above.
The text was updated successfully, but these errors were encountered: