Refactor PackageDependencies property #9803
Conversation
|
so this does not address the underlying issue right? (as you said above) - the point brought up by @ColinDayOrg - that likely which package is loaded depends on load order. IE if you restart Dynamo you might load packages in a different order than if install them manually in a single session. |
|
@mjkkirschner Right, this PR just allows package dependency collection to deal with cases where multiple packages, or versions of packages, are able to resolve nodes. The underlying issue will be addressed later, as the outcome of @aparajit-pratap 's spike into the issue. |
|
@scottmitchell this looks good to me - can you also cherry pick to 2.3 branch. |
| bool matchFound = false; | ||
| // Check if the target package is installed and loaded. | ||
| foreach (var loadedPackage in ws.LoadedPackageDependencies) | ||
| if (package.IsLoaded) |
There was a problem hiding this comment.
Much easier to refresh UI now, great. The only question is that how costly to maintain this property on each node add operation, would love to know how slow it is
|
LGTM |
|
@scottmitchell I have a DYN file with package dependency as clockwork 1.x and the same package installed. However when I load the file, I still see package dependency 1.x to be missing (in red). I tried the same thing with 2.x; existing DYN file with package dependency serialized as 2.x, and 2.x locally installed. Same issue; 2.x is still missing in view extension. Could you please test manually at your end to verify. |
|
@scottmitchell since we all missed that bug except @aparajit-pratap 🦅 - can we add a test for it? |
* Refactor PackageDependencies property * Add test to verify multiple packages not listed as dependencies for one node * Keep original package dependency by default and add API to void original package dependency * Add comment * Move helper methods out of properties region * Fix IsLoaded logic * Update test .dyn to avoid serialization test failures * Exclude new dyn from serialization tests * Use preexisting method to exclude test dyn from serialization tests
* Refactor PackageDependencies property * Add test to verify multiple packages not listed as dependencies for one node * Keep original package dependency by default and add API to void original package dependency * Add comment * Move helper methods out of properties region * Fix IsLoaded logic * Update test .dyn to avoid serialization test failures * Exclude new dyn from serialization tests * Use preexisting method to exclude test dyn from serialization tests
* Refactor PackageDependencies property * Add test to verify multiple packages not listed as dependencies for one node * Keep original package dependency by default and add API to void original package dependency * Add comment * Move helper methods out of properties region * Fix IsLoaded logic * Update test .dyn to avoid serialization test failures * Exclude new dyn from serialization tests * Use preexisting method to exclude test dyn from serialization tests
Purpose
This PR addresses https://jira.autodesk.com/browse/DYN-1997 and also simplifies the package dependency collection process.
This PR removes
LoadedPackageDependenciesproperty and refactorsPackageDependenciesproperty/collection andPackageDependencyInfoobject.PackageInfotype.IsLoadedproperty toPackageDependencyInfoThis should fix the case found by @QilongTang where a node can have two package dependencies when multiple packages are loaded with the same custom node. Previously, both the deserialized package dependency and the collected package dependency of a single node could be contained in the
PackageDependenciesproperty of a workspace. Now, the original package dependency is maintained until it is voided usingVoidNodeDependency(). After the original dependency has been voided, the local package dependency will be used for serialization.Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @aparajit-pratap @QilongTang @ColinDayOrg