Ensure changes to IsInput & IsOutput mark the node/graph as modified #9257

alfarok · 2018-11-20T14:45:13Z

Purpose

Setting IsInput or IsOutput should mark the workspace as modified so the user will be notified to save when attempting to close a modified graph. I also noticed undo/redo is also broken for this functionality and working for other context menu items, will file an additional task as it was not included in this scope.

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.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @QilongTang

FYIs

@saintentropy

…ified

alfarok · 2018-11-20T14:45:44Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

+                if (isSetAsInput != value)
+                {
+                    isSetAsInput = value;
+                    this.OnNodeModified();


@mjkkirschner @QilongTang Is there a better even that can be raised here? I believe this event will force re-execution of the graph. I tried MarkNodeAsModified(false) which should mark as modified but not force execution as the AST is unchanged but it doesn’t seem to mark the graph as modified.

I think I need @aparajit-pratap 's input here, even though AST should be updated here, should the graph re-run? I don't see changing if a node is input or output would affect graph results, but maybe we should..

Yeah I wasn't sure about how clients would monitor these events and if re-execution is required say for Refinery.

mjkkirschner · 2018-11-20T16:34:54Z

@alfarok seems a little inconsistent:

for example, input and output seem just like changing the name of a node to me or any other data property, and for that we only do RaisePropertyChanged("Name") ... did you get a chance to investigate how this marks the graph with unsaved changes?

alfarok · 2018-11-20T17:10:41Z

@mjkkirschner Yes I did look into this a bit, it seems linked to the XML serialization/deserialization but I can take a deeper look into it. I quickly attempted to follow that pattern but it wasn't working, possibly because of an changed event name. I saw some old IsInput events that looks like they were changed when IsOutput was introduced.

For example we have:

IsInputNode
IsSetAsInput
isSelectedInput

I need to dig into the differences between these and when they are each used because I am personally not familiar with their behavior.

mjkkirschner · 2018-11-20T22:54:01Z

@alfarok IsInputNode vs IsSetAsInput - the prior determines which nodes can be inputs - the later determines if the node is currently set as input.

I have no idea what the third one is ;)

mjkkirschner · 2018-11-20T22:54:16Z

sorry, hit the close button 😢

aparajit-pratap · 2018-11-21T03:19:18Z

@alfarok I think the purpose of this task is to set the DYN file as modified when a node is set as an input or an output node so that the user is prompted to save the file before he closes it, to cite one reason for doing so. I don't think the purpose is to mark the "graph" or node itself as modified so that the graph is forced to re-execute. As @QilongTang points out I don't think that changing these properties of any node should warrant a graph update. Note that events like OnNodeModified and MarkAsModified are used to force graph/node re-execution.

jnealb · 2018-11-21T15:07:36Z

@alfarok @aparajit-pratap @mjkkirschner @saintentropy This will need to be cherry-picked to 2.0.2 (please forgive me if you already planned on it).

QilongTang · 2018-11-21T15:12:55Z

@jnealb On the opposite, we do not plan to do this as RC2.0.2 will be delivered next Friday so it is not likely we have room for this improvement there

alfarok · 2018-11-26T19:19:25Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

+        {
+            if(e.PropertyName == "IsSetAsInput" || e.PropertyName == "IsSetAsOutput")
+            {
+                this.HasUnsavedChanges = true;


@aparajit-pratap Is this more of what you meant by setting the DYN file as modified when a node is set as an input or an output node?

@alfarok Can you add a gif? I think we are clear that after marking node as Input/output:

Graph does not re-run

Graph flagged as touched, so when closing Dynamo, it asks user to save

@QilongTang added a gif, should be working as expected now without re-executing the workspace

@alfarok I'm still confused by this. Other nodes must be doing this same thing somewhere else, why is this new handler necessary?

@alfarok - a quick look and I can't find what I thought existed ;) - but I am still curious how setting Name essentially works and setting these properties requires a new handler?

If you decide to keep this handler please look into it being unsubscribed when the workspace is disposed.

@mjkkirschner made a few changes, let me know what you think.

mjkkirschner · 2018-11-26T20:43:12Z

src/DynamoCore/Graph/Nodes/NodeModel.cs

+                if (isSetAsOutput != value)
+                {
+                    isSetAsOutput = value;
+                    RaisePropertyChanged("IsSetAsOutput");


since we now target c# 6 you may want to use nameof?

mjkkirschner · 2018-11-26T21:47:10Z

@alfarok I like it more, but consider that now the graph will not have unsaved changes unless the view is present. 😉

This might be okay!? The dirty flag for unsaved changes is most useful if a user can actually click save...

So I'm okay with this change - @QilongTang what do you think?

Also - please add a test to make sure this does not regress - and if you keep this implementation it will need to be a view test.

QilongTang · 2018-11-26T21:53:51Z

@mjkkirschner Agree. LGTM to me. HasUnsavedChanges is not serialized and only affect workspace closing when ViewModel exists so should be safe to touch this way.

Good point of adding a test

…AsOutput marks graph as modified

alfarok · 2018-11-27T16:29:49Z

@QilongTang @mjkkirschner Added some testing coverage. This should be good to go, I will merge once the self-serve passes if there are no objections.

Ensure changes to Is Input & Is Output mark the node/graph as mod…

fd74e0a

…ified

alfarok commented Nov 20, 2018

View reviewed changes

mjkkirschner closed this Nov 20, 2018

mjkkirschner reopened this Nov 20, 2018

mark workspace as modified when input/output gets set

38924d2

alfarok commented Nov 26, 2018

View reviewed changes

alfarok added the PTAL Please Take A Look 👀 label Nov 26, 2018

mjkkirschner reviewed Nov 26, 2018

View reviewed changes

alfarok added 2 commits November 26, 2018 16:30

remove new handler

53daf56

switch to nameof

7128215

add testing coverage verifying modifications to IsSetAsInput or IsSet…

27a6e11

…AsOutput marks graph as modified

alfarok added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Nov 27, 2018

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

ae0dc8e

alfarok merged commit 1d6cd02 into DynamoDS:master Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure changes to IsInput & IsOutput mark the node/graph as modified #9257

Ensure changes to IsInput & IsOutput mark the node/graph as modified #9257

alfarok commented Nov 20, 2018 •

edited

Loading

alfarok Nov 20, 2018

QilongTang Nov 20, 2018

alfarok Nov 20, 2018

mjkkirschner commented Nov 20, 2018 •

edited

Loading

alfarok commented Nov 20, 2018 •

edited

Loading

mjkkirschner commented Nov 20, 2018 •

edited

Loading

mjkkirschner commented Nov 20, 2018

aparajit-pratap commented Nov 21, 2018

jnealb commented Nov 21, 2018

QilongTang commented Nov 21, 2018

alfarok Nov 26, 2018

aparajit-pratap Nov 26, 2018

QilongTang Nov 26, 2018

alfarok Nov 26, 2018

mjkkirschner Nov 26, 2018

mjkkirschner Nov 26, 2018

alfarok Nov 26, 2018

mjkkirschner Nov 26, 2018

mjkkirschner commented Nov 26, 2018

QilongTang commented Nov 26, 2018 •

edited

Loading

alfarok commented Nov 27, 2018

Ensure changes to IsInput & IsOutput mark the node/graph as modified #9257

Ensure changes to IsInput & IsOutput mark the node/graph as modified #9257

Conversation

alfarok commented Nov 20, 2018 • 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

mjkkirschner commented Nov 20, 2018 • edited Loading

alfarok commented Nov 20, 2018 • edited Loading

mjkkirschner commented Nov 20, 2018 • edited Loading

mjkkirschner commented Nov 20, 2018

aparajit-pratap commented Nov 21, 2018

jnealb commented Nov 21, 2018

QilongTang commented Nov 21, 2018

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

Choose a reason for hiding this comment

mjkkirschner commented Nov 26, 2018

QilongTang commented Nov 26, 2018 • edited Loading

alfarok commented Nov 27, 2018

alfarok commented Nov 20, 2018 •

edited

Loading

mjkkirschner commented Nov 20, 2018 •

edited

Loading

alfarok commented Nov 20, 2018 •

edited

Loading

mjkkirschner commented Nov 20, 2018 •

edited

Loading

QilongTang commented Nov 26, 2018 •

edited

Loading