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

Pass the Preferences as part of the Initializacion configuration #2933

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

jesusalvino
Copy link
Contributor

Purpose

As part of the task https://jira.autodesk.com/browse/DYN-5816 the DynamoModel that the class RevitDynamoModel inherits, needs to pass the explicit Preferences as part of its initialization configuration.

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
  • Snapshot of UI changes, if any.

Reviewers

@QilongTang
@reddyashish

@@ -607,7 +607,8 @@ private static RevitDynamoModel InitializeCoreModel(DynamoRevitCommandData comma
AuthProvider = new RevitOAuth2Provider(new DispatcherSynchronizationContext(Dispatcher.CurrentDispatcher)),
ExternalCommandData = commandData,
UpdateManager = revitUpdateManager,
ProcessMode = isAutomationMode ? TaskProcessMode.Synchronous : TaskProcessMode.Asynchronous
ProcessMode = isAutomationMode ? TaskProcessMode.Synchronous : TaskProcessMode.Asynchronous,
Preferences = PreferenceSettings.Instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to pass the PathResolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to pass the PathResolver?

yes as the previously been doing in the startconfiguration, now We assigning it to the PathManager in the DynamoModel ctor, the only update is need to pass the Preferences.

@QilongTang QilongTang requested a review from Mikhinja August 10, 2023 15:15
@QilongTang
Copy link
Contributor

@Mikhinja PTAL, please refer to DynamoDS/Dynamo#14118

@QilongTang
Copy link
Contributor

hi @Mikhinja This is a pairing change to work with the RC2.19.0 build

@mjkkirschner
Copy link
Member

Does this cause the multiple instance bug shown on slack?

@@ -276,7 +276,8 @@ protected override void StartDynamo(TestSessionConfiguration testConfig)
SchedulerThread = new TestSchedulerThread(),
PackageManagerAddress = "https://www.dynamopackages.com",
ExternalCommandData = commandData,
ProcessMode = RevitTaskProcessMode
ProcessMode = RevitTaskProcessMode,
Preferences = PreferenceSettings.Instance
Copy link
Member

Choose a reason for hiding this comment

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

have you tried running all RTF tests locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have you tried running all RTF tests locally?

I have tried but even when I put the paths and the proper tests it got me errors, I have tested with some Dynamo flows under the DynamoRevit pointing to DynamoCore 4.8

@jesusalvino
Copy link
Contributor Author

Does this cause the multiple instance bug shown on slack?

I Don't think so since I have tested a branch before this change, but looks like it's an issue that only pass for my local setup

@Mikhinja Mikhinja merged commit c47f49d into DynamoDS:master Aug 22, 2023
Mikhinja pushed a commit that referenced this pull request Aug 29, 2023
@QilongTang QilongTang mentioned this pull request Sep 20, 2023
5 tasks
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.

4 participants