insertChild() wrong index when re-inserting item into parent with higher index

[georeith]

When using Item#insertChild() on an item and specifying its current parent but at an index n where n > Item#index the resulting index is n - 1:

Example of issue

This is not the case for addLayer() which makes me think the index correcting code post removal of the item in Item#insertChildren() is to blame as Project#addLayer() just removes and splices and ends up with the correct result:

Project#addLayer() test

[lehni]

Since the item is already in the list, this case is not so clear: In order to be inserted into children list at a different index, the item has to be removed from it first. When you remove an item, the indices of all items above it shift down by one. And if the insertion index is above its current index, you'd assume it is in relation to these items that now shift, so one can argue that it makes sense to then also adjust the insertion index. Do you see what I mean?

[georeith]

@lehni I do see what you mean. Although its strange that Projects children and other Items children behave differently in this regard.

It makes more sense to me to guarantee the index provided. It is expected that when you splice into array that other items shift and thats fine, but I would always expect my spliced item to be at the index specified.

I can see what you are trying to do though where when you Array#splice() in JS the item whos index you are taking goes ahead of it.

[georeith]

Actually thinking about this more and how Array#splice() operates I think you are right in the current implementation of Item#insertChildren()... I think its Project#addLayer() that gets it wrong.

I imagine the most common use case is I want this item to be above or below something else... in which case for above you would give the target items index + 1, to be below the target items index, just like array splice.

I can see how it would be weird that the order would be different depending on whether the item is already a child or not, and think you just shouldn't care about what index it lands at, just that it lands in the correct order.

[lehni]

The reason why it's behaving differently right now is this line of code:
https://github.com/paperjs/paper.js/blob/develop/src/item/Item.js#L2346
And the calculation of shift here:
https://github.com/paperjs/paper.js/blob/develop/src/item/Item.js#L2343

I can't decide which approach is better... Both argumentations are valid, but I think I agree with you that the shift is more elegant, so I shall port this to insertLayer().

You should then be able to use insertAbove() / insertBelow() for layers correctly also. This is currently probably not the case, due to the lack of this code in insertLayer().

[lehni]

Here an example with layers that currently fails.

[georeith]

@lehni

Hmmm so thinking about this some more. I'm not so sure anymore. The issue I'm now seeing is that the inverse of insertChildren isn't the items old index.

I have 4 items.

• The 4th has an index of 3:
• I tell the 4th to insert at index 1.
• I tell the 4th to insert at index 3 (effectively the inverse).
• The 4th index is 2.

example

That does seem strange and hard to reason about. There is also the case that I tell it to move to one above its current index or its current index and it always stays in place:

I have 4 items:

• The 2nd has an index of 1.
• I tell the 2nd to insert at index 2.
• The 2nd index is 1.
• I tell the 2nd to insert at index 1.
• The 2nd index is 1.

example

In that case it effectively does nothing which is odd behaviour. To move up it has to be its index + 2.

Perhaps insertLayer() had it right and it should behave with guaranteed index (0 -> max index), and have insertAbove() and insertBelow() as what you should use if you care about putting a layer above or below another but not the actual index (they do the shift calculation before). Allowing you to do it both ways depending on your use case.

[lehni]

Yes I agree, I think you're right about this... I was just looking into how jQuery and the DOM handles this case, but the functionality is not even provided, so I think we can make up our own rules here.