Conversation
After thoroughly thinking this through, I decided that moving nodes across dimensions is effectively a copy followed by a delete, thus it CHANGES IDENTITIES of nodes then. This means it will work in all cases, even if the moved node already existed with the same ID in the target dimension. Lateron, we additionally need to expose CreateNodeVariant (which creates connected variants) in the UI as well. Resolves: #4614
skurfuerst
force-pushed
the
90-fix-cut-copy-paste-across-dimensions
branch
from
9819aec to
33df996
Compare
| if (!$precedingSibling->dimensionSpacePoint->equals($subject->dimensionSpacePoint)) { | ||
| // WORKAROUND for MOVE ACROSS DIMENSIONS: | ||
| // - we want it to work like a copy/paste, followed by an original delete. | ||
| // - This is to ensure the user can use it as expected from text editors, where context | ||
| // is not preserved during cut/paste. | ||
| // - LATERON, we need to expose CreateNodeVariant (which creates connected variants) in the UI as well. | ||
| $command = CopyNodesRecursively::createFromSubgraphAndStartNode( | ||
| $contentRepository->getContentGraph($subject->workspaceName)->getSubgraph( | ||
| $subject->dimensionSpacePoint, | ||
| VisibilityConstraints::withoutRestrictions() | ||
| ), | ||
| $subject->workspaceName, | ||
| $subject, | ||
| // NOTE: in order to be able to copy/paste across dimensions, we need to use | ||
| // the TARGET NODE's DimensionSpacePoint to create the node in the target dimension. | ||
| OriginDimensionSpacePoint::fromDimensionSpacePoint($precedingSibling->dimensionSpacePoint), | ||
| $parentNodeOfPreviousSibling->aggregateId, | ||
| $succeedingSibling?->aggregateId | ||
| ); | ||
|
|
||
| $contentRepository->handle($command); | ||
|
|
||
| $command = RemoveNodeAggregate::create( | ||
| $subject->workspaceName, | ||
| $subject->aggregateId, | ||
| $subject->dimensionSpacePoint, | ||
| NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS, | ||
| ); | ||
| $contentRepository->handle($command); | ||
| } else { | ||
| $command = MoveNodeAggregate::create( | ||
| $subject->workspaceName, | ||
| $subject->dimensionSpacePoint, | ||
| $subject->aggregateId, | ||
| RelationDistributionStrategy::STRATEGY_GATHER_ALL, | ||
| $hasEqualParentNode ? null : $parentNodeOfPreviousSibling->aggregateId, | ||
| $precedingSibling->aggregateId, | ||
| $succeedingSibling?->aggregateId, | ||
| ); | ||
| $contentRepository->handle($command); | ||
| } |
There was a problem hiding this comment.
im not sure i understand the full implications of this ... as far as i know is move node super well tested and supports a variety of cases to distribute node aggregates and i dont know if we want to trade that in for the use of this CopyNodesRecursively plus removing the original node?
isnt this supported already by MoveNodeAggregate or do we need to extend the core?
|
Found an alternative fix in neos/neos-development-collection#4614 (comment) |
|
As elaborated in neos/neos-development-collection#5475 i could reproduce one super edge case where moving nodes does not work because they have been varied already, but mostly the current behaviour is stable. I think we dont need this change for now and as i discussed with you we want to go into the direction first to prevent impossible behaviour of cut instead of magically making every edgecase work. |
2 participants
related neos/neos-development-collection#4614
After thoroughly thinking this through, I decided
that moving nodes across dimensions is effectively a copy followed by
a delete, thus it CHANGES IDENTITIES of nodes then.
This means it will work in all cases, even if the moved node already
existed with the same ID in the target dimension.
Lateron, we additionally need to expose CreateNodeVariant (which creates
connected variants) in the UI as well.
Resolves: #4614