BUGFIX: Insert copied or moved nodes in order of selection #3708

Sebobo · 2024-02-01T14:49:11Z

This reverses the order of nodes for insertAfter (copy & move) operations to prevent the reverse order of insertion due to the way the atomic inserts are done by the change operations.

Also turns insertInto internally into a insertAfter if the selected target parent node already has children. Here the reversion is not necessary as always the last child node is picked to "insert after" and therefore keeping the order of the users selection (clipboard).

Resolves: #3040

Sebobo · 2024-02-01T14:51:38Z

I keep this for now as draft, as I have to check the move operations too.

neos-bot · 2024-02-01T15:00:44Z

🎥 End-to-End Test Recordings

These videos demonstrate the end-to-end tests for the changes in this pull request.

Sebobo · 2024-02-01T16:08:02Z

moveInto doesn't have this issue, as it internally always sets the inserted node to POSITION_LAST

Now debatable why copyInto doesn't do this 🤔

Sebobo · 2024-02-02T08:37:50Z

This is not an elegant change, but I'm out of ideas right now how to do it better without a larger code change that would introduce "batch changes" which carry more information about the ordering of elements.

mhsdesign · 2024-02-07T08:07:21Z

Okay i tested now without this fix first and that were my results: #3040 (comment)

interaction	`8.3`	`paste-in-order`
selection top -> bottom + insert after	❌(reversed)	✅
selection top -> bottom + insert before	✅	✅
selection bottom -> top + insert after	✅	❌ (random?)
selection bottom -> top + insert before	❌ (reversed)	❌ (random?)
selection random + insert after	❌ (order reversed)	🟠 (order)
selection random + insert before	🟠 (order)	🟠 (order)

✅ = expected order like the nodes arranged in the ui
🟠 = order like nodes selected in the neos ui, marked as orange as its not expected that the application will actually remember this.

So the problem with this fix is, the workaround that is currently used to select the nodes from bottom to top will be now reserved which is also mindblowing?

mhsdesign · 2024-02-07T08:10:42Z

Classes/Domain/Model/Changes/CopyInto.php

+            $parentNode = $this->getParentNode();
+            $nodeName = $this->generateUniqueNodeName($parentNode);
+            // If the parent node has children, we copy the node after the last child node to prevent the copied nodes
+            // from being mixed with the existing ones due the duplication of their relative indices.
+            if ($parentNode->hasChildNodes()) {
+                $lastChildNode = array_slice($parentNode->getChildNodes(), -1, 1)[0];
+                $node = $this->getSubject()->copyAfter($lastChildNode, $nodeName);
+            } else {
+                $node = $this->getSubject()->copyInto($parentNode, $nodeName);
+            }


that makes absolutely sense. But i will have to check how its done in Neos9 to align behaviour.

-> yes that seems to be the case. The nodes were always pasted at the end.

And in 8.3 this seems to be a bug:

the nodes should have not been pasted before subpage1.

Sebobo · 2024-02-07T08:25:46Z

@mhsdesign thx for the thorough testing!
Will check the two random cases again. I thought I had verified them too.

I wouldn't do anything about the clipboard order issue as this would be a larger change we can check separately if we can implement this as a fix or feature for future Neos or keep it like it is.

mhsdesign · 2024-02-07T09:43:28Z

We found out that my testing uncovered another problem. Shift click and command click behave different

Sebobo · 2024-02-07T09:43:40Z

OK after a screen share session we found out that there is a difference behaviour on how items are added to the clipboard depending whether one uses Command+Select or Shift+Select. The later seems to add them in the incorrect order to the clipboard and therefore the resulting result of the insert operation seems again random.

Sebobo · 2024-02-07T11:35:38Z

@mhsdesign my latest change fixes that RangeSelect behaviour

grebaldi · 2024-02-13T17:12:00Z

EDIT: The descriptions in this comment are based on false assumptions about the requirements in this PR. For clarification see: #3708 (comment)

Hi @Sebobo,

I have tried to compile some E2E tests for this problem. The original issue #3040 focuses on move and copy operations for content nodes. I based my tests on the existing tests for the document tree, but I expect this to translate well to the content tree. This way I was able to to write up a comprehensive set of cases.

Since this problem space has a couple of dimensions, I ended up with 45 different scenarios. Those test cases alone run for quite a while (~8min on my machine), which is - amongst some other, minor problems - why I didn't push the test code right away.

Nevertheless, I still wanted to share my findings. Since there are lots of parameters, I'll start by describing some background assumptions for the tests:

Let's suppose, we have the following simplified document tree:

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ 🗋 Page D

For the first part of the test matrix, we need to distinguish between two methods of selecting multiple nodes (CMD-Click and SHIFT-Click), both of which allow for different orders of selection (as identified by @mhsdesign: top-down, bottom-up, random):

CMD-Click - We select nodes by clicking on each node individually while holding the CMD(or Ctrl)-key
- top-down - We select first 🗋 Page A, then 🗋 Page B, then 🗋 Page C
- bottom-up - We select first 🗋 Page C, then 🗋 Page B, then 🗋 Page A
- random - We select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A
SHIFT-Click - We select a range of nodes by clicking on a start node and then, while holding the SHIFT-key, we click an end node
- top-down - We click first on 🗋 Page A, then 🗋 Page C
- bottom-up - We click first on 🗋 Page C, then 🗋 Page A

The second part of the test matrix concerns the operations a user can perform with a selection of multiple nodes. Those are basically Move and Copy. Both operations happen in an "insert mode" (AFTER, BEFORE, INTO), which results in a total of 6 possible operations.

The following tables illustrate what the resulting page tree (given the tree from above) is expected to look like after each operation:

Scenario: Move

AFTER BEFORE INTO

🗋 Parent
├─ 🗋 Page D
├─ ❙🗋 Page A
├─ ❙🗋 Page B
└─ ❙🗋 Page C

🗋 Parent
├─ ❙🗋 Page B
├─ ❙🗋 Page C
├─ ❙🗋 Page D
└─ 🗋 Page A

🗋 Parent
└─ ❙🗋 Page D
    ├─ ❙🗋 Page A
    ├─ ❙🗋 Page B
    └─ ❙🗋 Page C

We have moved 🗋 Page A..C AFTER 🗋 Page D We have moved 🗋 Page B..D BEFORE 🗋 Page A We have moved 🗋 Page A..C INTO 🗋 Page D

Scenario: Copy

AFTER BEFORE INTO

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
├─ 🗋 Page D
├─ ❙🗋 Page A
├─ ❙🗋 Page B
└─ ❙🗋 Page C

🗋 Parent
├─ ❙🗋 Page B
├─ ❙🗋 Page C
├─ ❙🗋 Page D
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ 🗋 Page D

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ ❙🗋 Page D
    ├─ ❙🗋 Page A
    ├─ ❙🗋 Page B
    └─ ❙🗋 Page C

We have copied 🗋 Page A..C AFTER 🗋 Page D We have copied 🗋 Page B..D BEFORE 🗋 Page A We have copied 🗋 Page A..C INTO 🗋 Page D

Based on these assumptions, I ran my 45 test cases on both the 8.3 branch and the PR-branch bugfix/3040-paste-in-order. The following tables show the respective test results. ✅ means that the resulting page tree meets the expectation from above, ❌ means that it doesn't.

Test results on branch 8.3 (before the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	✅	❌	❌	✅	❌	❌	✅	❌	✅
	random	❌	❌	❌	❌	❌	❌	❌	❌	✅
SHIFT-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
SHIFT-Click	bottom-up	❌	❌	❌	❌	❌	❌	❌	❌	✅

Test results on branch bugfix/3040-paste-in-order (after the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	✅	✅	✅	✅	✅	✅
	bottom-up	✅	❌	❌	❌	❌	❌	❌	❌	❌
	random	❌	❌	❌	❌	❌	❌	❌	❌	❌
SHIFT-Click	top-down	❌	✅	✅	✅	✅	✅	✅	✅	✅
SHIFT-Click	bottom-up	✅	❌	❌	❌	❌	❌	❌	❌	❌

Both test runs result in 27 cases failing (though different cases fail).

I'm not sure whether it would be feasible to fix all those cases. If so, I'm also not sure whether it would be wise to add all cases to our test suite - because that's +8min on all CI runs :/

But I hope that I could give some insight 😅

(I will share the test code as well of course - I'll probably open a separate PR, so it doesn't clutter up this one)

Sebobo · 2024-02-13T21:12:07Z

Hi @grebaldi,

wow, thx for the thorough test!

The intention of the PR is actually for the bottom-to-top case to also reflect the bottom-to-top order and not invert it to top-to-bottom. Of course this is debatable, but I somehow preferred keeping the order of selection.

This should make most cases green except the random one.
We. An only make the whole thing more sane if we introduce a new batch operation instead of the atomic operations which simply cannot produce something plausible as the UI needs to know the quirks of the backend right now.
When the nodes are not on the same level, we are also out of luck to find any useful order except the order of selection.

markusguenther · 2024-02-14T08:10:59Z

wow, thx for the thorough test!

That was exactly the same thought when I saw the e-mail yesterday in the evening. Just WOW, and thank you @grebaldi for all your detailed descriptions and comments. You are so valuable and awesome :)

mhsdesign · 2024-02-14T08:27:52Z

Yes hats of to your testings @grebaldi :D
Its very unfortunate that the test runs take so long and force us to strip them down. But thank you for writing them as we currently have no tests for batch moving/copy i assume?
It would be great to have the happy cases (top-down) running in ci i think.

Also it seems your tests uncovered that Drag & Drop after does not behave as expected, so it would be great to have that corrected, that all the happy cases work?

grebaldi · 2024-02-14T10:24:17Z

@Sebobo:

The intention of the PR is actually for the bottom-to-top case to also reflect the bottom-to-top order and not invert it to top-to-bottom. Of course this is debatable, but I somehow preferred keeping the order of selection.

Whoops, how did I miss that? Sorry for the confusion, I should have guessed the intention from the title of this PR 🤦

Well, my intuition went exactly the other way 😅, but I understand the technical limitations of course.

So, just to clearify the expected behavior given the tree:

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ 🗋 Page D

If we CMD-Click-select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A, and then drag all three nodes into 🗋 Page D, the resulting tree is expected to look like this?:

🗋 Parent
└─ ❙🗋 Page D
    ├─ ❙🗋 Page B
    ├─ ❙🗋 Page C
    └─ ❙🗋 Page A

(With all other cases resulting in an analogous order?)

If so, I will adjust my test cases accordingly and report back :)

@mhsdesign:

[...] we currently have no tests for batch moving/copy i assume?

Well, there's treeMultiselect.e2e.js, but it doesn't assert the resulting order of nodes.

Its very unfortunate that the test runs take so long and force us to strip them down.

It may be possible to run some of the tests conditionally. I believe we can tell Circle CI / Github Actions to filter the tests based on whether a specific label is set on the PR. Or maybe we could run specific tests by prompting some bot? Don't know exactly how to set up something like that, but it would allow us to keep the average push/PR test-runs small and only run more thourough tests when they're actually needed. For the release branches, we could run the entire, much more comprehensive test suite which would greatly increase our certainty.

Sebobo · 2024-02-14T10:44:43Z

If we CMD-Click-select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A, and then drag all three nodes into 🗋 Page D, the resulting tree is expected to look like this?:

Yes

grebaldi · 2024-02-14T17:00:29Z

Here are the updated test results:

Test results on branch 8.3 (before the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	❌	✅	✅	❌	✅	✅	❌	✅	❌
	random	❌	✅	❌	❌	✅	❌	❌	✅	❌
SHIFT-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
SHIFT-Click	bottom-up	❌	❌	❌	❌	❌	❌	❌	❌	❌

Test results on branch bugfix/3040-paste-in-order (after the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	❌	✅	✅	✅	✅	✅	✅	✅
	bottom-up	❌	❌	✅	✅	✅	✅	✅	✅	✅
	random	❌	❌	❌	✅	✅	❌	✅	✅	✅
SHIFT-Click	top-down	❌	❌	✅	✅	✅	✅	✅	✅	✅
SHIFT-Click	bottom-up	❌	❌	✅	✅	✅	✅	✅	✅	✅

There are only 12 remaining failures. Almost the entire Drag & Drop - BEFORE-column appears to consist of regressions.

Sebobo · 2024-02-15T07:24:58Z

@grebaldi thx for the update :)

I will check on the drag & drop, I didn't even expect it to behave different so I didn't test it before 😔

Resolves: #3040

…hildnodes Without this change the copied nodes get their „index“ copied too, which could position them in between existing nodes and creating a seemingly random result for the editor. Resolves: #3040

…ttom to top Relates: #3040

Sebobo · 2024-04-10T12:15:33Z

@grebaldi I finally got to make the adjustment for the drag & drop part. With that The remaining red crosses are checks for me.
Though I couldn't reproduce the issue you had with the "before" and "into" column which both worked fine for me, of course under consideration that the nodes should be inserted in the order of selection.

grebaldi

#3040 Selection order of nodes is preserved when selecting multiple tree-nodes...
 ✓ ...in top-down order via CMD-click and moving them AFTER another node via DND
 ✓ ...in top-down order via CMD-click and moving them BEFORE another node via DND
 ✓ ...in top-down order via CMD-click and moving them INTO another node via DND
 ✓ ...in top-down order via CMD-click and moving them AFTER another node via Cut&Paste
 ✓ ...in top-down order via CMD-click and moving them BEFORE another node via Cut&Paste
 ✓ ...in top-down order via CMD-click and moving them INTO another node via Cut&Paste
 ✓ ...in top-down order via CMD-click and copying them AFTER another node via Copy&Paste
 ✓ ...in top-down order via CMD-click and copying them BEFORE another node via Copy&Paste
 ✓ ...in top-down order via CMD-click and copying them INTO another node via Copy&Paste
 ✓ ...in bottom-up order via CMD-click and moving them AFTER another node via DND
 ✓ ...in bottom-up order via CMD-click and moving them BEFORE another node via DND
 ✓ ...in bottom-up order via CMD-click and moving them INTO another node via DND
 ✓ ...in bottom-up order via CMD-click and moving them AFTER another node via Cut&Paste
 ✓ ...in bottom-up order via CMD-click and moving them BEFORE another node via Cut&Paste
 ✓ ...in bottom-up order via CMD-click and moving them INTO another node via Cut&Paste
 ✓ ...in bottom-up order via CMD-click and copying them AFTER another node via Copy&Paste
 ✓ ...in bottom-up order via CMD-click and copying them BEFORE another node via Copy&Paste
 ✓ ...in bottom-up order via CMD-click and copying them INTO another node via Copy&Paste
 ✓ ...in a random order via CMD-click and moving them AFTER another node via DND
 ✓ ...in a random order via CMD-click and moving them BEFORE another node via DND
 ✓ ...in a random order via CMD-click and moving them INTO another node via DND
 ✓ ...in a random order via CMD-click and moving them AFTER another node via Cut&Paste
 ✓ ...in a random order via CMD-click and moving them BEFORE another node via Cut&Paste
 ✓ ...in a random order via CMD-click and moving them INTO another node via Cut&Paste
 ✓ ...in a random order via CMD-click and moving them AFTER another node via Copy&Paste
 ✓ ...in a random order via CMD-click and copying them BEFORE another node via Copy&Paste
 ✓ ...in a random order via CMD-click and copying them INTO another node via Copy&Paste
 ✓ ...in top-down order via SHIFT-click and moving them AFTER another node via DND
 ✓ ...in top-down order via SHIFT-click and moving them BEFORE another node via DND
 ✓ ...in top-down order via SHIFT-click and moving them INTO another node via DND
 ✓ ...in top-down order via SHIFT-click and moving them AFTER another node via Cut&Paste
 ✓ ...in top-down order via SHIFT-click and moving them BEFORE another node via Cut&Paste
 ✓ ...in top-down order via SHIFT-click and moving them INTO another node via Cut&Paste
 ✓ ...in top-down order via SHIFT-click and copying them AFTER another node via Copy&Paste
 ✓ ...in top-down order via SHIFT-click and copying them BEFORE another node via Copy&Paste
 ✓ ...in top-down order via SHIFT-click and copying them INTO another node via Copy&Paste
 ✓ ...in bottom-up order via SHIFT-click and moving them AFTER another node via DND
 ✓ ...in bottom-up order via SHIFT-click and moving them BEFORE another node via DND
 ✓ ...in bottom-up order via SHIFT-click and moving them INTO another node via DND
 ✓ ...in bottom-up order via SHIFT-click and moving them AFTER another node via Cut&Paste
 ✓ ...in bottom-up order via SHIFT-click and moving them BEFORE another node via Cut&Paste
 ✓ ...in bottom-up order via SHIFT-click and moving them INTO another node via Cut&Paste
 ✓ ...in bottom-up order via SHIFT-click and copying them AFTER another node via Copy&Paste
 ✓ ...in bottom-up order via SHIFT-click and copying them BEFORE another node via Copy&Paste
 ✓ ...in bottom-up order via SHIFT-click and copying them INTO another node via Copy&Paste

👍 🎉 👍

Thanks a lot for the effort, @Sebobo!

Though I couldn't reproduce the issue you had with the "before" and "into" column which both worked fine for me, [...]

The "before"-column is now green - so no issues there. The two red ones in the "into"-column were - as I now realized after revisiting the tests - in fact my fault. Those were most likely copy&paste errors - I seem to have lost track given the amount of scenarios. My apologies for that! I hope this didn't cause you too much trouble.

Sebobo self-assigned this Feb 1, 2024

github-actions bot added Bug Label to mark the change as bugfix 8.3 labels Feb 1, 2024

Sebobo changed the title ~~BUGFIX: Insert copied nodes in order of selection~~ BUGFIX: Insert copied or moved nodes in order of selection Feb 1, 2024

Sebobo requested review from markusguenther, grebaldi and mhsdesign February 2, 2024 08:35

Sebobo marked this pull request as ready for review February 2, 2024 08:35

mhsdesign reviewed Feb 7, 2024

View reviewed changes

Sebobo requested a review from mhsdesign February 7, 2024 11:35

Sebobo added 3 commits April 10, 2024 12:59

BUGFIX: Insert nodes in order of selection on „insertAfter“

29d18d5

Resolves: #3040

BUGFIX: Prevent index mixup when inserting nodes „into“ a node with c…

445e275

…hildnodes Without this change the copied nodes get their „index“ copied too, which could position them in between existing nodes and creating a seemingly random result for the editor. Resolves: #3040

BUGFIX: Reverse items added to clipboard if they are selected from bo…

157521c

…ttom to top Relates: #3040

Sebobo force-pushed the bugfix/3040-paste-in-order branch from 5ed9fe9 to f552dae Compare April 10, 2024 12:14

grebaldi approved these changes Apr 10, 2024

View reviewed changes

BUGFIX: Insert tree items in selected order on drag & drop

ceeccbc

Sebobo force-pushed the bugfix/3040-paste-in-order branch from f552dae to ceeccbc Compare April 10, 2024 19:55

Sebobo merged commit 994f4e1 into 8.3 Apr 10, 2024
10 checks passed

Sebobo deleted the bugfix/3040-paste-in-order branch April 10, 2024 20:13

mhsdesign mentioned this pull request Apr 20, 2024

Upmgerge #3708 to Neos 9 #3768

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Insert copied or moved nodes in order of selection #3708

BUGFIX: Insert copied or moved nodes in order of selection #3708

Sebobo commented Feb 1, 2024 •

edited

Loading

Sebobo commented Feb 1, 2024

neos-bot commented Feb 1, 2024 •

edited

Loading

Sebobo commented Feb 1, 2024 •

edited

Loading

Sebobo commented Feb 2, 2024

mhsdesign commented Feb 7, 2024

mhsdesign Feb 7, 2024 •

edited

Loading

Sebobo commented Feb 7, 2024

mhsdesign commented Feb 7, 2024

Sebobo commented Feb 7, 2024

Sebobo commented Feb 7, 2024

grebaldi commented Feb 13, 2024 •

edited

Loading

Sebobo commented Feb 13, 2024

markusguenther commented Feb 14, 2024

mhsdesign commented Feb 14, 2024

grebaldi commented Feb 14, 2024

Sebobo commented Feb 14, 2024

grebaldi commented Feb 14, 2024

Sebobo commented Feb 15, 2024

Sebobo commented Apr 10, 2024

grebaldi left a comment

BUGFIX: Insert copied or moved nodes in order of selection #3708

BUGFIX: Insert copied or moved nodes in order of selection #3708

Conversation

Sebobo commented Feb 1, 2024 • edited Loading

Sebobo commented Feb 1, 2024

neos-bot commented Feb 1, 2024 • edited Loading

Sebobo commented Feb 1, 2024 • edited Loading

Sebobo commented Feb 2, 2024

mhsdesign commented Feb 7, 2024

mhsdesign Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Sebobo commented Feb 7, 2024

mhsdesign commented Feb 7, 2024

Sebobo commented Feb 7, 2024

Sebobo commented Feb 7, 2024

grebaldi commented Feb 13, 2024 • edited Loading

Sebobo commented Feb 13, 2024

markusguenther commented Feb 14, 2024

mhsdesign commented Feb 14, 2024

grebaldi commented Feb 14, 2024

Sebobo commented Feb 14, 2024

grebaldi commented Feb 14, 2024

Sebobo commented Feb 15, 2024

Sebobo commented Apr 10, 2024

grebaldi left a comment

Choose a reason for hiding this comment

Sebobo commented Feb 1, 2024 •

edited

Loading

neos-bot commented Feb 1, 2024 •

edited

Loading

Sebobo commented Feb 1, 2024 •

edited

Loading

mhsdesign Feb 7, 2024 •

edited

Loading

grebaldi commented Feb 13, 2024 •

edited

Loading