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

Save warning due unresolved linter issues #11757

Merged
merged 10 commits into from
Jun 21, 2021
Merged

Save warning due unresolved linter issues #11757

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
87637ff
initial commit
SHKnudsen Jun 10, 2021
a967e58
Update DynamoViewModelDelegates.cs
SHKnudsen Jun 10, 2021
8620d3e
Update Dynamo.All.sln
SHKnudsen Jun 10, 2021
a2e2a13
Merge remote-tracking branch 'upstream/master' into Save-warning-due-…
SHKnudsen Jun 15, 2021
5f4ccb2
update test
SHKnudsen Jun 15, 2021
56d375e
event updates
SHKnudsen Jun 15, 2021
aeee7f4
test update
SHKnudsen Jun 15, 2021
4927fb4
Update DynamoViewModel.cs
SHKnudsen Jun 15, 2021
ac4a38b
Update Resources.en-US.resx
SHKnudsen Jun 16, 2021
6f7a287
comment updates
SHKnudsen Jun 21, 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
2 changes: 1 addition & 1 deletion src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2669,7 +2669,7 @@ private void DisplayXmlDummyNodeWarning()
OnRequestTaskDialog(null, args);
}

enum ButtonId
internal enum ButtonId
{
Ok = 43420,
Cancel,
Expand Down
3 changes: 2 additions & 1 deletion src/DynamoCoreWpf/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@
[assembly: InternalsVisibleTo("DocumentationBrowserViewExtension")]
[assembly: InternalsVisibleTo("SystemTestServices")]
[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]
[assembly: InternalsVisibleTo("PythonNodeModelsWpf")]
[assembly: InternalsVisibleTo("PythonNodeModelsWpf")]
[assembly: InternalsVisibleTo("LintingViewExtension")]
45 changes: 45 additions & 0 deletions src/DynamoCoreWpf/Properties/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions src/DynamoCoreWpf/Properties/Resources.en-US.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2399,6 +2399,21 @@ Uninstall the following packages: {0}?</value>
<data name="ResetCPythonButtonToolTip" xml:space="preserve">
<value>Resets CPython environment by reloading modules.</value>
</data>
<data name="GraphIssuesOnSave_CancelBtn" xml:space="preserve">
<value>Cancel and Show Issues</value>
</data>
<data name="GraphIssuesOnSave_Description" xml:space="preserve">
<value>There are unresolved issues with the graph type. If this graph is designed to be used outside of Dynamo, resolve all issues and save the graph again.</value>
</data>
<data name="GraphIssuesOnSave_ProceedBtn" xml:space="preserve">
<value>Save with Issues</value>
</data>
<data name="GraphIssuesOnSave_Summary" xml:space="preserve">
<value>You are trying to save a graph with unresolved issues</value>
</data>
<data name="GraphIssuesOnSave_Title" xml:space="preserve">
<value>Graph Type Issues found</value>
</data>
<data name="PackagePathsExpanderName" xml:space="preserve">
<value>Node and Package Paths</value>
</data>
Expand Down
15 changes: 15 additions & 0 deletions src/DynamoCoreWpf/Properties/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2399,6 +2399,21 @@ Uninstall the following packages: {0}?</value>
<data name="ResetCPythonButtonToolTip" xml:space="preserve">
<value>Resets CPython environment by reloading modules.</value>
</data>
<data name="GraphIssuesOnSave_CancelBtn" xml:space="preserve">
<value>Cancel and Show Issues</value>
</data>
<data name="GraphIssuesOnSave_Description" xml:space="preserve">
<value>There are unresolved issues with the graph type. If this graph is designed to be used outside of Dynamo, resolve all issues and save the graph again.</value>
</data>
<data name="GraphIssuesOnSave_ProceedBtn" xml:space="preserve">
<value>Save with Issues</value>
</data>
<data name="GraphIssuesOnSave_Summary" xml:space="preserve">
<value>You are trying to save a graph with unresolved issues</value>
</data>
<data name="GraphIssuesOnSave_Title" xml:space="preserve">
<value>Graph Type Issues found</value>
</data>
<data name="PackagePathsExpanderName" xml:space="preserve">
<value>Node and Package Paths</value>
</data>
Expand Down
59 changes: 59 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
using Dynamo.Selection;
using Dynamo.Services;
using Dynamo.UI;
using Dynamo.UI.Prompts;
using Dynamo.Updates;
using Dynamo.Utilities;
using Dynamo.Visualization;
Expand Down Expand Up @@ -758,6 +759,12 @@ internal void OnNodeViewReady(object nodeView)
}
}

internal event EventHandler RequestOpenLinterView;
internal void OnRequestOpenLinterView()
{
RequestOpenLinterView?.Invoke(this, EventArgs.Empty);
}

#region Event handler destroy/create

protected virtual void UnsubscribeAllEvents()
Expand Down Expand Up @@ -1535,6 +1542,8 @@ private bool CanOpenRecent(object path)
/// </summary>
private void Save(object parameter)
{
if (!ShowWarningDialogOnSaveWithUnresolvedIssues()) return;

if (!String.IsNullOrEmpty(Model.CurrentWorkspace.FileName))
{
// For read-only file, re-direct save to save-as
Expand Down Expand Up @@ -1864,12 +1873,16 @@ public void ShowSaveDialogIfNeededAndSaveResult(object parameter)
if (string.IsNullOrEmpty(vm.Model.CurrentWorkspace.FileName))
{
if (CanShowSaveDialogAndSaveResult(parameter))
{
ShowSaveDialogAndSaveResult(parameter);
}
}
else
{
if (CanSave(parameter))
{
Save(parameter);
}
}
}

Expand All @@ -1880,6 +1893,11 @@ internal bool CanShowSaveDialogIfNeededAndSaveResultCommand(object parameter)

public void ShowSaveDialogAndSaveResult(object parameter)
{
if (!ShowWarningDialogOnSaveWithUnresolvedIssues())
{
return;
}

var vm = this;

FileDialog _fileDialog = vm.GetSaveDialog(vm.Model.CurrentWorkspace);
Expand All @@ -1903,6 +1921,47 @@ public void ShowSaveDialogAndSaveResult(object parameter)
}
}

private bool ShowWarningDialogOnSaveWithUnresolvedIssues()
{
if (Model.LinterManager.RuleEvaluationResults.Count <= 0)
SHKnudsen marked this conversation as resolved.
Show resolved Hide resolved
{
return true;
}

string imageUri = "/DynamoCoreWpf;component/UI/Images/task_dialog_future_file.png";
var args = new TaskDialogEventArgs(
new Uri(imageUri, UriKind.Relative),
WpfResources.GraphIssuesOnSave_Title,
WpfResources.GraphIssuesOnSave_Summary,
WpfResources.GraphIssuesOnSave_Description);

args.AddRightAlignedButton((int)DynamoModel.ButtonId.Proceed, WpfResources.GraphIssuesOnSave_ProceedBtn);
args.AddRightAlignedButton((int)DynamoModel.ButtonId.Cancel, WpfResources.GraphIssuesOnSave_CancelBtn);

var dialog = new GenericTaskDialog(args);

if (DynamoModel.IsTestMode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall I think this looks good.
For the test I'm ok with it as it is but I'm not sure how much value we get since we're not going through the same route when user is at the helm and we're adding some code to make that happen.
How about we just test if the LinterView has opened when there are issues and cancel was pressed ?
I'm thinking to do this : we trigger an event for the cancel action for which we have a handler that will check issues and will open linter view if it has to ( this event will be triggered from both user mode and test mode ).
And this would be the testable part, we will just raise this event from the test, no pressing on Cancel button since we don't have access/can't control it.

{
dialog.Show();
if (dialog.IsActive)
{
var saveWarningArgs = new SaveWarningOnUnresolvedIssuesArgs(dialog);
OnSaveWarningOnUnresolvedIssuesShows(saveWarningArgs);
}
return false;
}

dialog.ShowDialog();

if (args.ClickedButtonId == (int)DynamoModel.ButtonId.Cancel)
{
OnRequestOpenLinterView();
return false;
}

return true;
}

internal bool CanShowSaveDialogAndSaveResult(object parameter)
{
return true;
Expand Down
15 changes: 15 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModelEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Dynamo.Graph.Notes;
using Dynamo.Graph.Workspaces;
using Dynamo.PackageManager;
using Dynamo.UI.Prompts;

namespace Dynamo.ViewModels
{
Expand Down Expand Up @@ -337,4 +338,18 @@ private static string GetInputNames(NodeModel node)
return string.Join(",", inputNames);
}
}

/// <summary>
/// Provides information about the task dialog used when saving a graph while there are unresolved linter issues in the graph.
/// This is meant to be used only for unit tests to verify that the dialog has been showed.
/// </summary>
internal class SaveWarningOnUnresolvedIssuesArgs : EventArgs
{
internal GenericTaskDialog TaskDialog;

internal SaveWarningOnUnresolvedIssuesArgs(GenericTaskDialog taskDialog)
{
TaskDialog = taskDialog;
}
}
}
12 changes: 12 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModelEvents.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,17 @@ internal void OnRequestReturnFocusToView()
if (RequestReturnFocusToView != null)
RequestReturnFocusToView();
}

/// <summary>
/// Event used to verify that the correct dialog is being showed when saving a graph with unresolved linter issues.
/// This is only meant to be used for unit testing purposes.
/// As the GenericTaskDialog is not owned by the DynamoWindow we need another way to verify that it shows up
/// when doing unit tests.
/// </summary>
internal event Action<SaveWarningOnUnresolvedIssuesArgs> SaveWarningOnUnresolvedIssuesShows;
internal void OnSaveWarningOnUnresolvedIssuesShows(SaveWarningOnUnresolvedIssuesArgs e)
{
SaveWarningOnUnresolvedIssuesShows?.Invoke(e);
}
}
}
3 changes: 3 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using Dynamo.Graph.Workspaces;
using Dynamo.Models;
using Dynamo.Selection;
using Dynamo.UI.Prompts;
using Dynamo.Utilities;
using Dynamo.Wpf.ViewModels;
using Dynamo.Wpf.ViewModels.Core;
Expand Down Expand Up @@ -556,7 +557,9 @@ internal void Save(string filePath, bool isBackup = false, EngineController engi
try
{
if (!isBackup)
{
Model.OnSaving(saveContext);
}

//set the name before serializing model.
this.Model.setNameBasedOnFileName(filePath, isBackup);
Expand Down
8 changes: 8 additions & 0 deletions src/LintingViewExtension/LintingViewExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public override void Loaded(ViewLoadedParams viewLoadedParams)
this.linterViewModel = new LinterViewModel(linterManager, viewLoadedParamsReference);
this.linterView = new LinterView() { DataContext = linterViewModel };

(viewLoadedParams.DynamoWindow.DataContext as DynamoViewModel).RequestOpenLinterView += OnRequestOpenLinterView;

// Add a button to Dynamo View menu to manually show the window
this.linterMenuItem = new MenuItem { Header = Resources.MenuItemText, IsCheckable = true };
this.linterMenuItem.Checked += MenuItemCheckHandler;
Expand All @@ -48,6 +50,12 @@ public override void Loaded(ViewLoadedParams viewLoadedParams)
this.linterManager.PropertyChanged += OnLinterManagerPropertyChange;
}

private void OnRequestOpenLinterView(object sender, System.EventArgs e)
{
if (linterMenuItem.IsChecked) return;
linterMenuItem.IsChecked = true;
}

public override void Shutdown()
{
// Do nothing for now
Expand Down
Loading