Dynamo Package Dependency View Extension #9787

QilongTang · 2019-06-10T21:13:06Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Dynamo Package Dependency View Extension using DataGrid.

This PR:

Adds a view extension managed by Dynamo Team to the Dynamo.all.sln so it will always be built with Dynamo with every new commit.
Such view extension displays graph package dependencies when there are packages logged in dyn not installed locally (no exact package name and version match)
Packages are marked grey when they are installed and red when they are not
There is currently a dummy download button beneath the dependency table, I created so we could show people mock-ups.

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.

Reviewers

@DynamoDS/dynamo

FYIs

QilongTang · 2019-06-10T21:15:48Z

I found adding extension project inside Dynamo repo is such a pain 👎 Localization resources seems a must-have and now getting this error which I have never encountered myself. Had no clue comparing with other view extensions and stackoverflow, will continue to look at the error

src/DynamoManipulation/DynamoManipulation.csproj

src/Dynamo.All.sln

src/PackageDependencyViewExtension/PackageDependencyViewExtension.csproj

src/DynamoCore/Graph/Workspaces/PackageDependencyInfo.cs

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

src/PackageDependencyViewExtension/PackageDependencyViewExtension.cs

mjkkirschner · 2019-06-12T00:25:36Z

this is getting cool.

1. Using ready param instead of data context 2. Exposed workspace cleared event for extension to watch for and clear dependency data

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

aparajit-pratap · 2019-06-14T13:37:26Z

@QilongTang you might have missed moving Notifications project to the new Extensions folder. It is also an extension (view extension).

QilongTang · 2019-06-14T14:26:35Z

@aparajit-pratap It is moved already, see my picture shot in PR discription

src/PackageDependencyViewExtension/PackageDependencyView.xaml

mjkkirschner · 2019-06-18T00:02:54Z

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

+            currentWorkspace = p.CurrentWorkspaceModel as WorkspaceModel;
+            p.CurrentWorkspaceChanged += OnWorkspaceChanged;
+            p.CurrentWorkspaceCleared += OnWorkspaceCleared;
+            currentWorkspace.PropertyChanged += OnWorkspacePropertyChanged;


you need to unsubscribe this event handler.

I see how this could always be the same workspace - but I think for safety's sake we should unsub this when the workspace changes and re sub it there.

yup, I removed because this is causing crashes sometimes (I think I showed you).. Let me add it back and test

mjkkirschner · 2019-06-18T00:05:31Z

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

