Color picker undo fix by mjkkirschner · Pull Request #7813 · DynamoDS/Dynamo

mjkkirschner · 2017-04-22T18:13:40Z

Purpose

This PR enables undo/redo for the color picker - it finishes up changes initiated in this PR:
#7769

Because the xceed color picker provides no event before the color changes we use the property change event on the model to record the state of the model for undo, and save the state before we set the field to the new value.

As a consequence of this the undo action actually can set values into the undo redo stack... not good! So to avoid that another property Update is used to define when a redo/ undo should be recorded - An undo state should only be recorded when the new value is not equal to the current value or previous value - otherwise we are in an undo or redo.

There is also a test added - to facilitate this test I implemented UpdateValueCore for the color picker node.

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

@QilongTang

FYIs

@kronz @Gytaco

Here is the correct branch with the Colorpicker undo functionality

Since I am using the DsColor class for the node there is no need to have the override since it's being controlled there anyway and has been removed.

This fixes the double up of value stores, however it still requires to do an initial undo twice.

add undo test

QilongTang · 2017-04-24T20:20:21Z

-        public DSColor dsColor
+        private DSColor dscolor = DSColor.ByARGB(255, 0, 0, 0);
+        private DSColor prevColor = DSColor.ByARGB(255, 0, 0, 0);
+        private bool Isundo = false;


In what case do you expect this Isundo to be used? I only see it used in the setter. Also the naming is a bit confusing, can we make it more clearer by renaming to HasUndoableChange or IsUndoable, or something else

IsUndo gets used when an Undo or Redo command is actually doing the property setting - when an Undo or Redo is effecting the property state we want to ignore this state, and not propagate an update, as this will get recorded as an undo state... which is not how the undo recorder works.

What about calling it - DoNotRecordForUndo

to maybe clarify a bit more - we don't want undo's and redo's to alter the undo stack - just move up and down it.

@mjkkirschner Thanks for the explaination. Generally for boolean, we come up with a name starting with is has can should, so in this case maybe ShouldRecordForUndo?

sure, will go with that or something close.

QilongTang · 2017-04-24T20:21:10Z

            set
-            {                              
+            {
+                if (dscolor.Equals(value) ||prevColor.Equals(value))


Also curious why do we need to compare with the prevColor here?

we need to compare with previous color because if the value is same as the previous value or the current value we're potentially in an undo or a redo.

imagine undoing and redoing back and forth

Got you. Make sense

QilongTang · 2017-04-24T20:22:24Z

        public ColorPalette()
-        {
+        {           
+            //this.PropertyChanged += RaisePropertyChanged;


let's delete this

QilongTang · 2017-04-24T20:31:35Z

-        /// </summary>
-        public override bool IsInputNode
-        {
-            get { return false; }


Thought if we remove this we are allowing customizer to display color picker node?

good point, not sure why this was removed, I'll add it back - THOUGH, now that we have UI nodes 🃏

QilongTang

A few questions

add IsInputNode property back.

mjkkirschner · 2017-04-25T00:01:33Z

@QilongTang made the changes, PTAL

QilongTang · 2017-04-25T00:35:54Z

+                if (ShouldRecordForUndo)
+                {
+                    //If undo recorder is not being used updates the undorecorder stack with new value
+                    ShouldRecordForUndo = true;


When if satisfied, we do not need to set the boolean right? I feel like we can move this set to the else of the if above

QilongTang

One last question then LGTM

only set the undo state when the color picker window closes - this is closer to what is done with slider update tests

mjkkirschner · 2017-04-26T03:42:19Z

@QilongTang - after thinking about this and the flaw you identified for a while I decided to use a different approach.

I got rid of the two way binding and handle the updates between the UI and model manually - this way when the color picker window closes we still have the old nodeModel state to save into the undo recorder - then we manually convert the color and update the model.

We also bind to the property change event on the model manually and update the UI when the model changes, if the color is different than the one we already have selected.

Another test is added.

mjkkirschner · 2017-04-26T03:45:01Z

@QilongTang - the builds are failing due to a public nuget outage.

…o recorder

QilongTang · 2017-04-26T21:38:50Z

@mjkkirschner LGTM

Gytaco and others added 4 commits April 10, 2017 23:08

ColorPicker Undo

18141db

Here is the correct branch with the Colorpicker undo functionality

Removed ToString Override

4eb1960

Since I am using the DsColor class for the node there is no need to have the override since it's being controlled there anyway and has been removed.

Partial Undo Fix

a746c89

This fixes the double up of value stores, however it still requires to do an initial undo twice.

get undo working

54dcdd9

add undo test

QilongTang self-assigned this Apr 24, 2017

QilongTang reviewed Apr 24, 2017

View reviewed changes

invert boolean and rename it

89bd1bc

add IsInputNode property back.

QilongTang reviewed Apr 25, 2017

View reviewed changes

take more control over the UI and ditch the binding

9e786eb

only set the undo state when the color picker window closes - this is closer to what is done with slider update tests

one piece of missing comparison - dont save duplicate states into und…

dc3e457

…o recorder

QilongTang added the LGTM label Apr 26, 2017

mjkkirschner merged commit 42c36ea into DynamoDS:master Apr 26, 2017

Conversation

mjkkirschner commented Apr 22, 2017

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Apr 24, 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!

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 Apr 24, 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!

mjkkirschner Apr 24, 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!

QilongTang left a comment

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Apr 25, 2017

Uh oh!

QilongTang Apr 25, 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!

QilongTang left a comment

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Apr 26, 2017

Uh oh!

mjkkirschner commented Apr 26, 2017

Uh oh!

QilongTang commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mjkkirschner Apr 24, 2017 •

edited

Loading

QilongTang Apr 24, 2017 •

edited

Loading

mjkkirschner Apr 24, 2017 •

edited

Loading

QilongTang Apr 25, 2017 •

edited

Loading