Workspace and WorkspaceViewModel Serialization by ikeough · Pull Request #7953 · DynamoDS/Dynamo

ikeough · 2017-06-12T06:45:29Z

Purpose

This PR adds two different modes for Workspace serialization:

If serialization is requested from the UI, it goes through DynamoViewModel which in turn calls CurrentSpaceViewModel.Save. The WorkspaceViewModel serialization first calls WorkspaceModel.ToJson(...), then injects the View property whose value is the serialization of the WorkspaceViewModel into the resulting json. Then the json is saved to the file.
If serialization is requested using WorkspaceModel.Save then WorkspaceModel.ToJson is called, and the results are written directly to the file.

A number of API changes have been made and are called out in comments below.

Declarations

Check these if you believe they are true

The code base 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. (One existing regression remains)
Snapshot of UI changes, if any. n/a
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@QilongTang

…tension methods.

ikeough · 2017-06-12T07:10:25Z

        /// we should add it to recent files. Otherwise leave it.</param>
        /// <returns></returns>
-        public override bool Save(string newPath, bool isBackup = false)
+        public override void Save(string newPath, bool isBackup = false)


API Change. WorkspaceModel.Save now returns void.

ikeough · 2017-06-12T07:11:05Z

    /// by looking it up in the CustomNodeManager.
    /// </summary>
-    public class NodeModelConverter : JsonConverter
+    public class NodeReadConverter : JsonConverter


Potential API Change. Need to check if these converters were included in 1.3.

ikeough · 2017-06-12T07:11:32Z

-        public static string ToJson(this WorkspaceModel workspace, LibraryServices libraryServices,
-            EngineController engineController, DynamoScheduler scheduler, NodeFactory factory,
-            bool isTestMode, bool verboseLogging, CustomNodeManager manager)
+        public static string ToJson(this WorkspaceModel workspace)


API Change. The SerializationExtensions class has been added to provide the WorkspaceModel.ToJson(...) extension method.

ikeough · 2017-06-12T07:12:03Z

-        public event Action WorkspaceSaved;
-        protected virtual void OnWorkspaceSaved()
+        public event Action Saved;
+        internal virtual void OnSaved()


API change. The WorkspaceModel.WorkspaceSaved event is now WorkspaceModel.Saved.

ikeough · 2017-06-12T07:12:36Z

        /// we should add it to recent files. Otherwise leave it.</param>
-        public virtual bool Save(string newPath, bool isBackup = false)
+        /// <exception cref="ArgumentNullException">Thrown when the file path is null.</exception>
+        public virtual void Save(string filePath, bool isBackup = false)


API change. Save now returns void. An Exception is thrown if an error occurs.

ikeough · 2017-06-12T07:14:54Z

-        /// <param name="path">The path to save to</param>
-        /// <param name="ws">workspace to save</param>
-        /// <param name="isBackup">indicate saving for backup</param>
-        public bool SaveWorkspace(string path, WorkspaceModel ws, bool isBackup = false)


API change. DynamoModel.SaveWorkspace(...) has been removed. Use WorkspaceModel.Save(...) instead.

ikeough · 2017-06-12T07:17:54Z

+        /// </summary>
+        /// <param name="viewModel"></param>
+        /// <returns>A JSON string representing the WorkspaceViewModel</returns>
+        public static string ToJson(this WorkspaceViewModel viewModel)


API change. WorkspaceViewModel.ToJson(...) has been added.

QilongTang · 2017-06-12T14:35:20Z

        /// </summary>
-        /// <param name="newPath">The path to save to</param>
-        /// <param name="isBackup">Indicates whether saved workspace is backup or not. If it's not backup,
+        /// <param name="filePath">The path the file.</param>


The path of the file

QilongTang · 2017-06-12T14:37:26Z

+                    OnSaved();
+                }
+
+                OnSaved();


The OnSaved() in if seems redundant

Good catch. This is a mistake.

QilongTang · 2017-06-12T15:17:04Z

            string fn = "ruthlessTurtles.dyn";
            string path = Path.Combine(TempFolder, fn);
-            CurrentDynamoModel.SaveWorkspace(path, CurrentDynamoModel.CurrentWorkspace);
+            CurrentDynamoModel.CurrentWorkspace.Save(path);


This was done to make multiple home workspaces easier in the future.

QilongTang · 2017-06-12T19:08:29Z

Restarting EngOps Build to make sure Dynamo builds, other than that the changes looks good

QilongTang · 2017-06-12T19:16:03Z

@ikeough Looks like there are a few minor build errors after merging with master branch:
Graph\Workspaces\SerializationConverters.cs(203,39): error CS1002: ; expected [E:\Builds\Dynamo_master\Dynamo\src\DynamoCore\DynamoCore.csproj]
Graph\Workspaces\SerializationConverters.cs(279,38): error CS1002: ; expected [E:\Builds\Dynamo_master\Dynamo\src\DynamoCore\DynamoCore.csproj]

mjkkirschner · 2017-06-12T20:02:00Z

    /// construction and should not be serialized.
    /// </summary>
-    public class WorkspaceConverter : JsonConverter
+    public class WorkspaceReadConverter : JsonConverter


Hey @ikeough I'm curious what the advantage was for splitting the converters into two?

@mjkkirschner It's not being split in two. The workspace serialization doesn't need to use this converter. Trying to do so required references to dynamo model objects like the scheduler, which is not allowed in workspace. I removed this converter from the logic in ToJson(...), and changed the name to make it explicit that it's a converter for reading only.

Ian Keough added 15 commits June 6, 2017 14:23

Move workspace functionality into partials.

b6b5d77

Make NodeToCode static.

a9ff851

Make graph Layout a static class.

3df6e9e

Make Layout and Presets extension classes.

b42558a

More extensions.

0486f70

Remove one obsolete test. Use one extension method.

bc59473

Merge remote-tracking branch 'upstream/master' into QNTM-687

3b1b2c2

Downgrade one c# language feature to fix build. Make serialization ex…

2a5e913

…tension methods.

Move Save logic into Workspace save method.

3f6cdde

Serialize View block from WorkspaceViewModel.

5401d36

Merge branch 'master' into QNTM-687

45c4712

Merge branch 'QNTM-687' into QNTM-687-a

7357bda

Cleanup missing file references.

2d8428b

Merge branch 'QNTM-687' into QNTM-687-a

3ffcfd7

Merge remote-tracking branch 'upstream/master' into QNTM-687-a

badeb4b

ikeough commented Jun 12, 2017

View reviewed changes

Ian Keough added 3 commits June 12, 2017 00:23

Revert two changes which aren't required.

785cbae

Fix one regression with recent files list.

c9584fb

Fix two regressions where test now throws exception.

10f45da

QilongTang added the JSON label Jun 12, 2017

QilongTang reviewed Jun 12, 2017

View reviewed changes

Trigger OnSaved only when not saving backup.

1bc5799

Ian Keough added 2 commits June 12, 2017 11:20

Fix one typo.

59831a3

Merge remote-tracking branch 'upstream/master' into QNTM-687-a

6f9f109

mjkkirschner reviewed Jun 12, 2017

View reviewed changes

ikeough added this to the 2.0 milestone Jun 13, 2017

ikeough merged commit cef50a7 into DynamoDS:master Jun 13, 2017

ikeough deleted the QNTM-687-a branch June 13, 2017 17:22

Conversation

ikeough commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

Uh oh!

ikeough Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikeough Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikeough Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikeough Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QilongTang Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Jun 12, 2017

Uh oh!

QilongTang commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ikeough commented Jun 12, 2017 •

edited

Loading

ikeough Jun 12, 2017 •

edited

Loading

ikeough Jun 12, 2017 •

edited

Loading

ikeough Jun 12, 2017 •

edited

Loading

ikeough Jun 12, 2017 •

edited

Loading

QilongTang Jun 12, 2017 •

edited

Loading