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

Defending code #13696

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Defending code #13696

merged 2 commits into from
Jan 24, 2023

Conversation

jesusalvino
Copy link
Contributor

Purpose

Adding defensive code to deal the bug https://jira.autodesk.com/browse/DYN-5467

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

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

@mjkkirschner

FYIs

mjkkirschner
mjkkirschner reviewed Jan 20, 2023
View reviewed changes
src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs Outdated
}
catch (Exception ex)
{
dependencyViewExtension.OnMessageLogged(LogMessage.Info(string.Format(Properties.Resources.DependencyViewExtensionErrorTemplate, ex.ToString())));
Copy link
Member

Choose a reason for hiding this comment

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

perhaps it would be good to note in the message what failed to happen - so If I am correct about this code, it would be something like "failure to set package uninstall state" ? - I think it's acceptable for the text to be non localized if the message is not intended for users but more for debugging for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mjkkirschner
mjkkirschner reviewed Jan 20, 2023
View reviewed changes
src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs
@@ -187,9 +187,16 @@ internal void DependencyRegen(WorkspaceModel ws, bool forceCompute = false)
foreach (var package in dependencyViewExtension.pmExtension.PackageLoader.LocalPackages.Where(x =>
x.LoadState.ScheduledState == PackageLoadState.ScheduledTypes.ScheduledForDeletion || x.LoadState.ScheduledState == PackageLoadState.ScheduledTypes.ScheduledForUnload))
{
(packageDependencies.FirstOrDefault(x => x.Name == package.Name) as PackageDependencyInfo).State =
Copy link
Member

@mjkkirschner mjkkirschner Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I notice above that ws and its properties are never checked for being null... seems that would be valuable at the start of the function - you can probably use ? for most of it.

Copy link
Member

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators#null-conditional-operators--and-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mjkkirschner
mjkkirschner reviewed Jan 20, 2023
View reviewed changes
src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs
var targetInfo = pmExtension.PackageLoader.LocalPackages.Where(x => x.Name == packageDependencyInfo.Name).FirstOrDefault();
if (targetInfo != null)
{
packageDependencyInfo.Path = targetInfo.RootDirectory;
Copy link
Member

@mjkkirschner mjkkirschner Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

could it cause problems for this to be left null if an exception is caught? should it be to set to empty string or something? I don't actually know if it's null - just something to consider.

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 could be null

imagen

@zeusongit zeusongit merged commit 3256622 into DynamoDS:master Jan 24, 2023
zeusongit pushed a commit to zeusongit/Dynamo that referenced this pull request Feb 6, 2023
@jesusalvino @zeusongit
Defending code (DynamoDS#13696)
0e5ace0 
* Defending code

* refining code
@zeusongit zeusongit mentioned this pull request Feb 6, 2023
Merged
7 tasks
zeusongit added a commit that referenced this pull request Feb 7, 2023
@zeusongit @filipeotero @jesusalvino
Multiple cherrypicks to 2.17.1 (#13727)
dc8d57d 
* Dyn 5347 unified UI information icons (#13706)

* standardizing information icons in the preference settings

* Defending code (#13696)

* Defending code

* refining code

* Improve Package Manager search sort order (#13700)

* add search sort order

* Add option to preview or hide preview of all geometry in a group and Fix top menu pixel difference (#13702)

* Adjust border to fix 1 pixel anomaly

* remove unused login grid

* add logic to toggle preview for group

* add analytics

* Preview > Preview Geometry

* ensure that signout option no seen when user sign out

* DYN-4964-WorkingRange-Popup (#13656)

* DYN-4964-WorkingRange-Popup

I've added a new button in the Workspace that when is clicked will show a popup containing the working ranges and the one currently selected.
This new button will show a different image when the mouse is hover and when is clicked, also I've added a tooltip.
For implementing this functionality I've added a new Popup (GeometryScalingPopup.xaml) and it's corresponding ViewModel. Finally I've added a converter that will receive as a parameter the current Working Range and will return a Brush with a specific color so the checkmark will be visible or not.

* DYN-4964-WorkingRange-Popup CodeReview1

Added functionality for the new DefaultGeometryScaling property that will be serialized in the DynamoSettings.xml.
Also I've disconnected the functionality of selecting the Geometry Scale for the current Workspace in the Preferences panel, now will be selected from the Dynamo workspace and will be serialized in the dyn file (as currently is happening).

* DYN-4964-WorkingRange-Popup CodeReview1

Updated the string shown in Preferences panel (Geometry Scaling section) and the tooltip showed when the mouse is over the new Workspace button.

* Build Fix

When merging master to my branch there were some changed that I didn't noticed in the PR so I'm reverting back those changes.

* DYN-4964-WorkingRange-Popup CodeReview 2

Fixed several comments also several methods were removed (like RadioGeometryScaling_Checked method) or moved
The property GeoScalingViewModel was moved from DynamoViewModel to WorkspaceViewModel..
The property CurrentGeometryScaling was deleted due that was duplicating a functionality.

* DYN-4964-WorkingRange-Popup CodeReview 2

When changing the Workspace Geometry Scaling it was not running the graph so I did some changes so it will be running the graph every time is updated.

* DYN-4964-WorkingRange-Popup CodeReview 2

Add functionality for when a custom node is created the Workspace Scale Factor is set.
Updating and removing some comments and also I started to add the unit test.

* DYN-4964-WorkingRange-Popup CodeReview2

Updating Unit Test

* DYN-4964-WorkingRange-Popup Fixing Tests

I did the next fixes:
The test TestImportCopySettings() was failing due that was reading the DynamoSettings-NewSettings.xml and comparing against the properties in PreferencesSetting so the DefaultScaleFactor was missing in the DynamoSettings-NewSettings.xml file.

The test PreferencesGeoScaling_RunGraph_Automatic due that was opening the Preferences panel and changing the Geometry Scaling value for the workspace but now that this value was moved to the Workspace in a Popup then the code needed some changes so we can change the Geometry Scaling value using the Popup.

* Dyn 4964 popup geo scaling bugs (#13717)

* DYN-4964-PopupGeoScaling-Bugs

Fixing the next bugs reported by Aabi:
- The Popup was not closing when clicking in the Dynamo workspace (adding a similar fix like the Workspace ContextMenu).
- Adding a shadow over the new Geometry Scaling popup icon.

* DYN-4964-WorkingRange-Popup Bug fixes

There was a missing case that when the user click a Dynamo menu the Popup was not closing then I added some extra code to close it.

* DYN-4964-WorkingRange-Popup Bug Code Review1

Removing extra trailing spaces.

* Do not force user login for package search operation (#13725)

* restrict user login for pkg search

* check login state

* Revert "Dyn 4964 popup geo scaling bugs (#13717)"

This reverts commit 35005c3.

* Revert "DYN-4964-WorkingRange-Popup (#13656)"

This reverts commit a526390.

* Markdown procedural image location (#13693)

* Markdown procedural image location

- fixed hard-coded image location
- now identifies image location

* Fixed Failing Tests

- caught a bug causing issues when no image is present

* Revert "Markdown procedural image location (#13693)"

This reverts commit 299bbe9.

* Address review comments for 2.17.1 cherrypick PR (#13729)

---------

Co-authored-by: filipeotero <[email protected]>
Co-authored-by: jesusalvino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjkkirschner mjkkirschner mjkkirschner approved these changes

@reddyashish reddyashish Awaiting requested review from reddyashish

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

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