From 00759cea193c287f20189bab74e66cb412c424d4 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Fri, 30 Dec 2022 15:12:01 +0100 Subject: [PATCH 1/5] return unresolved changes from `DefaultStateBasedChangeResolutionStrategy` to hide usage of internal intermediate resource set --- ...ltStateBasedChangeResolutionStrategy.xtend | 2 +- .../StateBasedChangeResolutionStrategy.java | 53 +++++++++++++++++++ .../StateBasedChangeResolutionStrategy.xtend | 33 ------------ .../BasicStateChangePropagationTest.xtend | 6 ++- .../StateChangePropagationTest.xtend | 5 +- .../views/impl/ChangeDerivingViewTest.java | 7 +-- 6 files changed, 65 insertions(+), 41 deletions(-) create mode 100644 bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.java delete mode 100644 bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.xtend diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/DefaultStateBasedChangeResolutionStrategy.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/DefaultStateBasedChangeResolutionStrategy.xtend index 51c6e31f9e..69675d93e2 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/DefaultStateBasedChangeResolutionStrategy.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/DefaultStateBasedChangeResolutionStrategy.xtend @@ -93,7 +93,7 @@ class DefaultStateBasedChangeResolutionStrategy implements StateBasedChangeResol changeRecorder.beginRecording changeRecorder.addToRecording(resource) function.apply() - return changeRecorder.endRecording + return changeRecorder.endRecording.unresolve } } diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.java b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.java new file mode 100644 index 0000000000..a0cbe75393 --- /dev/null +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.java @@ -0,0 +1,53 @@ +package tools.vitruv.framework.views.changederivation; + +import org.eclipse.emf.ecore.resource.Resource; + +import tools.vitruv.change.composite.description.VitruviusChange; + +/** + * Strategy for resolving state-based changes to individual change sequences. + * This strategy is used by the domains when there are no change sequences + * available and changes need to be propagated based on the difference between + * the old and new state. + */ +public interface StateBasedChangeResolutionStrategy { + + /** + * Resolves the state-based delta of two resources and returns the correlating + * change sequences. Changes must use hierarchical IDs and are returned + * unresolved. The {@code oldState} remains unmodified from calling this method. + * + * @param newState is the new state of the resource, must not be + * null and must not contain proxies. + * @param oldState is the current or old state of the resource, must not be + * null and must not contain proxies. + * @return a unresolved {@link VitruviusChange} that contains the individual + * change sequence. + */ + VitruviusChange getChangeSequenceBetween(Resource newState, Resource oldState); + + /** + * Resolves the state-based delta for creating the given resource and returns + * the correlating change sequences. Changes must use hierarchical IDs and are + * returned unresolved. + * + * @param newState is the new state of the resource, must not be + * null and must not contain proxies. + * @return a unresolved {@link VitruviusChange} that contains the individual + * change sequence. + */ + VitruviusChange getChangeSequenceForCreated(Resource newState); + + /** + * Resolves the state-based delta for deleting the given resource and returns + * the correlating change sequences. Changes must use hierarchical IDs and are + * returned unresolved. The {@code oldState} remains unmodified from calling + * this method. + * + * @param oldState is the new state of the resource, must not be + * null and must not contain proxies. + * @return a unresolved {@link VitruviusChange} that contains the individual + * change sequence. + */ + VitruviusChange getChangeSequenceForDeleted(Resource oldState); +} diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.xtend deleted file mode 100644 index 4ccd82a26c..0000000000 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/changederivation/StateBasedChangeResolutionStrategy.xtend +++ /dev/null @@ -1,33 +0,0 @@ -package tools.vitruv.framework.views.changederivation - -import org.eclipse.emf.ecore.resource.Resource -import tools.vitruv.change.composite.description.VitruviusChange - -/** - * Strategy for resolving state-based changes to individual change sequences. - * This strategy is used by the domains when there are no change sequences available and changes need to be propagated based on the difference between the old and new state. - */ -interface StateBasedChangeResolutionStrategy { - - /** - * Resolves the state-based delta of two resources and returns the correlating change sequences. - * @param newState is the new state of the resource, must not be null and must not contain proxies. - * @param oldState is the current or old state of the resource, must not be null and must not contain proxies. - * @return a {@link VitruviusChange} that contains the individual change sequence. - */ - def VitruviusChange getChangeSequenceBetween(Resource newState, Resource oldState) - - /** - * Resolves the state-based delta for creating the given resource and returns the correlating change sequences. - * @param newState is the new state of the resource, must not be null and must not contain proxies. - * @return a {@link VitruviusChange} that contains the individual change sequence. - */ - def VitruviusChange getChangeSequenceForCreated(Resource newState) - - /** - * Resolves the state-based delta for deleting the given resource and returns the correlating change sequences. - * @param oldState is the new state of the resource, must not be null and must not contain proxies. - * @return a {@link VitruviusChange} that contains the individual change sequence. - */ - def VitruviusChange getChangeSequenceForDeleted(Resource oldState) -} diff --git a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/BasicStateChangePropagationTest.xtend b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/BasicStateChangePropagationTest.xtend index 6f6be42dc7..d13b2c067a 100644 --- a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/BasicStateChangePropagationTest.xtend +++ b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/BasicStateChangePropagationTest.xtend @@ -163,7 +163,8 @@ class BasicStateChangePropagationTest extends StateChangePropagationTest { val validationResourceSet = new ResourceSetImpl().withGlobalFactories() val oldState = validationResourceSet.getResource(testUri, true) - val changes = strategyToTest.getChangeSequenceBetween(-modelResource, oldState) + val unresolvedChanges = strategyToTest.getChangeSequenceBetween(-modelResource, oldState) + val changes = unresolvedChanges.resolveAndApply(validationResourceSet) switch (strategyToTest.useIdentifiers) { case ONLY, case WHEN_AVAILABLE: { @@ -251,7 +252,8 @@ class BasicStateChangePropagationTest extends StateChangePropagationTest { val validationResourceSet = new ResourceSetImpl().withGlobalFactories() val oldState = validationResourceSet.getResource(testUri, true) - val changes = strategyToTest.getChangeSequenceBetween(-modelResource, oldState) + val unresolvedChanges = strategyToTest.getChangeSequenceBetween(-modelResource, oldState) + val changes = unresolvedChanges.resolveAndApply(validationResourceSet) switch (strategyToTest.useIdentifiers) { case ONLY, case WHEN_AVAILABLE: { diff --git a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/StateChangePropagationTest.xtend b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/StateChangePropagationTest.xtend index a375fd4da0..7add7659f2 100644 --- a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/StateChangePropagationTest.xtend +++ b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/changederivation/StateChangePropagationTest.xtend @@ -97,8 +97,9 @@ abstract class StateChangePropagationTest { protected def compareChanges(Resource model, Resource checkpoint, StateBasedChangeResolutionStrategy strategyToTest) { model.save(null) val deltaBasedChange = resourceSet.endRecording - val stateBasedChange = strategyToTest.getChangeSequenceBetween(model, checkpoint) - assertNotNull(stateBasedChange) + val unresolvedStateBasedChange = strategyToTest.getChangeSequenceBetween(model, checkpoint) + assertNotNull(unresolvedStateBasedChange) + val stateBasedChange = unresolvedStateBasedChange.resolveAndApply(checkpoint.resourceSet) val message = getTextualRepresentation(stateBasedChange, deltaBasedChange) val stateBasedChangedObjects = stateBasedChange.affectedAndReferencedEObjects val deltaBasedChangedObjects = deltaBasedChange.affectedAndReferencedEObjects diff --git a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java index 93e0e38735..369bef4f4b 100644 --- a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java +++ b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java @@ -206,15 +206,16 @@ public void commitChanges() throws Exception { ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); verify(mockChangeableViewSource).propagateChange(argument.capture()); + VitruviusChange change = argument.getValue().resolveAndApply(root.eResource().getResourceSet()); InsertRootEObject expectedChange = RootFactory.eINSTANCE.createInsertRootEObject(); expectedChange.setNewValue(root); expectedChange.setUri(testResourceUriString); - assertThat(argument.getValue().getEChanges().size(), is(3)); // Create, Insert, ReplaceId - assertThat(argument.getValue().getEChanges().get(1), + assertThat(change.getEChanges().size(), is(3)); // Create, Insert, ReplaceId + assertThat(change.getEChanges().get(1), equalsDeeply(expectedChange, ignoringFeatures(EobjectPackage.eINSTANCE.getEObjectAddedEChange_NewValueID(), RootPackage.eINSTANCE.getRootEChange_Resource()))); - assertThat(argument.getValue().getEChanges().get(2), instanceOf(ReplaceSingleValuedEAttribute.class)); + assertThat(change.getEChanges().get(2), instanceOf(ReplaceSingleValuedEAttribute.class)); } } } From c35e5d340c15dcab64bab82f7e253438ad674c87 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Fri, 30 Dec 2022 16:26:47 +0100 Subject: [PATCH 2/5] add indirection in view API to allow view type to transform view changes before propagating then to the underlying model --- .../framework/views/CommittableView.java | 13 ++--- .../views/impl/ChangeDerivingView.xtend | 10 ++-- .../views/impl/ChangeRecordingView.xtend | 3 +- .../views/impl/IdentityMappingViewType.xtend | 7 +++ .../views/impl/ViewCreatingViewType.java | 17 ++++++- .../views/impl/ChangeDerivingViewTest.java | 48 ++++++++++++------ .../views/impl/ChangeRecordingViewTest.java | 50 +++++++++++-------- .../framework/vsum/VirtualModelTest.xtend | 3 +- 8 files changed, 95 insertions(+), 56 deletions(-) diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/CommittableView.java b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/CommittableView.java index 002c7f83b8..e77aa4c771 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/CommittableView.java +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/CommittableView.java @@ -1,9 +1,5 @@ package tools.vitruv.framework.views; -import java.util.List; - -import tools.vitruv.change.composite.description.PropagatedChange; - /** * A {@link View} that allows to commit its changes back to the underlying {@link ChangeableViewSource}. */ @@ -20,12 +16,11 @@ public interface CommittableView extends View { * commit, consider using {@link #commitChangesAndUpdate()} instead to perform * an update of the view afterwards. * - * @return the changes resulting from propagating the recorded changes * @throws IllegalStateException if called on a closed view * @see #isClosed() * @see #commitChangesAndUpdate() */ - List commitChanges(); + void commitChanges(); /** * Convenience method for subsequent execution of {@link #commitChanges()} and @@ -34,15 +29,13 @@ public interface CommittableView extends View { * elements from the {@link ChangeableViewSource} afterwards to reflect * potential further changes made during commit. * - * @return the changes resulting from propagating the recorded changes * @throws IllegalStateException if called on a closed view * @see #commitChanges() * @see #update() * @see #isClosed() */ - default List commitChangesAndUpdate() { - List changes = commitChanges(); + default void commitChangesAndUpdate() { + commitChanges(); update(); - return changes; } } diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeDerivingView.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeDerivingView.xtend index 9fe06b01c7..cd49ae4d48 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeDerivingView.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeDerivingView.xtend @@ -8,6 +8,7 @@ import org.eclipse.emf.ecore.resource.ResourceSet import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl import org.eclipse.xtend.lib.annotations.Delegate import tools.vitruv.change.composite.description.VitruviusChange +import tools.vitruv.change.composite.description.VitruviusChangeFactory import tools.vitruv.framework.views.CommittableView import tools.vitruv.framework.views.View import tools.vitruv.framework.views.changederivation.StateBasedChangeResolutionStrategy @@ -55,17 +56,16 @@ class ChangeDerivingView implements ModifiableView, CommittableView { override commitChanges() { view.checkNotClosed() - val propagatedChanges = new ArrayList() + val changes = new ArrayList() val allResources = new HashSet(originalStateResourceMapping.keySet) allResources.addAll(view.viewResourceSet.resources) // consider newly added resources for (changedResource : allResources.filter[!URI.isPathmap]) { val change = generateChange(changedResource, originalStateResourceMapping.get(changedResource)) - if (change.containsConcreteChange) { - propagatedChanges += viewSource.propagateChange(change) - } + changes += change } + val change = VitruviusChangeFactory.instance.createCompositeChange(changes) + view.viewType.commitViewChanges(this, change) view.viewChanged = false - return propagatedChanges } override close() throws Exception { diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeRecordingView.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeRecordingView.xtend index ae45027945..1727f3a887 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeRecordingView.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ChangeRecordingView.xtend @@ -40,10 +40,9 @@ class ChangeRecordingView implements ModifiableView, CommittableView { override commitChanges() { view.checkNotClosed() changeRecorder.endRecording() - val propagatedChanges = viewSource.propagateChange(changeRecorder.change) + view.viewType.commitViewChanges(this, changeRecorder.change) view.viewChanged = false changeRecorder.beginRecording() - return propagatedChanges } override close() throws Exception { diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend index b7ea775b06..f7457e6331 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend @@ -1,6 +1,8 @@ package tools.vitruv.framework.views.impl +import tools.vitruv.change.composite.description.VitruviusChange import tools.vitruv.framework.views.ChangeableViewSource +import tools.vitruv.framework.views.CommittableView import tools.vitruv.framework.views.View import tools.vitruv.framework.views.ViewSource import tools.vitruv.framework.views.selectors.DirectViewElementSelector @@ -46,4 +48,9 @@ class IdentityMappingViewType extends AbstractViewType commitViewChanges(V view, VitruviusChange viewChange) { + view.viewSource.propagateChange(viewChange) + } + } diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java index 4ee5eec2e8..6f54f5e270 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java @@ -1,7 +1,10 @@ package tools.vitruv.framework.views.impl; -import tools.vitruv.framework.views.ViewType; +import tools.vitruv.change.composite.description.VitruviusChange; +import tools.vitruv.framework.views.ChangeableViewSource; +import tools.vitruv.framework.views.CommittableView; import tools.vitruv.framework.views.ViewSelector; +import tools.vitruv.framework.views.ViewType; /** * A specific view type that is able to create and update views. This is not its @@ -27,4 +30,16 @@ public interface ViewCreatingViewType extends ViewType void commitViewChanges(V view, VitruviusChange viewChange); } diff --git a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java index 369bef4f4b..76626be254 100644 --- a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java +++ b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeDerivingViewTest.java @@ -6,7 +6,9 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static tools.vitruv.testutils.matchers.ModelMatchers.equalsDeeply; @@ -27,12 +29,12 @@ import allElementTypes.NonRoot; import allElementTypes.Root; -import tools.vitruv.change.composite.description.VitruviusChange; import tools.vitruv.change.atomic.eobject.EobjectPackage; import tools.vitruv.change.atomic.feature.attribute.ReplaceSingleValuedEAttribute; import tools.vitruv.change.atomic.root.InsertRootEObject; import tools.vitruv.change.atomic.root.RootFactory; import tools.vitruv.change.atomic.root.RootPackage; +import tools.vitruv.change.composite.description.VitruviusChange; import tools.vitruv.framework.views.ChangeableViewSource; import tools.vitruv.framework.views.ModifiableViewSelection; import tools.vitruv.framework.views.changederivation.DefaultStateBasedChangeResolutionStrategy; @@ -203,10 +205,12 @@ public void commitChanges() throws Exception { String testResourceUriString = "test://test.aet"; view.registerRoot(root, URI.createURI(testResourceUriString)); assertThat(view.getRootObjects(), hasItem(root)); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeDerivingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - verify(mockChangeableViewSource).propagateChange(argument.capture()); - VitruviusChange change = argument.getValue().resolveAndApply(root.eResource().getResourceSet()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + VitruviusChange change = changeArgument.getValue().resolveAndApply(root.eResource().getResourceSet()); InsertRootEObject expectedChange = RootFactory.eINSTANCE.createInsertRootEObject(); expectedChange.setNewValue(root); expectedChange.setUri(testResourceUriString); @@ -276,12 +280,15 @@ public void commitChanges() throws Exception { String movedResourceUriString = "test://test2.aet"; view.registerRoot(root, URI.createURI("test://test.aet")); view.commitChanges(); - reset(mockChangeableViewSource); + reset(mockChangeableViewSource, mockViewType); view.moveRoot(root, URI.createURI(movedResourceUriString)); assertThat(view.getRootObjects(), hasItem(root)); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeDerivingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChangesAndUpdate(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + assertTrue(changeArgument.getValue().containsConcreteChange(), "change must contain some concrete change"); assertThat(view.getRootObjects().size(), is(1)); Root updatedRoot = (Root)view.getRootObjects().iterator().next(); assertThat(updatedRoot.eResource().getURI(), is(URI.createURI(movedResourceUriString))); @@ -301,8 +308,8 @@ public void prepareViewWithRootElement() { mockViewSelection), new DefaultStateBasedChangeResolutionStrategy()); root = aet.Root(); view.registerRoot(root, URI.createURI("test://test.aet")); - view.commitChanges(); - reset(mockChangeableViewSource); + view.commitChangesAndUpdate(); + reset(mockChangeableViewSource, mockViewType); } @AfterEach @@ -316,9 +323,12 @@ public void once() { NonRoot nonRoot = aet.NonRoot(); nonRoot.setId("nonRoot"); root.setSingleValuedContainmentEReference(nonRoot); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeDerivingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChangesAndUpdate(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + assertTrue(changeArgument.getValue().containsConcreteChange(), "change must contain some concrete change"); assertThat(view.getRootObjects().size(), is(1)); Root root = (Root)view.getRootObjects().iterator().next(); assertThat(root.eContents().size(), is(1)); @@ -332,13 +342,16 @@ public void twice() { firstNonRoot.setId("first"); root.setSingleValuedContainmentEReference(firstNonRoot); view.commitChanges(); - reset(mockChangeableViewSource); + reset(mockChangeableViewSource, mockViewType); NonRoot secondNonRoot = aet.NonRoot(); secondNonRoot.setId("second"); root.setSingleValuedContainmentEReference(secondNonRoot); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeDerivingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChangesAndUpdate(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + assertTrue(changeArgument.getValue().containsConcreteChange(), "change must contain some concrete change"); assertThat(view.getRootObjects().size(), is(1)); Root root = (Root)view.getRootObjects().iterator().next(); assertThat(root.eContents().size(), is(1)); @@ -348,9 +361,12 @@ public void twice() { @Test @DisplayName("without changes") public void withoutChanges() { + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeDerivingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChangesAndUpdate(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + assertFalse(changeArgument.getValue().containsConcreteChange(), "change must be empty"); assertThat(view.getRootObjects().size(), is(1)); } } diff --git a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeRecordingViewTest.java b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeRecordingViewTest.java index 673d7bcfd8..01bb50a503 100644 --- a/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeRecordingViewTest.java +++ b/tests/tools.vitruv.framework.views.tests/src/tools/vitruv/framework/views/impl/ChangeRecordingViewTest.java @@ -196,18 +196,21 @@ public void commitChanges() throws Exception { String testResourceUriString = "test://test.aet"; view.registerRoot(root, URI.createURI(testResourceUriString)); assertThat(view.getRootObjects(), hasItem(root)); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeRecordingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - verify(mockChangeableViewSource).propagateChange(argument.capture()); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + VitruviusChange recordedChange = changeArgument.getValue(); InsertRootEObject expectedChange = RootFactory.eINSTANCE.createInsertRootEObject(); expectedChange.setNewValue(root); expectedChange.setUri(testResourceUriString); - assertThat(argument.getValue().getEChanges().size(), is(3)); // Create, Insert, ReplaceId - assertThat(argument.getValue().getEChanges().get(1), + assertThat(recordedChange.getEChanges().size(), is(3)); // Create, Insert, ReplaceId + assertThat(recordedChange.getEChanges().get(1), equalsDeeply(expectedChange, ignoringFeatures(EobjectPackage.eINSTANCE.getEObjectAddedEChange_NewValueID(), RootPackage.eINSTANCE.getRootEChange_Resource()))); - assertThat(argument.getValue().getEChanges().get(2), instanceOf(ReplaceSingleValuedEAttribute.class)); + assertThat(recordedChange.getEChanges().get(2), instanceOf(ReplaceSingleValuedEAttribute.class)); } } } @@ -263,13 +266,15 @@ public void commitChanges() throws Exception { String movedResourceUriString = "test://test2.aet"; view.registerRoot(root, URI.createURI("test://test.aet")); view.commitChanges(); - reset(mockChangeableViewSource); + reset(mockChangeableViewSource, mockViewType); view.moveRoot(root, URI.createURI(movedResourceUriString)); assertThat(view.getRootObjects(), hasItem(root)); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeRecordingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); - List capturedEChanges = argument.getValue().getEChanges(); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + List capturedEChanges = changeArgument.getValue().getEChanges(); InsertRootEObject expectedChange = RootFactory.eINSTANCE.createInsertRootEObject(); expectedChange.setNewValue(root); expectedChange.setUri(movedResourceUriString); @@ -294,7 +299,7 @@ public void prepareViewWithRootElement() { root = aet.Root(); view.registerRoot(root, URI.createURI("test://test.aet")); view.commitChanges(); - reset(mockChangeableViewSource); + reset(mockChangeableViewSource, mockViewType); } @AfterEach @@ -308,10 +313,12 @@ public void once() { NonRoot nonRoot = aet.NonRoot(); nonRoot.setId("nonRoot"); root.setSingleValuedContainmentEReference(nonRoot); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeRecordingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); - List capturedEChanges = argument.getValue().getEChanges(); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + List capturedEChanges = changeArgument.getValue().getEChanges(); assertThat(capturedEChanges.size(), is(3)); // Create, Insert, ReplaceId assertThat(capturedEChanges.get(0), instanceOf(CreateEObject.class)); assertThat(capturedEChanges.get(1), instanceOf(ReplaceSingleValuedEReference.class)); @@ -325,14 +332,15 @@ public void twice() { firstNonRoot.setId("first"); root.setSingleValuedContainmentEReference(firstNonRoot); view.commitChanges(); - reset(mockChangeableViewSource); + reset(mockChangeableViewSource, mockViewType); NonRoot secondNonRoot = aet.NonRoot(); secondNonRoot.setId("second"); root.setSingleValuedContainmentEReference(secondNonRoot); + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeRecordingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); - List capturedEChanges = argument.getValue().getEChanges(); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + List capturedEChanges = changeArgument.getValue().getEChanges(); assertThat(capturedEChanges.size(), is(4)); // Create, Insert, ReplaceValue, Delete assertThat(capturedEChanges.get(0), instanceOf(CreateEObject.class)); assertThat(capturedEChanges.get(1), instanceOf(ReplaceSingleValuedEReference.class)); @@ -351,10 +359,12 @@ public void twice() { @Test @DisplayName("without changes") public void withoutChanges() { + ArgumentCaptor viewArgument = ArgumentCaptor.forClass(ChangeRecordingView.class); + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); view.commitChanges(); - ArgumentCaptor argument = ArgumentCaptor.forClass(VitruviusChange.class); - verify(mockChangeableViewSource).propagateChange(argument.capture()); - assertThat(argument.getValue().getEChanges(), not(hasItem(anything()))); + verify(mockViewType).commitViewChanges(viewArgument.capture(), changeArgument.capture()); + assertThat(viewArgument.getValue(), is(view)); + assertThat(changeArgument.getValue().getEChanges(), not(hasItem(anything()))); } } diff --git a/tests/tools.vitruv.framework.vsum.tests/src/tools/vitruv/framework/vsum/VirtualModelTest.xtend b/tests/tools.vitruv.framework.vsum.tests/src/tools/vitruv/framework/vsum/VirtualModelTest.xtend index 4fd10d6894..7182413b53 100644 --- a/tests/tools.vitruv.framework.vsum.tests/src/tools/vitruv/framework/vsum/VirtualModelTest.xtend +++ b/tests/tools.vitruv.framework.vsum.tests/src/tools/vitruv/framework/vsum/VirtualModelTest.xtend @@ -337,8 +337,7 @@ class VirtualModelTest { assertThat("view must have been modified", testView.isModified) // Commit changes and assert VSUM was updated correctly - val changes = testView.commitChanges() - assertThat(new HashSet(changes), not(emptySet())) + testView.commitChanges() assertThat("view source must have been changed", testView.outdated) assertThat("view must not have been modified", !testView.isModified) From dc3ac25f02cce15eeb7a65be51f4386d1d440418 Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Fri, 30 Dec 2022 16:46:24 +0100 Subject: [PATCH 3/5] refine view type interface --- .../vitruv/framework/views/impl/IdentityMappingViewType.xtend | 3 +-- .../vitruv/framework/views/impl/ViewCreatingViewType.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend index f7457e6331..b6f31dbca1 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend @@ -2,7 +2,6 @@ package tools.vitruv.framework.views.impl import tools.vitruv.change.composite.description.VitruviusChange import tools.vitruv.framework.views.ChangeableViewSource -import tools.vitruv.framework.views.CommittableView import tools.vitruv.framework.views.View import tools.vitruv.framework.views.ViewSource import tools.vitruv.framework.views.selectors.DirectViewElementSelector @@ -49,7 +48,7 @@ class IdentityMappingViewType extends AbstractViewType commitViewChanges(V view, VitruviusChange viewChange) { + override commitViewChanges(ModifiableView view, VitruviusChange viewChange) { view.viewSource.propagateChange(viewChange) } diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java index 6f54f5e270..e4dcbd2d8a 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java @@ -2,7 +2,6 @@ import tools.vitruv.change.composite.description.VitruviusChange; import tools.vitruv.framework.views.ChangeableViewSource; -import tools.vitruv.framework.views.CommittableView; import tools.vitruv.framework.views.ViewSelector; import tools.vitruv.framework.views.ViewType; @@ -41,5 +40,5 @@ public interface ViewCreatingViewType extends ViewType void commitViewChanges(V view, VitruviusChange viewChange); + void commitViewChanges(ModifiableView view, VitruviusChange viewChange); } From 552dcf3f3f56b5c8b008b244aa6439e73aa2558f Mon Sep 17 00:00:00 2001 From: Jan Wittler Date: Mon, 9 Jan 2023 09:49:32 +0100 Subject: [PATCH 4/5] improve documentation --- .../vitruv/framework/views/impl/ViewCreatingViewType.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java index e4dcbd2d8a..15604c0043 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/ViewCreatingViewType.java @@ -35,7 +35,8 @@ public interface ViewCreatingViewType extends ViewType Date: Wed, 11 Jan 2023 13:03:03 +0100 Subject: [PATCH 5/5] implement tests for IdentityMappingViewType#commitViewChanges --- .../views/impl/IdentityMappingViewType.xtend | 3 + .../impl/IdentityMappingViewTypeTest.java | 89 +++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend index b6f31dbca1..cdc675c22d 100644 --- a/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend +++ b/bundles/tools.vitruv.framework.views/src/tools/vitruv/framework/views/impl/IdentityMappingViewType.xtend @@ -8,6 +8,7 @@ import tools.vitruv.framework.views.selectors.DirectViewElementSelector import tools.vitruv.framework.views.util.ResourceCopier import static com.google.common.base.Preconditions.checkArgument +import static com.google.common.base.Preconditions.checkNotNull import static extension tools.vitruv.framework.views.util.ResourceCopier.requiresFullCopy @@ -49,6 +50,8 @@ class IdentityMappingViewType extends AbstractViewType basicViewType.commitViewChanges(view, null)); + } + + @Test + @DisplayName("with null view") + void withNullView() { + VitruviusChange someChange = VitruviusChangeFactory.getInstance().createTransactionalChange(Set.of()); + assertThrows(NullPointerException.class, () -> basicViewType.commitViewChanges(null, someChange)); + } + + @ParameterizedTest + @MethodSource("testEmptyChanges") + @DisplayName("with empty changes") + void withEmptyChanges(VitruviusChange change) { + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); + basicViewType.commitViewChanges(view, change); + verify(viewSource).propagateChange(changeArgument.capture()); + assertEquals(changeArgument.getValue(), change); + } + + private static Stream testEmptyChanges() { + VitruviusChangeFactory factory = VitruviusChangeFactory.getInstance(); + return Stream.of( + factory.createTransactionalChange(Set.of()), + factory.createCompositeChange(Set.of()) + ); + } + + @Test + @DisplayName("with non-empty change") + void withNonEmptyChange() { + Root root = createResourceWithSingleRoot(URI.createURI("test://test.aet")); + try (ChangeRecorder changeRecorder = new ChangeRecorder(testResourceSet)) { + changeRecorder.addToRecording(root); + changeRecorder.beginRecording(); + root.setId("testid2"); + changeRecorder.endRecording(); + VitruviusChange change = changeRecorder.getChange().unresolve(); + + ArgumentCaptor changeArgument = ArgumentCaptor.forClass(VitruviusChange.class); + basicViewType.commitViewChanges(view, change); + verify(viewSource).propagateChange(changeArgument.capture()); + List eChanges = changeArgument.getValue().unresolve().getEChanges(); + assertThat(eChanges.size(), is(1)); + assertThat(eChanges.get(0), equalsDeeply(change.getEChanges().get(0))); + } + } + } }