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.

Sign up for GitHub

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

Jump to bottom

move package search paths to preferences settings #11762

Merged
merged 25 commits into from
Jun 22, 2021
Merged

move package search paths to preferences settings #11762

merged 25 commits into from
Jun 22, 2021

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Jun 15, 2021 •
edited
Loading

Purpose

This PR moves the node and package dialog to the package prefs settings dialog.

  • PackagePathView and PackagePathViewModel are reused but heavily modified.
  • Since the new design does not (need or) support selection of the paths, the selectedIndex property is obsoleted and disconnected from the UI - it now serves no purpose.
  • I've tried to reuse some styles from the modern dynamo dictionary
  • adds some colors to the dictionary
  • renames RemoveStyleButtonStyle template to FlatIconButtonStyle.
  • The commands that are used to bind to the buttons are unchanged, but their implementations no longer use selectedIndex instead relying on the bound path of the current button - that path is converted to an index by looking up the path in the RootLocations property of the PackagePathViewModel.
  • adds package manager to preference settings tabs so expander state is preserved.
  • packagePathView is no longer a window - now a user control.
  • modifies the expander styles to make the header text size 14 and medium (though maybe it should be full bold?)
  • package paths are committed on close of the prefs window, and packages re reloaded.

Feedback requested

  • ⚠️ one thing I'm not sure about - is that the packages are reloaded and paths saved whenever the preferences window is closed, this means that if you open the preferences to change some random setting having nothing to do with package paths, then close the prefs window, you will see some notifications about packages that were ignored because they were already loaded - I think we should fix this issue of expected warnings in a followup task -it's an issue today as well, but this pr will make it more frequent. I'm not sure if this is tracked - but I do know @sm6srw brought it up.

TODO

  • on close - actually save the data to the preferences - and load packages
  • unit tests
  • update existing tests
  • ui automation tests
  • fix styling
    • better icons (arrows and slim X)
    • add paths button right align
    • disabled buttons/command should be greyed out as in design - buttons are not clickable but look the same currently.
    • scroll viewer
    • textblock font - non bold
    • expander header font size should be 14pt medium bold
    • paths that are disabled should be greyed out
    • use new icons that are symmetric about their axes to borders.

This screenshot shows what happens if we turn on the hidden DisableCustomPackagePaths flag.

Screen Shot 2021-06-17 at 3 23 51 PM

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

@mjkkirschner
add new color for dark backgrounds
2e542ca 
remove unusued setter
add property for packagePathsViewModel to prefsViewModel
construct packpathsVMwhen creating prefsVM
add item template, button, and itemcontrol first draft
@mjkkirschner
rename style
a37047b 
rename property for viewmodel
make internal commands work for ints or string paths
temporarily fix for making packagepathsview a usercontrol - will remove this code when done
use new control in prefs
modify packagePathView
deal with the fact that dataContext for packagePathView will be late bound and will change from null to PackagePathViewModel.

this currently will add and remove paths correctly - other commands are not modified yet,

no data is saved to prefs xml on dynamo close - and package reload is not implemented on prefs close yet.
@mjkkirschner
get rid of selected index
91c87c7 
convert paths to indices before calling internal command implementations
add namespace for packages
hook up command bindings for each button to pass path as parameter - bind to packagepathViewModel
remove old code from packagePathView for selection we no longer need
@mjkkirschner
missed
9952bf2
@mjkkirschner
Merge branch 'master' of https://github.com/DynamoDS/Dynamo into sear…
560d207 
…chpathslist
@mjkkirschner
tweak some margins
11aefcb
@mjkkirschner
new icons
5b59f9e 
scroll expander
@mjkkirschner
expander bigger font
ea02e3e 
scroll bar bigger area
@mjkkirschner
change all expander headers to 14pt font, medium
4e7925c
@mjkkirschner
add isdisabled helper
768e9f5 
remove unused static var
save packages and load them on prefs close
use datatrigger to grey out disabled paths
@mjkkirschner
missed
5530b7c
@mjkkirschner
fix tooltips
cea771d 
fix tests
@mjkkirschner
add new icons
f0c149e
@mjkkirschner
fix bug
a58d6e8 
add new tests
@mjkkirschner mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Jun 17, 2021
@mjkkirschner mjkkirschner requested a review from sm6srw June 17, 2021 20:57
mjkkirschner
mjkkirschner commented Jun 17, 2021
View reviewed changes
src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml
<TextBlock
Text="{Binding}"
FontSize="14"
FontWeight="Medium">
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this will probably do something once we change all the fonts to Artifakt.

@mjkkirschner mjkkirschner requested a review from pinzart90 June 18, 2021 14:53
@mjkkirschner
obsolete commands for raising old dialogs.
fdfaca3 
obsolete dialog raise events and request methods
remove event subscription
pinzart90
pinzart90 approved these changes Jun 21, 2021
View reviewed changes
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

@aparajit-pratap
Copy link
Contributor

@mjkkirschner are the packages reloaded even when there is no change to the package paths? What about when some other tab other than the "package manager" tab of the preferences dialog is opened?

@mjkkirschner
Copy link
Member Author

@mjkkirschner are the packages reloaded even when there is no change to the package paths? What about when some other tab other than the "package manager" tab of the preferences dialog is opened?

yes, and yes - I've mentioned this in the description of this pr.
I'd like to get this merged without addressing it at the moment, I think it might be a bit tricky.

@mjkkirschner mjkkirschner merged commit bdd63b6 into DynamoDS:master Jun 22, 2021
@mjkkirschner mjkkirschner deleted the searchpathslist branch June 22, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinzart90 pinzart90 pinzart90 approved these changes

@sm6srw sm6srw sm6srw approved these changes

@aparajit-pratap aparajit-pratap aparajit-pratap approved these changes

Assignees
No one assigned
Labels
PTAL Please Take A Look 👀
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mjkkirschner @aparajit-pratap @sm6srw @pinzart90