refactor() change common ancenstors returned data to help isInFrontOf

[asturur]

Have findCommonAncestors return 3 arrays, representing, the common ancestors, and the forks of non common ancestors down to the objects themselves.

Rules:

• an object is not parent of itself
• there is always something to return from findCommonAncestors ( at worst 2 disjunted forks )
• an empty array of common ancestors means no common ancestors, regardless of where objects live ( same canvas, different apps entirely )
• EDITED common ancestors can include this | other, forks are completely different

[asturur]

@ShaMan123 this is how i would like to change findCommonAncestors so that we don't have to repeat the logic in isInFronOf. Code amount is the same ( i think i added 3 lines ) but the logic is less.
Also i find the data returned in this case from findCommonAncestors more easily understandble compared to indexes of arrays we don't have anymore.

Let me know what do you think

[ShaMan123]

This is much better!

1. Now I think of another change that will make it more comprehensible.
   I think we should rename ancestors prop to common
   so the return value will be an object of fork, otherFork, common.
   Then we handle the 2 cases of this other that are descendants of each other as follows:
   If b is a child of a then:

var retVal = b.findCommonAncestors(a);
retVal === { fork: [b], otherFork: [], common: [a] }

instead of

var retVal = b.findCommonAncestors(a);
retVal === { fork: [b, a], otherFork: [a], ancestors: [] }

because strictly thinking of the meaning of fork isn't suitable to how it's used currently.

2. The other suggestion is logic. Don't you prefer unshifting the ancestor arrays and removing the equality checks of this === otherAncestor[j] | other=== ancestor[i]
   I think it's less confusing.

3. Maybe add some more comments as it is logically complicated.

4. we might want to rename the method to compareTrees or compareAncestors

[ShaMan123]

updated review comment

[asturur]

The only reason for not unshifiting is not readibility, but is that once you put the object inside the array, you don't know anymore which is a parent and which not. If it stay outside for the internal logic is better.

I have no issue with having the function oriented on commonTree VS commonAncestors, but if there is not a direct practical advantage in doing that now, i prefer to move forward.

[asturur]

How else this function is used? right now just for isInFrontOf and is doing its job fine enough.

[ShaMan123]

Only isInFrontOf uses it. but devs will use it probably. And it is healthier what I suggested.
As you say, we can iterate later on. I don't mind patching it now.
I leave the decision at your hands.

[ShaMan123]

Ahh the return value needs to be changed to utilize the typedef Ancestors

[asturur]

Only isInFrontOf uses it. but devs will use it probably. And it is healthier what I suggested. As you say, we can iterate later on. I don't mind patching it now. I leave the decision at your hands.

What does it mean healtier? in this case we have 3 parts, the common trunk and the branches. in the other case we have a slightly different point of view of the same data structure.
Where is the advantage?

[ShaMan123]

The advantage is that the forks are entirely different.
Currently they might have the same top ancestor. I think it's confusing.

[ShaMan123]

I have in mind what you said about absolutePositioned - once it's out there we can't take it back as easily.
That's why we should commit something mature/final.

[asturur]

no they can't have the same top ancestor right now, how would that happen?

[ShaMan123]

I meant 2 forks can have the same headOfFork.

[ShaMan123]

Pushed 2319d4f
Are you OK with it?

[ShaMan123]

maybe this condition should come after the for loop for readability

[asturur]

I meant 2 forks can have the same headOfFork.

Not sure why that would happen, but is fine by me changing, i wouldn't have re-typed everything, but since you did is fine.
I find the difference a matter of preference.

Pushed 2319d4f
Are you OK with it?

In general try to don't push on others PRs without a prior agreement.