Python engine property should copy/paste and undo correctly. by mjkkirschner · Pull Request #11006 · DynamoDS/Dynamo

mjkkirschner · 2020-08-14T19:59:24Z

Purpose

https://jira.autodesk.com/browse/DYN-3022
Some properties of nodeModel cannot be undone using undo/redo and do not copy/paste correctly. This includes the new Engine property on Python nodes.

This PR does the following:

adds xml serialization and deserialization of the missing properties to NodeModel and PythonNodeModel classes, as the undo redo recorder and copy/paste rely on legacy xml model serialization.
adds cases for the updateModelValue command handler on NodeModel to handle the properties we want to be undoable.
in the viewModel layer - anywhere we wish to have undoable property settings, we use a UpdateModelValueCommand to set the property, these are automatically serialized into the undo/redo recorder.
tests are added for the nodeModel properties, and for the Python engine properties.
Fix a small issue with the PythonNodeViewCustmizationTests and when the python engines were being scanned for to update the available engines.

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

python node update valure core can handle engine python nodeview right click uses umv commands

…irst test case as well as subsequent tests.

QilongTang · 2020-08-14T20:38:15Z

test/DynamoCoreWpfTests/PythonNodeCustomizationTests.cs

        {
-            libraries.Add("DSCPython.dll");
-            libraries.Add("DSIronPython.dll");
+            base.Start();


Can you remind me what does base.Start() do?

a bunch of stuff, it basically starts the DynamoModel, and in this case the View as well.

QilongTang · 2020-08-14T20:42:42Z

test/DynamoCoreWpfTests/PythonNodeCustomizationTests.cs

+            Assert.AreEqual(new List<string> { "2.7.9", "3.8.3" }, pynode2.CachedValue.GetElements().Select(x => x.Data));
+            DispatcherUtil.DoEvents();
+            Model.CurrentWorkspace.Undo();
+            Assert.AreEqual(new List<string> { "3.8.3", "3.8.3" }, pynode2.CachedValue.GetElements().Select(x => x.Data));


QilongTang

Thank you for adding tests for the other newly supported commands within this PR. Looks good to me

QilongTang · 2020-08-14T20:44:41Z

src/Libraries/PythonNodeModels/PythonNode.cs

            element.AppendChild(script);
+            XmlElement engine = element.OwnerDocument.CreateElement(nameof(Engine));
+            engine.InnerText = Enum.GetName(typeof(PythonEngineVersion), Engine);
+            element.AppendChild(engine);


Seems these can still be easily converted to use Json in the future

Agreed. The fact that there are things in Dynamo depending on XML serialization, when we have obsoleted it, is scary.

I will file a task to do a spike on this, I believe we did one way back when we looked at JSON originally, maybe I can dig up some notes.

mmisol

Added some minor comments. Looks good overall

mmisol · 2020-08-14T22:32:33Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

-                    nodeLogic.IsSetAsInput = value;
-                    RaisePropertyChanged("IsSetAsInput");
+                    DynamoViewModel.ExecuteCommand(
+               new DynamoModel.UpdateModelValueCommand(


Can you check formatting here? If this was Python the statement would be outside the if :)

mmisol · 2020-08-14T22:32:45Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

-                    nodeLogic.IsSetAsOutput = value;
-                    RaisePropertyChanged("IsSetAsOutput");
+                    DynamoViewModel.ExecuteCommand(
+               new DynamoModel.UpdateModelValueCommand(


mmisol · 2020-08-14T22:33:55Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

-                RaisePropertyChanged("IsDisplayingLabels");
+
+                    DynamoViewModel.ExecuteCommand(
+               new DynamoModel.UpdateModelValueCommand(


No if for this property?

yeah, I'm not sure why that is, I didn't touch it because it didn't exist before and was present for those other properties - I can make the change and see if there are any test failures if you would like.

I guess it would be more consistent but not really required. Your call

mmisol · 2020-08-14T22:37:30Z

src/Libraries/PythonNodeModels/PythonNode.cs

            element.AppendChild(script);
+            XmlElement engine = element.OwnerDocument.CreateElement(nameof(Engine));
+            engine.InnerText = Enum.GetName(typeof(PythonEngineVersion), Engine);
+            element.AppendChild(engine);


Agreed. The fact that there are things in Dynamo depending on XML serialization, when we have obsoleted it, is scary.

mmisol · 2020-08-14T22:40:03Z

test/DynamoCoreTests/CoreTests.cs


+        [Test]
+        [Category("UnitTests")]
+        public void CanCopydAndPasteAndUndoShowLabels()


Typo Copyd => Copy. I see this is consistently copied over to other tests.

…S#11006) merging this in and will cp to 2.8

…#11017) merging this in and will cp to 2.8

mjkkirschner added 7 commits August 11, 2020 16:06

save load engine and display labels to xml

b5301ae

add value core handlers for display labels, input, outut

8053d6b

properties should use upate model value commands to be undoable

9420fcc

python node update valure core can handle engine python nodeview right click uses umv commands

update menu item click handlers for pystring node

e509be0

move engine value update to base class

cc3e54b

tests

154791f

move scanpythonengines function to location where it works for very f…

88716ac

…irst test case as well as subsequent tests.

mjkkirschner requested review from QilongTang and zeusongit August 14, 2020 19:59

mjkkirschner changed the title ~~Pythonundo~~ Python engine property should copy/paste and undo correctly. Aug 14, 2020

QilongTang reviewed Aug 14, 2020

View reviewed changes

QilongTang approved these changes Aug 14, 2020

View reviewed changes

QilongTang reviewed Aug 14, 2020

View reviewed changes

mmisol reviewed Aug 14, 2020

View reviewed changes

mjkkirschner added this to the 2.8 milestone Aug 17, 2020

review comments

e0be420

mmisol approved these changes Aug 17, 2020

View reviewed changes

mjkkirschner merged commit 3a35539 into DynamoDS:master Aug 17, 2020

mjkkirschner added a commit to mjkkirschner/Dynamo that referenced this pull request Aug 17, 2020

Python engine property should copy/paste and undo correctly. (DynamoD…

9de0867

…S#11006) merging this in and will cp to 2.8

mjkkirschner mentioned this pull request Aug 17, 2020

Python engine property should copy/paste and undo correctly. (#11006) #11017

Merged

mjkkirschner added a commit that referenced this pull request Aug 18, 2020

Python engine property should copy/paste and undo correctly. (#11006) (…

8302c71

…#11017) merging this in and will cp to 2.8

Conversation

mjkkirschner commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Uh oh!

QilongTang Aug 14, 2020 • 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!

mjkkirschner Aug 14, 2020 • 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!

QilongTang left a comment

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!

mjkkirschner Aug 17, 2020 • 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!

mmisol left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjkkirschner commented Aug 14, 2020 •

edited

Loading

QilongTang Aug 14, 2020 •

edited

Loading

mjkkirschner Aug 14, 2020 •

edited

Loading

mjkkirschner Aug 17, 2020 •

edited

Loading