DYN-6259 Fix Handling preferences bearing in mind the hosts #14427
Conversation
avidit
changed the title
src/DynamoCore/Models/DynamoModel.cs
Outdated
| @@ -1056,7 +1056,7 @@ internal PathManager CreatePathManager(IStartConfiguration config) | |||
| internal PreferenceSettings CreatePreferences(IStartConfiguration config) | |||
| { | |||
| PreferenceSettings preferences = null; | |||
| if (!config.StartInTestMode) | |||
| if (config.Preferences != null) | |||
There was a problem hiding this comment.
in the D4R case, this will be false and we will go to the else block for loading the preferences?
There was a problem hiding this comment.
yes, at that point we already have a PathManager pointing to the proper paths so we will create the preferences from the expected file.
There was a problem hiding this comment.
Don't we want to do the same in the StartInTestMode also? What happens in test mode then?
There was a problem hiding this comment.
For test mode, I think it depends on if Preferences was passed through start config? Can you confirm that from unit tests?
There was a problem hiding this comment.
@reddyashish @QilongTang This is transparent for the TestMode. Yes, each test is responsible to pass its preferences or not.
There was a problem hiding this comment.
This seems like a foundational problem with this set of preference singleton changes - now the tests are not testing what users are experiencing and test authors ( at least me) have no idea how the different test fixtures are interacting with preferences.
There was a problem hiding this comment.
It looks like we can just revert back to the previous code at https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Models/DynamoModel.cs#L681 to simply be PreferenceSettings = (PreferenceSettings)CreateOrLoadPreferences(config.Preferences);
I guess to avoid confusion from @mjkkirschner , let's document what is is the difference before and after PreferenceSetting singleton change
There was a problem hiding this comment.
It looks like we can just revert back to the previous code at https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Models/DynamoModel.cs#L681 to simply be
PreferenceSettings = (PreferenceSettings)CreateOrLoadPreferences(config.Preferences);I guess to avoid confusion from @mjkkirschner , let's document what is is the difference before and after PreferenceSetting singleton change
Done.
|
How will you test this change? |
I think we can unit test this. Passing in preferences v.s. rely on path manager to load a settings file. I was going to propose adding a unit test after this PR. |
* Fix Handling preferences bearing in mind the hosts * Creating the Preferences on demand --------- Co-authored-by: Jesus Alfredo Alviño <[email protected]>
…14431) * Fix Handling preferences bearing in mind the hosts * Creating the Preferences on demand --------- Co-authored-by: jesusalvino <[email protected]> Co-authored-by: Jesus Alfredo Alviño <[email protected]>
Purpose
Fixing the bug https://jira.autodesk.com/browse/DYN-6259 handling the creation/assignation of the Preferences in the Dynamic Model ctor bearing in mind the PathManager info.
A host doesn't need to pass any Preferences instance because it will be created in the Dynamo side considering the PathManager has the proper paths to do it.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@QilongTang
@reddyashish