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

Add dropdown to package preferences dialog for package paths for download #11747

Merged
merged 26 commits into from
Jun 22, 2021

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Jun 10, 2021

Purpose

DYN-3723: Add a dropdown to list all package paths where packages can be downloaded.

WIP:

  • Avoid hardcoding Dynamo version for AppData path
  • Filter out paths to file names from custom package paths to display in dropdown
  • Add property changed event for changing selected path for analytics and in order to update label and tooltip in preferences dialog
  • Use selected package path to tell package manager client to download packages to that path
  • Add tooltip displaying full path for long paths cut off from dropdown display
  • Add unit test to verify downloading package to path chosen by simulating selection from dropdown
  • UI Automation tests

image

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 (WIP)
  • 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

FYIs

@QilongTang
@Amoursol
@Jingyi-Wen

@aparajit-pratap aparajit-pratap changed the title Add dropdown to package preferences dialog for package paths for package download Add dropdown to package preferences dialog for package paths for download Jun 10, 2021
@mjkkirschner
Copy link
Member

Screen Shot 2021-06-10 at 4 02 51 PM

@aparajit-pratap relative to the design - it looks like we're missing this label.

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jun 10, 2021

Screen Shot 2021-06-10 at 4 02 51 PM

@aparajit-pratap relative to the design - it looks like we're missing this label.

Yes, I had seen this but I didn't quite understand why there was one label underneath another unless the Node and Package Paths label is the title for the page overall. @Jingyi-Wen?

@mjkkirschner
Copy link
Member

mjkkirschner commented Jun 10, 2021

@aparajit-pratap node and package paths is the expander/title for the section. It includes both the dropdown to set a specific download path, as well as the search paths - each of those sub sections have a label.

Then there is another expander/section for the installed packages.

@aparajit-pratap
Copy link
Contributor Author

@aparajit-pratap node and package paths is the expander/title for the section. It includes both the dropdown to set a specific download path, as well as the search paths - each of those sub sections have a label.

Then there is another expander/section for the installed packages.

Ok, I got it now. That clears my confusion. I was wondering where the Package/Library Search Paths section disappeared in the mockups. I thought that was an expander by itself, didn't realize it was part of the Node and Package Paths expander.

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jun 10, 2021

@Jingyi-Wen for the New Package Download Directory label, I've used the same style as the Default Python Engine label, however, I notice that the font size of these labels is bigger than the respective expander labels. I feel that the expander label being a top-level label should have a higher font size.
image

@Jingyi-Wen
Copy link

@Jingyi-Wen for the New Package Download Directory label, I've used the same style as the Default Python Engine label, however, I notice that the font size of these labels is bigger than the respective expander labels. I feel that the expander label being a top-level label should have a higher font size.
image

@aparajit-pratap Great catch! We were using the same font size but different thickness(bold vs semi-bold vs regular) to visually indicate levels in the design. We might need to adjust the thickness for item labels and expander labels

@mjkkirschner
Copy link
Member

@Jingyi-Wen @aparajit-pratap - another thought here - when a really long package download path is selected it appears like it's getting cut off - which makes sense since the dialog width is fixed.

Can we do something like if you hover over the path the tooltip shows the full path?

I think addressing @QilongTang wanted to address making the dialog resizable later after 2.12.

@aparajit-pratap
Copy link
Contributor Author

@Jingyi-Wen @aparajit-pratap - another thought here - when a really long package download path is selected it appears like it's getting cut off - which makes sense since the dialog width is fixed.

Can we do something like if you hover over the path the tooltip shows the full path?

I think addressing @QilongTang wanted to address making the dialog resizable later after 2.12.

Yeah, I was thinking the same thing about having the tooltip. I'll try to add it.

@Jingyi-Wen
Copy link

@Jingyi-Wen @aparajit-pratap - another thought here - when a really long package download path is selected it appears like it's getting cut off - which makes sense since the dialog width is fixed.
Can we do something like if you hover over the path the tooltip shows the full path?
I think addressing @QilongTang wanted to address making the dialog resizable later after 2.12.

Yeah, I was thinking the same thing about having the tooltip. I'll try to add it.

Thank you! That sounds good. I thought about this but missed it in mockup, adding it now

@mjkkirschner
Copy link
Member

@aparajit-pratap - this looks really solid, just a few comments besides the ones inline.

  • looks like some conflict from a recent pr.
  • unit tests for the new property getters, and preference setting would be good I think.

<Label>SOME CONTENT</Label>
<Label Content="{x:Static p:Resources.PreferencesViewPackageDownloadDirectory}"
Padding="0,5,5,5"
FontSize="13"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this font is supposed to be re-used in other parts of the UI, maybe we can store it in the styles xaml ?

Copy link
Member

Choose a reason for hiding this comment

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

I have added a style for this now, we can clean it up later though after these all get merged.

@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Jun 21, 2021
Copy link
Contributor

@pinzart90 pinzart90 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

a few small comments - probably not blockers.

<Label>SOME CONTENT</Label>
<Label Content="{x:Static p:Resources.PreferencesViewPackageDownloadDirectory}"
Padding="0,5,5,5"
FontSize="13"
Copy link
Member

Choose a reason for hiding this comment

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

I have added a style for this now, we can clean it up later though after these all get merged.

src/DynamoPackages/PackageLoader.cs Show resolved Hide resolved

internal void SetPackagesDownloadDirectory(string downloadDirectory, string userDataFolder)
{
defaultPackagesDirectoryIndex = packagesDirectories.IndexOf(
Copy link
Member

Choose a reason for hiding this comment

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

what if this returns -1, is that acceptable?

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 was being assigned -1 even before, the core logic is the same as before so I didn't think much about it.

{
var folder = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);
var dynamoVersion = FileVersionInfo.GetVersionInfo(Assembly.GetExecutingAssembly().Location);
var appDataFolder = Path.Combine(Path.Combine(folder, "Dynamo", "Dynamo Core"),
Copy link
Member

Choose a reason for hiding this comment

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

are these strings possibly already defined somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These probably should be defined as constants somewhere as they are hardcoded in several places throughout the codebase. I'm not doing it here as it could clutter this PR.

test/Libraries/PackageManagerTests/PackageLoaderTests.cs Outdated Show resolved Hide resolved
@aparajit-pratap aparajit-pratap merged commit b069b24 into DynamoDS:master Jun 22, 2021
@aparajit-pratap aparajit-pratap deleted the dyn-3723 branch June 22, 2021 21:12
@aparajit-pratap aparajit-pratap restored the dyn-3723 branch June 22, 2021 21:14
@aparajit-pratap aparajit-pratap deleted the dyn-3723 branch June 22, 2021 21:16
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.

5 participants