Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-3780 : fix modified flag for default home workspace #11761

Merged
merged 2 commits into from
Jun 15, 2021
Merged

DYN-3780 : fix modified flag for default home workspace #11761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8a0a18
fix modified flag for default home workspace
BogdanZavu Jun 15, 2021
bafca7a
add reg test
BogdanZavu Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 31 additions & 33 deletions src/GraphMetadataViewExtension/GraphMetadataViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,12 @@ public string GraphDescription
get { return currentWorkspace.Description; }
set
{
if (this.currentWorkspace != null && GraphDescription != value)
if (currentWorkspace != null && GraphDescription != value)
{
this.currentWorkspace.HasUnsavedChanges = true;
MarkCurrentWorkspaceModified();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This become repeated code in a few places, I would recommend you listen for Model property changed event since you RaisePropertyChanged() any way and you can MarkCurrentWorkspaceModified() in the handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is indeed a repetitive pattern across all these properties and we will look to address it as part of the next PRs. Not sure if we want to base that on property changed since we don't want to mark graph as modified every time the underlying property is changed - we want to leave that decision to the caller, in this case, the extension.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BogdanZavu Sure, on the handler you can check the property name and choose what to do. Just seems a bit cleaner and more scalable in the future to me but I will leave the decision to you. example here: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Menu/PreferencesViewModel.cs#L692

currentWorkspace.Description = value;
RaisePropertyChanged(nameof(GraphDescription));
}

currentWorkspace.Description = value;
RaisePropertyChanged(nameof(GraphDescription));
}
}

Expand All @@ -47,13 +46,12 @@ public string GraphAuthor
get { return currentWorkspace.Author; }
set
{
if (this.currentWorkspace != null && GraphAuthor != value)
if (currentWorkspace != null && GraphAuthor != value)
{
this.currentWorkspace.HasUnsavedChanges = true;
MarkCurrentWorkspaceModified();
currentWorkspace.Author = value;
RaisePropertyChanged(nameof(GraphAuthor));
}
currentWorkspace.Author = value;
RaisePropertyChanged(nameof(GraphAuthor));

}
}

Expand All @@ -65,13 +63,12 @@ public Uri HelpLink
get { return currentWorkspace.GraphDocumentationURL; }
set
{
if (this.currentWorkspace != null && HelpLink != value)
if (currentWorkspace != null && HelpLink != value)
{
this.currentWorkspace.HasUnsavedChanges = true;
MarkCurrentWorkspaceModified();
currentWorkspace.GraphDocumentationURL = value;
RaisePropertyChanged(nameof(HelpLink));
}

currentWorkspace.GraphDocumentationURL = value;
RaisePropertyChanged(nameof(HelpLink));
}
}

Expand All @@ -88,13 +85,12 @@ public BitmapImage Thumbnail
set
{
var base64 = value is null ? string.Empty : Base64FromImage(value);
if (this.currentWorkspace != null && base64 != currentWorkspace.Thumbnail)
if (currentWorkspace != null && base64 != currentWorkspace.Thumbnail)
{
this.currentWorkspace.HasUnsavedChanges = true;
MarkCurrentWorkspaceModified();
currentWorkspace.Thumbnail = base64;
RaisePropertyChanged(nameof(Thumbnail));
}

currentWorkspace.Thumbnail = base64;
RaisePropertyChanged(nameof(Thumbnail));
}
}

Expand Down Expand Up @@ -210,18 +206,15 @@ internal void AddCustomProperty(string propertyName, string propertyValue, bool
control.PropertyChanged += HandlePropertyChanged;
CustomProperties.Add(control);

if (markChange && this.currentWorkspace != null)
if (markChange)
{
this.currentWorkspace.HasUnsavedChanges = true;
MarkCurrentWorkspaceModified();
}
}

private void HandlePropertyChanged(object sender, EventArgs e)
{
if (this.currentWorkspace != null)
{
this.currentWorkspace.HasUnsavedChanges = true;
}
MarkCurrentWorkspaceModified();
}

private void HandleDeleteRequest(object sender, EventArgs e)
Expand All @@ -231,22 +224,27 @@ private void HandleDeleteRequest(object sender, EventArgs e)
customProperty.RequestDelete -= HandleDeleteRequest;
customProperty.PropertyChanged -= HandlePropertyChanged;
CustomProperties.Remove(customProperty);
if (this.currentWorkspace != null)
{
this.currentWorkspace.HasUnsavedChanges = true;
}
MarkCurrentWorkspaceModified();
}
}

private void MarkCurrentWorkspaceModified()
{
if (currentWorkspace != null && !string.IsNullOrEmpty(currentWorkspace.FileName))
{
currentWorkspace.HasUnsavedChanges = true;
}
}

public void Dispose()
{
public void Dispose()
{
this.viewLoadedParams.CurrentWorkspaceChanged -= OnCurrentWorkspaceChanged;

foreach (var cp in CustomProperties)
{
cp.RequestDelete -= HandleDeleteRequest;
cp.PropertyChanged -= HandlePropertyChanged;
}
}
}
}
}
25 changes: 25 additions & 0 deletions test/DynamoCoreWpfTests/ViewExtensions/GraphMetadataViewExtensionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ public void ExistingGraphWithCustomPropertiesWillBeAddedToExtension()
Open(@"core\CustompropertyTest.dyn");

// Assert
Assert.IsFalse(ViewModel.HomeSpace.HasUnsavedChanges);
Assert.IsTrue(customPropertiesBeforeOpen == 0);
Assert.That(propertiesExt.viewModel.CustomProperties.Count == 3);
Assert.That(propertiesExt.viewModel.CustomProperties[0].PropertyName == expectedCP1Key);
Expand All @@ -132,5 +133,29 @@ public void ExistingGraphWithCustomPropertiesWillBeAddedToExtension()
Assert.That(propertiesExt.viewModel.CustomProperties[2].PropertyName == expectedCP3Key);
Assert.That(propertiesExt.viewModel.CustomProperties[2].PropertyValue == expectedCP3Value);
}

[Test]
public void ExistingGraphOpenModifiedAndClosedWillSetAndClearModifiedFlag()
{
var extensionManager = View.viewExtensionManager;
var propertiesExt = extensionManager.ViewExtensions
.FirstOrDefault(x => x.Name == GraphMetadataViewExtension.extensionName)
as GraphMetadataViewExtension;

var customPropertiesBeforeOpen = propertiesExt.viewModel.CustomProperties.Count;
Open(@"core\CustompropertyTest.dyn");

Assert.IsFalse(ViewModel.HomeSpace.HasUnsavedChanges);

propertiesExt.viewModel.AddCustomProperty("TestPropertyName-X", "TestPropertyValue-X");

Assert.IsTrue(ViewModel.HomeSpace.HasUnsavedChanges);

ViewModel.HomeSpace.HasUnsavedChanges = false;
ViewModel.CloseHomeWorkspaceCommand.Execute(null);
ViewModel.MakeNewHomeWorkspace(null);

Assert.IsFalse(ViewModel.HomeSpace.HasUnsavedChanges);
}
}
}