DYN-1874: I want to download missing graph package dependencies easily #9854

scottmitchell · 2019-07-23T20:48:49Z

Purpose

This PR addresses JIRA task DYN-1874. It adds an API to the PackageManagerClientViewModel to initiate download and install of a package. It also adds the ability for the Dependency Viewer to download missing packages.

Declarations

Check these if you believe they are true

The code base 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 feature is in Preview Mode

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

…kage

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

QilongTang · 2019-07-28T19:11:50Z

A few comments, but looks pretty good so far and thank you for adding row details function in an early stage. Since there are lots of new style changes, have you looked at reusing color defined from Dynamo style sheets instead of using hex codes?

src/DynamoCoreWpf/Extensions/ViewLoadedParams.cs

mjkkirschner · 2019-07-28T21:20:45Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

-    /// Class defining data for each column
-    /// </summary>
-    public class ColumnData
+    public class NodeLibraryDependencyRow


did something weird happen to this file? The diff is pretty hard to read, it looks like a bunch of public classes were deleted? Or am I mis-reading the diff?

(you can't delete the public classes if they get released with another version - however short lived that version is) it's safer to just obsolete them as we don't know what binary compatibility issues this will cause without extensive testing, and it's just not worth it IMO.

@mjkkirschner Do you remember the last case this was an issue? This class is only used for data binding to the UI so we can't make it internal. If any class went out with a beta release that we can no longer rename, that sounds like too strict..

I'm not talking about beta releases.

If it's in 2.3 then we should consider it stable, unless @scottmitchell wants to refactor 2.3 before release to not include this class and then redo testing there.

Sure, I guess I am wondering if the API restriction need to be applied on extension level. I understand once it is in RC branch we should make it stable but this dll is not in our public nuget so I do not expect developers to consume it nor us maintaining the stability.. My 2 cents.. Unless you are talking about internal ones..

@QilongTang - that's a great great point! I think it's acceptable to make this entire .dll not part of the API.

👍

I think we'll need to consider what consequences this has technically and also for build processes though.

Where do we communicate/document this?

@mjkkirschner I don't think we have it specified according quickly glancing at https://github.com/DynamoDS/Dynamo/wiki/Dynamo-Versions or https://github.com/DynamoDS/Dynamo/wiki/API-Changes. And seems we also missed 2.x versions' API changes...

regarding the API Changes doc... I find it kind of useless work - the API diff job now computes this for us - or something like
https://www.fuget.org/packages/DynamoVisualProgramming.Core/2.2.1.5204/lib/net47/diff/2.1.0.7465/

is SOO much better than that page - it's only missing the explanations/readme portion.

scottmitchell · 2019-07-29T21:09:15Z

@mjkkirschner @QilongTang I think I've addressed all the comments in this PR. In terms of the API / semantic versioning issue for the WorkspaceDependencyViewExtension--I think this will be addressed by release notes and by a PR to 2.3 marking the view extension (and/or its classes) as obsolete/beta.

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

src/DynamoCore/Graph/Workspaces/PackageDependencyInfo.cs

src/DynamoCoreWpf/ViewModels/PackageManager/IPackageDownloader.cs

mjkkirschner · 2019-07-29T21:31:48Z

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs

+                        break;
+
+                    case PackageDependencyState.Missing:
+                        iconPath = "NodeLibraryDependency_Missing.png";


would it be possible to move these to a resx file? I guess we can do it as as followup - raw resources like this are difficult to manage and make patch installer issues much worse.

for example look at:
https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/GeometryUIWpf/ProtoGeometryImages.resx

@mjkkirschner I spent some time trying to figure this out last night, but couldn't quite get it to work. If it's alright with you, I'd like to deal with this in a follow up UI pass.

@mjkkirschner Finally got it. Turns out we have an existing utility for converting bitmaps to ImageSources: https://github.com/DynamoDS/Dynamo/blob/ec10f936824152e7dd7d6d019efdcda0d78a5264/src/DynamoCoreWpf/Utilities/ResourceUtilities.cs

aparajit-pratap

@scottmitchell one main concern about the API breaking change. Is there anyway to avoid adding the interface?

src/DynamoCoreWpf/ViewModels/PackageManager/IPackageDownloader.cs

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

scottmitchell · 2019-07-30T13:46:17Z

@aparajit-pratap @mjkkirschner Thank you for the comments. I believe I have addressed them all--the one thing I did not fix here that will need a follow up is moving the package state icons to a resx file

scottmitchell · 2019-08-03T18:44:52Z

@mjkkirschner I have moved the dependency state icons to a resx, and also added a "Package Manager Terms of Use" check to the new package download API (thanks for pointing this out @kronz). This PR should be good to go now.

QilongTang · 2019-08-05T16:26:50Z

The TOU check and image changes looks good. Thank you for removing the static images so they will not pollute binary compatibility. Merging for testing in 2.5.

DynamoDS#9854) * Add IPackageInfo interface * Add IPackageInfo interface * Add PackageManagerClient API to intiate download and install of a package * Add download button to test package download * Add IPackageInfo interface * Use PD state * Fix comment * Refactor Dependency Viewer UI * Rename ExecutePackage -> ExecutePackageDownload * Add PackageManagerClientViewModel to ViewLoadedParams * Add IPackageDownloader to LoadedViewParams * Move messages to resource file * Add state-based messages * Font size * Keep ReferenceType and INodeLibraryDepInfo internal * IPackageDownloader.cs -> IPackageInstaller.cs * IPackageInstaller comments * Move details message to resx * Fix resource conflict * Check terms of use acceptance before downloading package * Embed icons in resources.resx * Remove raw resources * Add summary to public class * Add summaries for PackageDependencyRow members (cherry picked from commit 8ca9d0f)

* DYN-1874: I want to download missing graph package dependencies easily (#9854) * Add IPackageInfo interface * Add IPackageInfo interface * Add PackageManagerClient API to intiate download and install of a package * Add download button to test package download * Add IPackageInfo interface * Use PD state * Fix comment * Refactor Dependency Viewer UI * Rename ExecutePackage -> ExecutePackageDownload * Add PackageManagerClientViewModel to ViewLoadedParams * Add IPackageDownloader to LoadedViewParams * Move messages to resource file * Add state-based messages * Font size * Keep ReferenceType and INodeLibraryDepInfo internal * IPackageDownloader.cs -> IPackageInstaller.cs * IPackageInstaller comments * Move details message to resx * Fix resource conflict * Check terms of use acceptance before downloading package * Embed icons in resources.resx * Remove raw resources * Add summary to public class * Add summaries for PackageDependencyRow members (cherry picked from commit 8ca9d0f) * merge conflict assemblyInfo * remove unnecessary API * update dev view UI when package installed. (#9915) * get rid of properties that were not set set package loader to update dep UI add this outisde the view to make it clear this is intended to be a singleton view * flipped usings (cherry picked from commit 9243c17) * review comments * sneaky equals

scottmitchell added 8 commits July 19, 2019 13:53

Add IPackageInfo interface

83a995c

Add IPackageInfo interface

8b244db

Add PackageManagerClient API to intiate download and install of a pac…

b03663e

…kage

Add download button to test package download

d54d9a4

Merge Master

9096e8a

Add IPackageInfo interface

32c867c

Use PD state

319f497

Fix comment

f5a4dca

scottmitchell changed the title ~~DYN-1874: I want to download missing graph package dependencies easily~~ [WIP] DYN-1874: I want to download missing graph package dependencies easily Jul 23, 2019

mjkkirschner reviewed Jul 23, 2019

View reviewed changes

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 23, 2019

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Outdated Show resolved Hide resolved

scottmitchell added 3 commits July 26, 2019 15:27

Refactor Dependency Viewer UI

968caa3

Rename ExecutePackage -> ExecutePackageDownload

5b36a36

Add PackageManagerClientViewModel to ViewLoadedParams

12e7fd1

scottmitchell changed the title ~~[WIP] DYN-1874: I want to download missing graph package dependencies easily~~ DYN-1874: I want to download missing graph package dependencies easily Jul 28, 2019

QilongTang reviewed Jul 28, 2019

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Outdated Show resolved Hide resolved

QilongTang reviewed Jul 28, 2019

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 28, 2019

View reviewed changes

src/DynamoCoreWpf/Extensions/ViewLoadedParams.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 28, 2019

View reviewed changes

scottmitchell added 4 commits July 29, 2019 15:01

Add IPackageDownloader to LoadedViewParams

6ac49fb

Move messages to resource file

7b48355

Add state-based messages

0e2395b

Font size

5d2ffc3

scottmitchell commented Jul 29, 2019

View reviewed changes

src/WorkspaceDependencyViewExtension/WorkspaceDependencyView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 29, 2019

View reviewed changes

src/DynamoCore/Graph/Workspaces/PackageDependencyInfo.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 29, 2019

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/IPackageDownloader.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jul 29, 2019

View reviewed changes

aparajit-pratap requested changes Jul 30, 2019

View reviewed changes

aparajit-pratap approved these changes Jul 30, 2019

View reviewed changes

scottmitchell added 6 commits July 30, 2019 02:01

Keep ReferenceType and INodeLibraryDepInfo internal

48d8f2a

IPackageDownloader.cs -> IPackageInstaller.cs

a502ad0

IPackageInstaller comments

0ba290a

Move details message to resx

50d28e6

Fix resource conflict

5af95b6

Fix resource conflict

db28e0c

scottmitchell added 3 commits August 3, 2019 12:47

Check terms of use acceptance before downloading package

f9ac4b2

Embed icons in resources.resx

8a19113

Remove raw resources

1bb8afa

scottmitchell requested a review from mjkkirschner August 3, 2019 18:33

scottmitchell added 2 commits August 3, 2019 14:36

Add summary to public class

ef2fec8

Add summaries for PackageDependencyRow members

6ca7e5d

QilongTang merged commit 8ca9d0f into DynamoDS:master Aug 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-1874: I want to download missing graph package dependencies easily #9854

DYN-1874: I want to download missing graph package dependencies easily #9854

scottmitchell commented Jul 23, 2019 •

edited

Loading

QilongTang commented Jul 28, 2019

mjkkirschner Jul 28, 2019

mjkkirschner Jul 28, 2019

QilongTang Jul 29, 2019

mjkkirschner Jul 29, 2019

QilongTang Jul 29, 2019 •

edited

Loading

mjkkirschner Jul 29, 2019 •

edited

Loading

QilongTang Jul 29, 2019

mjkkirschner Jul 29, 2019

scottmitchell commented Jul 29, 2019

mjkkirschner Jul 29, 2019

mjkkirschner Jul 29, 2019

scottmitchell Jul 30, 2019

scottmitchell Aug 3, 2019

aparajit-pratap left a comment

scottmitchell commented Jul 30, 2019

scottmitchell commented Aug 3, 2019

QilongTang commented Aug 5, 2019

DYN-1874: I want to download missing graph package dependencies easily #9854

DYN-1874: I want to download missing graph package dependencies easily #9854

Conversation

scottmitchell commented Jul 23, 2019 • edited Loading

Purpose

Declarations

Reviewers

FYIs

QilongTang commented Jul 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmitchell commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

scottmitchell commented Jul 30, 2019

scottmitchell commented Aug 3, 2019

QilongTang commented Aug 5, 2019

scottmitchell commented Jul 23, 2019 •

edited

Loading

QilongTang Jul 29, 2019 •

edited

Loading

mjkkirschner Jul 29, 2019 •

edited

Loading