+            {
+                DependencyRegen(obj as WorkspaceModel);
+                currentWorkspace = obj as WorkspaceModel;
+                currentWorkspace.PropertyChanged += OnWorkspacePropertyChanged;


I think here we should unsubscribe the old workspace before subscribing the new one - even if they are usually the same.

src/PackageDependencyViewExtension/PackageDependencyViewExtension.cs

mjkkirschner · 2019-06-18T00:13:37Z

@QilongTang one more UI point - the text editing cursor should not be shown over users hover over the entries in the list - this implies they can edit the field, which they cant. I would look at a textBlock instead of a text box with readonly set. Or if you prefer set the hover cursor manually.

Of course we can improve this in another PR.

https://docs.microsoft.com/en-us/dotnet/api/system.windows.controls.textblock?view=netframework-4.8

QilongTang · 2019-06-18T00:53:57Z

@mjkkirschner Please check the latest commit, addressed

mjkkirschner · 2019-06-18T01:16:03Z

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

@@ -38,6 +38,7 @@ internal void OnWorkspaceCleared(IWorkspaceModel obj)
        {
            if (obj is WorkspaceModel)
            {
+                currentWorkspace.PropertyChanged -= OnWorkspacePropertyChanged;


this seems like an odd place to do this -couldn't the workspace be cleared for some other reason... and if you clear the workspace but then don't add the the subscription back then you're not watching the workspace any longer...

I would have imagined this would be done in the onCurrentWorkspaceChanged handler - you cache the current workspace, unsubscribe the handler, then get the new current workspace and then subscribe?

Does that not work?

mjkkirschner · 2019-06-18T01:17:44Z

@QilongTang I think the logic for workspace event subscription is not correct. I know it's not straight forward because the workspace removed is rarely triggered, and the workspaceCleared is triggered at unexpected times but even with that it mind it seems odd...

How do you want to proceed with tests, as a followup?

src/PackageDependencyViewExtension/PackageDependencyView.xaml

QilongTang · 2019-06-18T01:39:56Z

@mjkkirschner Yes, I would like to address tests as follow up and we should really manual test this together

mjkkirschner · 2019-06-18T01:40:45Z

LGTM

scottmitchell · 2019-06-18T02:51:09Z

@QilongTang This is awesome!

scottmitchell · 2019-06-18T02:56:17Z

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs

+                        continue;
+                    }
+                }
+                // TODO: Not ideal! O(N * M) complexicty, would like LoadedPackageDependencies to be a dictionary or something with constant package name search time


Good point. I think we could improve these properties/objects a lot. What if we removed LoadedPackageDependencies and just added an IsLoaded property to the PackageDependencyInfo type?

I think there are a few reasons that it might make sense to split PackageDependencyInfo into two classes: PackageInfo and PackageDependency

@scottmitchell We have to also not only deal with loaded but also higher version which are compatible and other cases, like lower version exist but not sure if they should satisfy.. Any case, there are some aspects to think more here

QilongTang · 2019-06-18T06:05:31Z

@mjkkirschner @reddyashish Three UI recorded tests to be fixed but is more related to the view injection API behavior, merging

* Initial Commit * Update csproj to use project config imports * Add Data bind * Workspace Open with package info * adjust styling and make cells read only * adjust styling * reduce package cell width * Code clean Up 1. Using ready param instead of data context 2. Exposed workspace cleared event for extension to watch for and clear dependency data * Add Property watch for Package Dependencies * use name of * Some UI tunning * Styling changes * Using View injection API * grid styling * Highlight packages not installed * Add Download Button and view injection Menu Item * Some comments missing * code clean up * Address Comments * UI Adjustment * Update unsubscribe logic * More comments * Fix Unit Tests

Initial Commit

1126ba5

QilongTang added the WIP label Jun 10, 2019

mjkkirschner reviewed Jun 10, 2019

View reviewed changes

src/DynamoManipulation/DynamoManipulation.csproj Show resolved Hide resolved

mjkkirschner reviewed Jun 10, 2019

View reviewed changes

src/Dynamo.All.sln Show resolved Hide resolved

mjkkirschner reviewed Jun 10, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyViewExtension.csproj Outdated Show resolved Hide resolved

QilongTang added 2 commits June 10, 2019 17:53

Update csproj to use project config imports

4d9303a

Add Data bind

6428ad8

mjkkirschner reviewed Jun 11, 2019

View reviewed changes

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

mjkkirschner reviewed Jun 11, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jun 11, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyViewExtension.cs Outdated Show resolved Hide resolved

Workspace Open with package info

226dd31

QilongTang added 5 commits June 13, 2019 02:21

adjust styling and make cells read only

01ca109

adjust styling

57a91f9

reduce package cell width

1379d94

Code clean Up

4dbdcc5

1. Using ready param instead of data context 2. Exposed workspace cleared event for extension to watch for and clear dependency data

Add Property watch for Package Dependencies

22992d0

mjkkirschner reviewed Jun 14, 2019

View reviewed changes

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

mjkkirschner reviewed Jun 14, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyView.xaml.cs Outdated Show resolved Hide resolved

QilongTang added 2 commits June 14, 2019 00:16

use name of

abcce1f

Some UI tunning

7a270ad

Merge branch 'master' into PackageDependencyViewExtension

7357094

QilongTang added 5 commits June 14, 2019 13:17

Styling changes

e8c3667

Merge branch 'master' into PackageDependencyViewExtension

d4c5a0b

Using View injection API

5679bfe

grid styling

dc748c7

Highlight packages not installed

27c05fc

QilongTang added the PTAL Please Take A Look 👀 label Jun 17, 2019

QilongTang added 3 commits June 17, 2019 00:57

Some comments missing

48346eb

code clean up

4904b89

Merge branch 'master' into PackageDependencyViewExtension

0bebfcc

mjkkirschner reviewed Jun 18, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyView.xaml Show resolved Hide resolved

mjkkirschner reviewed Jun 18, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyViewExtension.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jun 18, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyViewExtension.cs Outdated Show resolved Hide resolved

QilongTang added 2 commits June 17, 2019 20:39

Address Comments

23b06bb

UI Adjustment

5d483b1

mjkkirschner reviewed Jun 18, 2019

View reviewed changes

Update unsubscribe logic

6c4ede7

reddyashish reviewed Jun 18, 2019

View reviewed changes

src/PackageDependencyViewExtension/PackageDependencyView.xaml Outdated Show resolved Hide resolved

More comments

c4d57ef

mjkkirschner added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Jun 18, 2019

scottmitchell reviewed Jun 18, 2019

View reviewed changes

QilongTang added 2 commits June 18, 2019 01:25

Merge branch 'master' into PackageDependencyViewExtension

924dca9

Fix Unit Tests

befc36a

QilongTang merged commit 43e501e into master Jun 18, 2019

QilongTang deleted the PackageDependencyViewExtension branch June 18, 2019 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamo Package Dependency View Extension #9787

Dynamo Package Dependency View Extension #9787

QilongTang commented Jun 10, 2019 •

edited

Loading

QilongTang commented Jun 10, 2019

mjkkirschner commented Jun 12, 2019

aparajit-pratap commented Jun 14, 2019 •

edited

Loading

QilongTang commented Jun 14, 2019 •

edited

Loading

mjkkirschner Jun 18, 2019

mjkkirschner Jun 18, 2019

QilongTang Jun 18, 2019

mjkkirschner Jun 18, 2019

QilongTang Jun 18, 2019

mjkkirschner commented Jun 18, 2019 •

edited

Loading

QilongTang commented Jun 18, 2019

mjkkirschner Jun 18, 2019

mjkkirschner commented Jun 18, 2019 •

edited

Loading

QilongTang commented Jun 18, 2019

mjkkirschner commented Jun 18, 2019

scottmitchell commented Jun 18, 2019

scottmitchell Jun 18, 2019

scottmitchell Jun 18, 2019

QilongTang Jun 18, 2019

QilongTang commented Jun 18, 2019

Dynamo Package Dependency View Extension #9787

Dynamo Package Dependency View Extension #9787

Conversation

QilongTang commented Jun 10, 2019 • edited Loading

Purpose

Declarations

Reviewers

FYIs

QilongTang commented Jun 10, 2019

mjkkirschner commented Jun 12, 2019

aparajit-pratap commented Jun 14, 2019 • edited Loading

QilongTang commented Jun 14, 2019 • edited Loading

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

Choose a reason for hiding this comment

mjkkirschner commented Jun 18, 2019 • edited Loading

QilongTang commented Jun 18, 2019

Choose a reason for hiding this comment

mjkkirschner commented Jun 18, 2019 • edited Loading

QilongTang commented Jun 18, 2019

mjkkirschner commented Jun 18, 2019

scottmitchell commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang commented Jun 18, 2019

QilongTang commented Jun 10, 2019 •

edited

Loading

aparajit-pratap commented Jun 14, 2019 •

edited

Loading

QilongTang commented Jun 14, 2019 •

edited

Loading

mjkkirschner commented Jun 18, 2019 •

edited

Loading

mjkkirschner commented Jun 18, 2019 •

edited

Loading