DYN-4034 : open file from json content #12671

BogdanZavu · 2022-03-04T15:10:36Z

Purpose

Allow a workspace to be created from json content.
In Generative Design, we often work with multiple variants of the same graph for which currently we save a file on disk.
We want to avoid this as it is more efficient t work with the file content directly.

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

@mjkkirschner @saintentropy

FYIs

src/DynamoCore/Scheduler/DynamoSchedulerInternals.cs

mjkkirschner · 2022-03-04T16:35:13Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1870,7 +1885,7 @@ private void SetPeriodicEvaluation(WorkspaceModel ws)
                CustomNodeManager,
                this.LinterManager);

-            workspace.FileName = filePath;
+            workspace.FileName = String.IsNullOrEmpty(filePath) ? "" : filePath;


what affect does this have? Isn't workspace.FileName going to end up being null "" or filepath in all cases?

I just wanted to avoid having workspace.FileName set to null and set it to empty string instead so that I can mimic the exact FIleName setting of a new Home unsaved dyn file.

mjkkirschner · 2022-03-04T16:37:00Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1852,7 +1864,10 @@ private void SetPeriodicEvaluation(WorkspaceModel ws)
          bool forceManualExecutionMode,
          out WorkspaceModel workspace)
        {
-            CustomNodeManager.AddUninitializedCustomNodesInPath(Path.GetDirectoryName(filePath), IsTestMode);
+            if (!String.IsNullOrEmpty(filePath))


I guess this is good to do, though unnecessary as ScanNodeHeadersInDirectory already checks the directory exists and logs something.

Ok, will remove

mjkkirschner · 2022-03-04T16:38:16Z

Looks good @BogdanZavu -

just one a few small comments,
and some code that I think is from another PR.
Can we also add a test for the method - that loads some JSON from a string, and asserts the graph is loaded correctly?

saintentropy · 2022-03-07T19:17:29Z

LGTM @BogdanZavu with test from Mike's comment

BogdanZavu · 2022-03-09T15:08:32Z

src/DynamoCore/Models/DynamoModel.cs

@@ -1852,7 +1864,10 @@ private void SetPeriodicEvaluation(WorkspaceModel ws)
          bool forceManualExecutionMode,
          out WorkspaceModel workspace)
        {
-            CustomNodeManager.AddUninitializedCustomNodesInPath(Path.GetDirectoryName(filePath), IsTestMode);
+            if (!string.IsNullOrEmpty(filePath))


I added this check back because Path.GetDirectoryName throws when filePath is null.

BogdanZavu · 2022-03-09T15:09:56Z

src/DynamoCore/Models/RecordableCommands.cs

+            {
+                // Log file open action and the number of nodes in the opened workspace
+                Dynamo.Logging.Analytics.TrackFileOperationEvent(
+                    dynamoModel.CurrentWorkspace.Name,


Should we use something else here since Name will be an empty string ?

does this match the implementation/result for disk based file open?

ah, I see, the other uses filepath - maybe something that notes this is in memory open.

BogdanZavu · 2022-03-09T15:12:54Z

@mjkkirschner @saintentropy I added a command as well to be more inline with the regular OpenFileCommand and to benefit from the AskForSave utility on the Dynamo side. Please take another look

mjkkirschner · 2022-03-10T21:24:59Z

src/DynamoCore/Models/RecordableCommands.cs

@@ -144,6 +144,9 @@ internal static RecordableCommand Deserialize(XmlElement element)
                    case "OpenFileCommand":
                        command = OpenFileCommand.DeserializeCore(element);
                        break;
+                    case "OpenFileFromJsonCommand":


can you use nameof?

mjkkirschner · 2022-03-10T21:29:00Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

+                        errorMsgString = String.Format(Resources.MessageUnkownErrorOpeningFile, "Json file content");
+                    }
+                    model.Logger.LogNotification("Dynamo", commandString, errorMsgString, e.ToString());
+                    System.Windows.MessageBox.Show(errorMsgString);


I think we want to use the new themed dialog box where possible - @QilongTang @RobertGlobant20 can you point to an example of this?

Refer to this PR: https://github.com/DynamoDS/Dynamo/pull/12655/files

mjkkirschner · 2022-03-10T21:30:26Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

@@ -1541,17 +1633,42 @@ private bool CanOpen(object parameters)
            return PathHelper.IsValidPath(filePath);
        }

+        private bool CanOpenFromJson(object parameters)


what about reusing this helper?

Dynamo/src/DynamoUtilities/PathHelper.cs

Line 136 in ad0dcca

public static bool isValidJson(string path, out string fileContents, out Exception ex)

Please reuse the helper, I put a lot of thoughts into it.. Feel free to update as well

mjkkirschner · 2022-03-10T21:30:45Z

src/DynamoUtilities/DebugModes.cs

@@ -52,6 +52,8 @@ private static void RegisterDebugModes()

        internal static void LoadDebugModesStatusFromConfig(string configPath)
        {
+            if (!File.Exists(configPath)) return;


open file from json content

872f8ef

mjkkirschner reviewed Mar 4, 2022

View reviewed changes

src/DynamoCore/Scheduler/DynamoSchedulerInternals.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Mar 4, 2022

View reviewed changes

adress feedback 1

0d4a97f

openfilefromjson command and test

ba37305

BogdanZavu commented Mar 9, 2022

View reviewed changes

mjkkirschner reviewed Mar 10, 2022

View reviewed changes

adress feedback

9976ac7

mjkkirschner approved these changes Mar 11, 2022

View reviewed changes

Merge branch 'master' into DYN-4034

4bc82e6

BogdanZavu merged commit d5bc36b into DynamoDS:master Mar 11, 2022

BogdanZavu deleted the DYN-4034 branch March 11, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-4034 : open file from json content #12671

DYN-4034 : open file from json content #12671

BogdanZavu commented Mar 4, 2022 •

edited

Loading

mjkkirschner Mar 4, 2022

BogdanZavu Mar 4, 2022

mjkkirschner Mar 4, 2022

BogdanZavu Mar 4, 2022

mjkkirschner commented Mar 4, 2022

saintentropy commented Mar 7, 2022

BogdanZavu Mar 9, 2022

BogdanZavu Mar 9, 2022

mjkkirschner Mar 10, 2022

mjkkirschner Mar 10, 2022

BogdanZavu commented Mar 9, 2022

mjkkirschner Mar 10, 2022

mjkkirschner Mar 10, 2022

QilongTang Mar 10, 2022

mjkkirschner Mar 10, 2022

QilongTang Mar 10, 2022

BogdanZavu Mar 11, 2022

mjkkirschner Mar 10, 2022

DYN-4034 : open file from json content #12671

DYN-4034 : open file from json content #12671

Conversation

BogdanZavu commented Mar 4, 2022 • edited Loading

Purpose

Declarations

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Mar 4, 2022

saintentropy commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BogdanZavu commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BogdanZavu commented Mar 4, 2022 •

edited

Loading