-
Notifications
You must be signed in to change notification settings - Fork 634
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-1882] Support storing user defined graph documentation #11444
[AGD-1882] Support storing user defined graph documentation #11444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @SHKnudsen
@SHKnudsen the tests ran so long ago that the console logs are now gone - I've started a new build here: |
only one test failure:
|
Thanks @mjkkirschner, So this fails because its trying to compare a dyn file before getting opened and after getting saved. The old file will not have these new properties we are adding in this PR, but when its saved they will be appended, resulting in the files now being different. I can see that we previously ignored a new property (the This is the file the test is using https://github.com/DynamoDS/Dynamo/blob/master/test/core/dummy_node/2080_JSONTESTCRASH%20undo_redo.dyn |
@SHKnudsen either is fine with me - but guess it's better to update the test file if it's not too onerous? |
@@ -203,6 +203,8 @@ internal int CurrentPasteOffset | |||
private DateTime lastSaved; | |||
private string author = "None provided"; | |||
private string description; | |||
private string thumbnail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before this gets merged- a quick thought, do all of these new properties make sense on both home workspaces and custom node workspaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I hadn't considered that this also affected the CN workspace. Most of these probably don't make sense on the CN, maybe the Author
is okay, but not sure i see the need for the others.
@nate-peters @saintentropy, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just noticed that its the same with the new ExtensionWorkspaceData
not sure that makes sense in the CN workspace either?
Purpose
This pull request addresses JIRA task AGD-1882.
Example of
.dyn
file storing a Base64 thumbnail:This pull request does
Thumbnail
property to the workspace model, which stores a Base64 encoded string.Declarations
Check these if you believe they are true
*.resx
filesReviewers
@saintentropy
@nate-peters
FYIs
@mjkkirschner