Skip to content
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

[AGD-1799] Support persisting extension and view extension data #11347

Conversation

SHKnudsen
Copy link
Contributor

Purpose

This PR adds a new ExtensionStorage interface which can be implemented on Extensions and ViewExtension to give the author of the extension the possibility to store extension data directly in the DYN file.

The interface adds two new methods OnWorkspaceOpen and OnWorkspaceSaving which are triggered when a graph is opened and when its saved. Both methods passes a Dictionary<string,string> which can be used to store extension data in, OnWorkspaceSaving also passes a SaveContext so the implementer can react to different save contexts (i.e. save, save as).

The WorkspaceModel stores a collection of ExtensionData objects which is serialized to the DYN file on save. The OnWorkspaceOpen passes a copy of the extension data dictionary therefor modifications done to this will not be reflected in the ExtensionData object (FYI @saintentropy). OnWorkspaceSaving passes a direct reference to the ExtensionData objects dictionary and there for modification made to this will be reflected in the DYN file.

TODO

  • Determine the max size of the dictionary stored in the DYN file
  • Add extension version to the ExtensionData model

image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@saintentropy
@nate-peters

FYIs

@mjkkirschner
@QilongTang

@@ -33,6 +33,7 @@ public interface IExtensionSource
public class ExtensionManager: IExtensionManager, ILogSource
{
private readonly List<IExtension> extensions = new List<IExtension>();
private readonly List<IExtensionStorageAccess> storageAccessExtensions = new List<IExtensionStorageAccess>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use a hashset or dictionary of guid to extensions?

@SHKnudsen SHKnudsen changed the title Support persisting extension and view extension data [AGD-1799] Support persisting extension and view extension data Dec 9, 2020
@mjkkirschner
Copy link
Member

please add tests to your TODO list.

@SHKnudsen SHKnudsen marked this pull request as ready for review January 4, 2021 10:07
@SHKnudsen
Copy link
Contributor Author

@saintentropy @mjkkirschner moved this out of draft state. Let me know if there are any other comments that needs to be addressed.

@mjkkirschner
Copy link
Member

@SHKnudsen there are around ~80 test failures in the last build - I'll kick it off again incase it's a fluke, but you may want to take a look at the PR test validation link above.

@SHKnudsen
Copy link
Contributor Author

@mjkkirschner Yea looking at it now, i get the same locally. It looks like it might be something with the migration of some old test dyn files, which feels a bit weird.

@SHKnudsen
Copy link
Contributor Author

Turned out it was just me that had messed a few if statements up, should be fixed now.

@mjkkirschner
Copy link
Member

mjkkirschner commented Jan 5, 2021

@SHKnudsen only one failure left 👍

DynamoCoreWpfTests.SerializationTests.JSONisSameBeforeAndAfterSaveWithDummyNodes

I'm guessing maybe this test needs to ignore the new data you added?

@SHKnudsen
Copy link
Contributor Author

@mjkkirschner you where right, the test was failing because we are now adding a ExtensionWorkspaceData property on save. Im ignoring that property when comparing the two json files in the test, so that should be fixed now.

@mjkkirschner
Copy link
Member

@nate-peters @saintentropy - any other comments?

@mjkkirschner
Copy link
Member

Is there scope to add a sample extension to the dynamo samples repo illustrating use of this interface?

@SHKnudsen
Copy link
Contributor Author

Is there scope to add a sample extension to the dynamo samples repo illustrating use of this interface?

I think i can find some scope to do this, if you guys are happy with that.

@saintentropy

src/DynamoCore/Extensions/ExtensionData.cs Show resolved Hide resolved
src/DynamoCore/Extensions/ExtensionManager.cs Outdated Show resolved Hide resolved
src/DynamoCore/Graph/ModelBase.cs Show resolved Hide resolved
src/DynamoCore/Graph/Workspaces/SerializationConverters.cs Outdated Show resolved Hide resolved
src/DynamoCore/Graph/Workspaces/SerializationConverters.cs Outdated Show resolved Hide resolved
src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved
src/DynamoCore/Models/DynamoModel.cs Outdated Show resolved Hide resolved
src/DynamoCoreWpf/Extensions/ViewExtensionManager.cs Outdated Show resolved Hide resolved
@mjkkirschner mjkkirschner merged commit fffcb0d into DynamoDS:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants