Repository navigation
ColorPicker Undo - #7769
ColorPicker Undo#7769
Conversation
Here is the correct branch with the Colorpicker undo functionality
|
Hey Mike here is the colorpicker undo. Please note the reason the colors are setup as RGBA rather then ARGB is due to the setup of the Color.cs class in Dynamo in CoreNodes. Please check the code and you will see the reason why I have to set things to the RGBA for correct coloring and functionality. Also note with the undo if we need the first undo to do 2 undo's we need to add some additional code to manage the additional change. Let me know if your happy with the current setup and I can add the additional functionality. |
|
The problem isn't that the format is ARGB, the problem is that the Format argument string says to expect but the arguments are in the order The string should be |
|
Sorry Greg, so due to the format when i save to xml and have to reopen on line 85 I have to pull the data back in as RGBA, so the ToString was to standardise that same output since the node actually uses the same DsColor class. However since they both use the same class the ToString is actually not used at all and I have since removed it so thanks for pointing that out! |
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.
| viewNode = nodeView; | ||
| colorPaletteNode = model; | ||
| mcolor = Color.FromArgb(model.dsColor.Alpha, model.dsColor.Red, model.dsColor.Green, model.dsColor.Blue); | ||
| model.PropertyChanged += Model_PropertyChanged; |
There was a problem hiding this comment.
I think you should unsubscribe this handler in the dispose method below.
|
|
||
| public event PropertyChangedEventHandler PropertyChanged; | ||
|
|
||
| protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null) |
There was a problem hiding this comment.
while this is nice, lets try to match all other onPropertyChanged handlers in the repo and just make this the property name string, drop the attribute, and just use the string and raise this directly with the string as an explicit parameter.
| using System.Text.RegularExpressions; | ||
| using System.Xml; | ||
|
|
||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
shouldn't need this if you remove the callerMember attribute
| [OutPortTypes("Color")] | ||
| [OutPortDescriptions("Selected Color.")] | ||
| public class ColorPalette : NodeModel | ||
| public class ColorPalette : NodeModel, INotifyPropertyChanged |
There was a problem hiding this comment.
I don't think you need this interface as NodeModel should already implement INotifyPropertyChanged
| Update = dscolor; | ||
| } | ||
| OnNodeModified(); | ||
| OnPropertyChanged(); |
There was a problem hiding this comment.
call this with the property name explicitly
| set | ||
| { | ||
| { | ||
| if (dscolor.Equals(value)) |
There was a problem hiding this comment.
this logic requires some comments
| colorPaletteNode = model; | ||
| mcolor = Color.FromArgb(model.dsColor.Alpha, model.dsColor.Red, model.dsColor.Green, model.dsColor.Blue); | ||
| model.PropertyChanged += Model_PropertyChanged; | ||
| var undoRecorder = viewNode.ViewModel.WorkspaceViewModel.Model.UndoRecorder; |
There was a problem hiding this comment.
any reason you are recording the state of the color picker immediately? When the user first selects a color - that should be the time you record the old color. Maybe this has to do with the two first undo bug you are seeing.
|
@Gytaco thanks for this work - I have a few comments for you to check out - also - if possible it would be great to add an undo test for this node. very similar to your other test, except invoke undo and asset the value is correct. |
|
@Gytaco you can see an example here: |
This fixes the double up of value stores, however it still requires to do an initial undo twice.
|
Hey @mjkkirschner here is the updated version. Are you happy with the type of hack?? I'm not but be interested to here if you have better ideas. Otherwise we can expand on this idea for the undo? Once I've got this sorted I'll write the unit tests. |
3 participants
Here is the correct branch with the Colorpicker undo functionality