Skip to content

Conversation

@mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Aug 16, 2019

set package loader to update dep UI
add this outisde the view to make it clear this is intended to be a singleton view
get rid of properties that were not set

Purpose

update dep UI when package is loaded to catch new installations.
also gets rid of ready params from extension - those are only set on model extensions...

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

@QilongTang

FYIs

@scottmitchell

set package loader to update dep UI
add this outisde the view to make it clear this is intended to be a singleton view
@mjkkirschner mjkkirschner changed the title get rid of properties that were not set update dev view UI when package installed. Aug 17, 2019
using Dynamo.Wpf.Extensions;
using System;
using System.Linq;
using System.Windows.Controls;
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/DynamoDS/Dynamo/wiki/Coding-Standards#usings I think we agreed from the recent conversation

// as we may have installed a missing package.
pmExtension.PackageLoader.PackgeLoaded += (package) =>
{
DependencyView.DependencyRegen(viewLoadedParams.CurrentWorkspaceModel as WorkspaceModel);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this way, if a package is downloaded using the package manager interface, the view extension here will pick up that change as well? Does that cover all the cases in Dynamo?

Copy link
Member Author

Choose a reason for hiding this comment

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

@QilongTang it doesn't cover other dependency types - I'm also not sure if it covers packages being uninstalled - do you want me to add this same handler for package removal? (if that event exists?)

Copy link
Member Author

Choose a reason for hiding this comment

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

it does exist pmExtension.PackageLoader.PackageRemoved

Copy link
Member Author

Choose a reason for hiding this comment

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

a bit hesitant to get into packageRemoval now and how it relates to package's being in a pending uninstall state.
Would like to defer adding this handler until later or atleast splitting this into two PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave the uninstall case for now, I would like to give it some testing first



[Obsolete("This method is not implemented and will be removed.")]
public void Ready(ReadyParams readyParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@QilongTang
Copy link
Contributor

Looks good with one question.

@mjkkirschner
Copy link
Member Author

@QilongTang flipped the usings - and one thought on the question about dynamo package cases -
Let me know what you think or if we can merge this.

@QilongTang
Copy link
Contributor

@mjkkirschner I think it is good for now. :shipit:

@mjkkirschner mjkkirschner merged commit 9243c17 into DynamoDS:master Aug 19, 2019
mjkkirschner added a commit to mjkkirschner/Dynamo that referenced this pull request Aug 20, 2019
* 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)
mjkkirschner added a commit that referenced this pull request Aug 20, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants