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.

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

DYN-3572 std.lib should be visible in manage node and package path UI #11622

Merged
merged 29 commits into from
Apr 23, 2021

Conversation

sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Apr 16, 2021

Purpose

DYN-3572 std.lib should be visible in manage node and package path UI

Note: This will also fix DYN-3570 and partially fix DYN-3571 as it will stop Packet manager from downloading packages to the Standard Library directory (but it is till possible to download to the %ProgramData% directory).

This pull request does:

  • filters out the standard library directory token from the directory list in in PathManager.
  • add the standard library directory token as the first folder in DynamoModel.
  • insert the standard library directory token as the first folder if missing in DynamoModel.
  • add Standard Library to the resources in PackagePathViewModel so it can be translated.
  • substitute the standard library token with the Standard Library resource when the dialog initializes.
  • prevent the standard library entry from being deleted in PackagePathViewModel.
  • prevent the standard library entry from being updated in PackagePathViewModel.
  • update the wording in the Package Path dialog.
  • show the Standard Library with Italic font type if Standard Library is disabled.
  • substitute the translated Standard Library resource with the standard library token when changes are committed in the dialog.
  • make sure that the DefaultPackagesDirectory property points to the first non standard library entry in PackageLoader
  • add standard library first if missing in PackageLoader
  • substitute the standard library token with the runtime standard library directory in PackageLoader

Normal dialog:
image

Dialog when Standard Library is disabled:
image

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

@sm6srw sm6srw added the WIP label Apr 16, 2021
@sm6srw sm6srw changed the title WIP: DYN-3572 WIP: DYN-3572 std.lib should be visible in manage node and package path UI Apr 20, 2021
@sm6srw sm6srw changed the title WIP: DYN-3572 std.lib should be visible in manage node and package path UI DYN-3572 std.lib should be visible in manage node and package path UI Apr 20, 2021
@sm6srw sm6srw removed the WIP label Apr 20, 2021
@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Apr 20, 2021

image
In this dialog, can we change the description to say that the first path below "Standard Library" is the default save location. Can we also explain that the Standard Library is the location from where packages will be loaded first followed by the rest of the paths.

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 20, 2021

image
In this dialog, can we change the description to say that the first path below "Standard Library" is the default save location. Can we also explain that the Standard Library is the location from where packages will be loaded first followed by the rest of the paths.

Good point. I totally missed that text. I will try to come up with some wording that covers the new behavior.

@mjkkirschner
Copy link
Member

image
In this dialog, can we change the description to say that the first path below "Standard Library" is the default save location. Can we also explain that the Standard Library is the location from where packages will be loaded first followed by the rest of the paths.

Good point. I totally missed that text. I will try to come up with some wording that covers the new behavior.

this is actually covered in:
https://jira.autodesk.com/browse/DYN-3571

but fixing it now is even better.

I think it may ultimately change, but it's good to make it as correct as possible IMO.

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 20, 2021

@sm6srw - theres one AC that was included in the task that I don't see in your description:
if std.lib is disabled, the entry in the ui should appear disabled - what do you think of this one? We could also leave this for later depending on if we expose those options to the UI as well.

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 20, 2021

This looks pretty solid to me - one thought that occurs to me is that depending on our final strategy for dealing with conflicts during package install (as opposed to Dynamo startup) - that we may reevaluate std.lib's location (first or last), additionally feedback from UX may lead us to rethink this anyway.

Given that, do you think the position is difficult to change after this PR or it will be pretty straightforward? - I was wondering if the logic could be abstracted into a position field somewhere we could set.

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 21, 2021

@sm6srw - theres one AC that was included in the task that I don't see in your description:
if std.lib is disabled, the entry in the ui should appear disabled - what do you think of this one? We could also leave this for later depending on if we expose those options to the UI as well.

Thanks, I missed this one. I will take a look.

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 21, 2021

This looks pretty solid to me - one thought that occurs to me is that depending on our final strategy for dealing with conflicts during package install (as opposed to Dynamo startup) - that we may reevaluate std.lib's location (first or last), additionally feedback from UX may lead us to rethink this anyway.

Given that, do you think the position is difficult to change after this PR or it will be pretty straightforward? - I was wondering if the logic could be abstracted into a position field somewhere we could set.

It is a small change. It happens in two places right now. But I have been thinking that it might be possible to remove that logic all together now from the PackageLoader with these changes. I think I will take a look at that.

@sm6srw sm6srw added the PTAL Please Take A Look 👀 label Apr 23, 2021
@sm6srw sm6srw merged commit 0c7e3bf into DynamoDS:master Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants