-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug when creating from template with multiple programs #200
Comments
Here is a patch for this before needing to work on something else. Subject: [PATCH] focusPanDirection -> limitPanDirection, see https://github.com/phetsims/center-and-variability/issues/564
---
Index: client/creator/model/CreatorModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/client/creator/model/CreatorModel.js b/client/creator/model/CreatorModel.js
--- a/client/creator/model/CreatorModel.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f)
+++ b/client/creator/model/CreatorModel.js (date 1700066512121)
@@ -221,26 +221,58 @@
copyProgram( program ) {
assert && assert( this.programs.includes( program ), 'program is not in this list of programs' );
- const newPosition = program.positionProperty.value.plusXY( 20, 20 );
- const newProgram = this.createProgram( newPosition );
-
- const programJSON = program.save();
- newProgram.copyFromOther( programJSON, this.getUniqueCopyName.bind( this ), this.allModelComponents );
- return newProgram;
+ const copyJSON = { programs: [ program.save() ] };
+ this.copyProgramsFromJSON( copyJSON );
}
/**
* Create a copy of a program from the provided JSON state. The new program is given a random number. The program and
* component names are copied but will include a suffix or make them unique.
- * @param programJSON
+ * @param json
* @return {ProgramModel}
*/
- copyProgramFromJSON( programJSON ) {
- const newPosition = phet.dot.Vector2.fromStateObject( programJSON.positionProperty ).plusXY( 20, 20 );
- const newProgram = this.createProgram( newPosition );
+ copyProgramsFromJSON( json ) {
+ if ( json.programs ) {
+ debugger;
+
+ const newModelNames = {};
+ json.programs.forEach( programJSON => {
+
+ // first create all programs and load dependency model components
+ const newPosition = phet.dot.Vector2.fromStateObject( programJSON.positionProperty ).plusXY( 20, 20 );
+ const newProgram = this.createProgram( newPosition );
- newProgram.copyFromOther( programJSON, this.getUniqueCopyName.bind( this ), this.allModelComponents );
- return newProgram;
+ newProgram.copyMetadataFromOther( programJSON );
+
+ // This will update the programJSON and newModelNames with new names for copied components.
+ newProgram.copyDependencyComponentsFromOther( programJSON, this.getUniqueCopyName.bind( this ), newModelNames );
+ } );
+
+ // then load DerivedProperty components once dependencies are in place
+ json.programs.forEach( programJSON => {
+ debugger;
+ const program = this.programs.find( program => program.numberProperty.value === programJSON.number );
+
+ // We can use this directly because this JSON has had its names updated.
+ program.loadDependentModelComponents( programJSON, this.allModelComponents );
+ } );
+
+ // then load controller/view components once model components are in place
+ json.programs.forEach( programJSON => {
+ const program = this.programs.find( program => program.numberProperty.value === programJSON.number );
+
+ const getUniqueCopyName = this.getUniqueCopyName.bind( this );
+ program.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, this.allModelComponents, newModelNames );
+ program.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, this.allModelComponents, newModelNames );
+ program.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, this.allModelComponents, newModelNames );
+ } );
+
+ // then update custom code after all renames are complete
+ json.programs.forEach( programJSON => {
+ const program = this.programs.find( program => program.numberProperty.value === programJSON.number );
+ program.copyCustomCodeFromOther( programJSON, this.getUniqueCopyName.bind( this ), newModelNames );
+ } );
+ }
}
/**
@@ -330,9 +362,7 @@
try {
const templateJSON = JSON.parse( templateJSONString );
debugger;
- templateJSON.programs.forEach( programJSON => {
- this.copyProgramFromJSON( programJSON );
- } );
+ this.copyProgramsFromJSON( templateJSON );
}
catch( error ) {
this.errorOccurredEmitter.emit( error.message );
Index: client/creator/model/ProgramModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/client/creator/model/ProgramModel.js b/client/creator/model/ProgramModel.js
--- a/client/creator/model/ProgramModel.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f)
+++ b/client/creator/model/ProgramModel.js (date 1700064639261)
@@ -114,20 +114,30 @@
* @param {NamedProperty[]} allComponents - all model components in all programs
*/
copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents ) {
- const newModelNames = this.modelContainer.copyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, allComponents );
+ // const newModelNames = this.modelContainer.copyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, allComponents );
- this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames );
- this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames );
- this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames );
+ // this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames );
+ // this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames );
+ // this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames );
- return newModelNames;
+ // return newModelNames;
}
- copyFromOther( programJSON, getUniqueCopyName, allComponents ) {
- this.copyMetadataFromOther( programJSON );
- const newModelNames = this.copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents );
- this.copyCustomCodeFromOther( programJSON, newModelNames );
+ // copyFromOther( programJSON, getUniqueCopyName, allComponents ) {
+ // // this.copyMetadataFromOther( programJSON );
+ // // const newModelNames = this.copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents );
+ // // this.copyCustomCodeFromOther( programJSON, newModelNames );
+ // }
+
+ copyDependencyComponentsFromOther( programJSON, getUniqueCopyName, newModelNames ) {
+ return this.modelContainer.copyDependencyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, newModelNames );
}
+
+ // copyDependentComponentsFromOther( programJSON, getUniqueCopyName, allComponents, newModelNames ) {
+ // this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames );
+ // this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames );
+ // this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames );
+ // }
/**
* TODO: Remove any other connections with other programs.
Index: client/creator/model/ProgramModelContainer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/client/creator/model/ProgramModelContainer.js b/client/creator/model/ProgramModelContainer.js
--- a/client/creator/model/ProgramModelContainer.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f)
+++ b/client/creator/model/ProgramModelContainer.js (date 1700064997945)
@@ -202,9 +202,56 @@
*/
copyModelComponentsFromOther( otherContainerJSON, getUniqueCopyName, allComponents ) {
+ // // Need to map dependencies to the new copies so that we can determine whether the dependency should be
+ // // on a copied component or on the original.
+ // const nameChangeMap = {};
+ //
+ // // Update names of components and look for dependencies
+ // for ( const key in otherContainerJSON ) {
+ // const components = otherContainerJSON[ key ];
+ //
+ // components.forEach( component => {
+ // const originalName = component.name;
+ //
+ // // update the name to be unique
+ // component.name = getUniqueCopyName( originalName );
+ //
+ // // store the change so that we can appropriately update dependencies
+ // nameChangeMap[ originalName ] = component.name;
+ // } );
+ // }
+ //
+ // // Update dependency relationships and references in custom code. If a dependency was copied then use the new copy.
+ // for ( const key in otherContainerJSON ) {
+ // const componentObjects = otherContainerJSON[ key ];
+ //
+ // componentObjects.forEach( componentStateObject => {
+ // if ( componentStateObject.dependencyNames ) {
+ //
+ // // update the dependency to use the newly copied component if it exists
+ // componentStateObject.dependencyNames = componentStateObject.dependencyNames.map( dependencyName => {
+ // return nameChangeMap[ dependencyName ] || dependencyName;
+ // } );
+ //
+ // // update the derivation function to use the newly copied component if necessary
+ // for ( const name in nameChangeMap ) {
+ // const newName = nameChangeMap[ name ];
+ // componentStateObject.derivation = componentStateObject.derivation.replaceAll( name, newName );
+ // }
+ // }
+ // } );
+ // }
+
+ // this.loadDependencyModelComponents( otherContainerJSON );
+ // this.loadDependentModelComponents( otherContainerJSON, allComponents );
+
+ // return nameChangeMap;
+ }
+
+ copyDependencyModelComponentsFromOther( otherContainerJSON, getUniqueCopyName, nameChangeMap ) {
+
// Need to map dependencies to the new copies so that we can determine whether the dependency should be
// on a copied component or on the original.
- const nameChangeMap = {};
// Update names of components and look for dependencies
for ( const key in otherContainerJSON ) {
@@ -217,6 +264,9 @@
component.name = getUniqueCopyName( originalName );
// store the change so that we can appropriately update dependencies
+ if ( nameChangeMap[ originalName ] ) {
+ throw new Error( 'We have already stored a change for this name. If we do it again, references will be lost.' );
+ }
nameChangeMap[ originalName ] = component.name;
} );
}
@@ -243,7 +293,6 @@
}
this.loadDependencyModelComponents( otherContainerJSON );
- this.loadDependentModelComponents( otherContainerJSON, allComponents );
return nameChangeMap;
}
Index: client/creator/react/SpaceSelectControls.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/client/creator/react/SpaceSelectControls.js b/client/creator/react/SpaceSelectControls.js
--- a/client/creator/react/SpaceSelectControls.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f)
+++ b/client/creator/react/SpaceSelectControls.js (date 1700065144945)
@@ -63,7 +63,7 @@
setSelectedSpaceName( 'jg-tests' );
setTimeout( () => {
- setSelectedProjectName( 'projection-test' );
+ setSelectedProjectName( 'derived-test' );
}, 500 );
}, 500 );
}
|
This required a brand new implementation for copying programs and templates. Since we don't have unit tests Ill have to test as much as I can manually.
|
I was able to test ~half way through the above list and so far its looking good. I was able to successfully create 3 programs that use a Derived component from a template (template Described Grid), which was the original motivation for the issue. @brettfiedler offered to take on the rest, thanks so much! Ill commit the changes to the dev branch. |
…ng references on the serialized state before loading - dependencies and names are updated on ALL programs before setting up relationships, see #200
Able to create copies of programs with animation link as well as standard link components. Verified this worked for inter- and intra- program links.
|
Custom Code Copying
|
@jessegreenberg, I think all of copying and templating works for these! Is there anything else you wanted to see or more trials? |
Awesome, thanks so much for going through this! I read through #200 (comment) and agree with what you found. OK, closing! |
When creating multiple programs from a single template, the order of operations is wrong. We load each program sequentially, loading dependencies before dependent components. What we need to do is load all dependencies in all programs and then load all dependent components in all programs (like how we do for loading a project).
I noticed this when trying to load a template that used a NamedDerivedProperty with dependencies in a different program.
The text was updated successfully, but these errors were encountered: