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

Jump to bottom

Handling the Custom Nodes and Trusted Location message #13138

Merged
merged 10 commits into from
Aug 5, 2022
Merged

Handling the Custom Nodes and Trusted Location message #13138

merged 10 commits into from
Aug 5, 2022

Conversation

jesusalvino
Copy link
Contributor

@jesusalvino jesusalvino commented Jul 26, 2022 •
edited by QilongTang
Loading

Purpose

Handling the Trusted location message when a custom node space is opened.
https://jira.autodesk.com/browse/DYN-5089

Declarations

Check these if you believe they are true

  • The codebase 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.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@QilongTang

FYIs

@RobertGlobant20 @filipeotero

@jesusalvino
Copy link
Contributor Author

trusted

@QilongTang
Copy link
Contributor

QilongTang commented Jul 27, 2022 •
edited
Loading

Although this change is good, I don't think we should display trust warning in the bug senario from the first place. Can you hide the trust warning when switching to a different workspace which is a custom node workspace?

@jesusalvino
Copy link
Contributor Author

Although this change is good, I don't think we should display trust warning in the bug senario from the first place. Can you hide the trust warning when switching to a different workspace which is a custom node workspace?

trusted4

QilongTang
QilongTang reviewed Aug 3, 2022
View reviewed changes
src/DynamoCoreWpf/Controls/NotificationsControl.xaml.cs Outdated
{
hsvm.DynamoViewModel.FileTrustViewModel.ShowWarningPopup = false;
hsvm.ClearWarning();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm why do we need to touch this piece of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it forces to hide the untrusted message and bubble when we start to work in a just created custom node workspace, not only when we switch the workspaces.

QilongTang
QilongTang reviewed Aug 3, 2022
View reviewed changes
src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs Outdated
@@ -270,6 +270,7 @@ internal void UpdateRunStatusMsgBasedOnStates()
if(IsHomeSpace && !IsCurrentSpace)
{
SetCurrentWarning(NotificationLevel.Mild, string.Empty);
DynamoViewModel.FileTrustViewModel.ShowWarningPopup = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the gif, there will be a flash of trust warning displayed then disappear.. Less desirable, can we hide it even earlier?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we still need this fix here in addition to other changes?

@QilongTang
Copy link
Contributor

@jesusalvino Nice tries but I would love something more granular, for example, you can touch the visibility at https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs#L220 before executing the command to switch tabs. Or you can subscribe to ViewingHomespace property change to determine the visibility of file trust warning. From my point of view, touching visibility after the tab is switched is less desirable to me.

@jesusalvino
Copy link
Contributor Author

@jesusalvino Nice tries but I would love something more granular, for example, you can touch the visibility at https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs#L220 before executing the command to switch tabs. Or you can subscribe to ViewingHomespace property change to determine the visibility of file trust warning. From my point of view, touching visibility after the tab is switched is less desirable to me.

Done

trustedx

QilongTang
QilongTang reviewed Aug 4, 2022
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

Looks much cleaner, LGTM once the remaining comments addressed

@jesusalvino
Copy link
Contributor Author

Looks much cleaner, LGTM once the remaining comments addressed

Done

mjkkirschner
mjkkirschner reviewed Aug 4, 2022
View reviewed changes
src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs Outdated
SetupFooterNotificationItems();
}

private void DynamoViewModel_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == "CurrentSpace" && !(sender as DynamoViewModel).ViewingHomespace)
Copy link
Member

Choose a reason for hiding this comment

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

use nameof to avoid spelling mistakes and make refactoring easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use nameof to avoid spelling mistakes and make refactoring easier.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang Kindly, Could you let me know if we have any pending here please ?

QilongTang
QilongTang approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM

@QilongTang QilongTang added this to the 2.16.0 milestone Aug 4, 2022
@QilongTang
Copy link
Contributor

Waiting for PR checks and also checking @jesusalvino is the last gif still valid?

@jesusalvino
Copy link
Contributor Author

Waiting for PR checks and also checking @jesusalvino is the last gif still valid?

yes, the last gif is valid since the behavior is the same with the refactoring

@QilongTang
Copy link
Contributor

passed at https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1043

@QilongTang QilongTang merged commit d4e50f1 into DynamoDS:master Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang approved these changes

@mjkkirschner mjkkirschner mjkkirschner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jesusalvino @QilongTang @mjkkirschner