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

DYN-6470 Improve Dynamo PreferenceSettings.Instance singleton behavior. #14658

Merged
merged 5 commits into from
Dec 4, 2023
Merged

DYN-6470 Improve Dynamo PreferenceSettings.Instance singleton behavior. #14658

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99061fc
Add passthrough to DynamoModel.PreferenceSettings when available
saintentropy Nov 30, 2023
639f7bd
remove reference
saintentropy Nov 30, 2023
6363956
Don't need to reset the reference
saintentropy Nov 30, 2023
73b82c9
Simplify and invert
saintentropy Dec 1, 2023
e0ac7cd
update test
saintentropy Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DynamoCore/Configuration/PreferenceSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ internal readonly static Lazy<PreferenceSettings>
(() => PreferenceSettings.Load(PathManager.Instance.PreferenceFilePath));

[XmlIgnore]
public static PreferenceSettings Instance { get { return lazy.Value; } }
public static PreferenceSettings Instance { get; internal set; } = lazy.Value;

Copy link
Contributor

Choose a reason for hiding this comment

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

so now Dynamo supports local instances of Preferences, a singleton instance which can be loaded from disk, or set to incoming local Preferences instances during the DynamoModel construction (not all the time though).
We need a followup to untangle all of this before it gets out of hand.

At the moment I would not be able to predict what instance of the Preferences class would be used at a given spot in Dynamo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes throughly needs to be cleaned up. I would consider this a bandaid to resolve the current out of sync behavior. Fundamentally the issue (and to your question above @QilongTang ) is that from an API consumer perspective (like the Player) if you goal is to make changes to Preferences while Dynamo is open and you want those changes persisted you need to interact with the runtime Presence that is held by DynamoModel. It will aways keep the state for the application and overwrite its state to disk when Dynamo Shuts down. If I grab Instance ,make changes and also know to save things it doesn't matter at all. DynamoModel.PreferenceSettings wins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand the behavior today on first time start-up (ie no existing preference XML)
SplashScreen calls PreferenceSettings.Instance
Instance creates a new PreferenceSettings object.
DynamoModel start is passed the new PreferenceSettings object.
DynamoModel performs additional initialization on the new PrefenceSettings object
Eventually DynamoModel shutdown saves the PreferenceSettings to disk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the theme of this PR is looking at all the times Dynamo Player is using PreferenceSettings.Instance AKA DynamoPreferences singleton we were expecting to have the Runtime Preferences object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only other way I can think to work this way is make the PrefenceSettings always update if the disk is changed and always write to disk any time any properties are updated. That would also could be fragile due to concurrency issues... Other option is to actually get to a real singleton.

Copy link
Contributor

@QilongTang QilongTang Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@aparajit-pratap You are for sure on spot, that was exactly what our team was supposed to change! But this might be a much heavier change than this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can give it a shot. I believe one a lazy value is set it doesn't recall the into function over and over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the main issue will be all the special handling we do for testing but let's see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we currently don't let anyone just set the DynamoModel.Prefence it is readonly

Copy link
Contributor

Choose a reason for hiding this comment

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

Which is why we can keep it for the time being if we're worried about API breaks and simply reroute it to PreferenceSettings.Instance in its getter:

[Obsolete("this will be removed in 4.0")]
public readonly PreferenceSettings PreferenceSettings { get {return PreferenceSettings.Instance;} }

private string numberFormat;
private string lastUpdateDownloadPath;
Expand Down
8 changes: 5 additions & 3 deletions src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,8 @@ internal static string AppVersion
/// <summary>
/// Preference settings for this instance of Dynamo.
/// </summary>
public readonly PreferenceSettings PreferenceSettings;
[Obsolete("this will be removed in 4.0")]
Copy link
Contributor

@aparajit-pratap aparajit-pratap Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I just meant 4.0 as an example, maybe just mention a future release of Dynamo instead of committing to a specific release.

public PreferenceSettings PreferenceSettings => PreferenceSettings.Instance;

/// <summary>
/// Node Factory, used for creating and intantiating loaded Dynamo nodes.
Expand Down Expand Up @@ -691,7 +692,8 @@ protected DynamoModel(IStartConfiguration config)

OnRequestUpdateLoadBarStatus(new SplashScreenLoadEventArgs(Resources.SplashScreenInitPreferencesSettings, 30));

PreferenceSettings = (PreferenceSettings)CreateOrLoadPreferences(config.Preferences);
PreferenceSettings.Instance = (PreferenceSettings)CreateOrLoadPreferences(config.Preferences);

if (PreferenceSettings != null)
{
SetUICulture(PreferenceSettings.Locale);
Expand Down Expand Up @@ -779,7 +781,7 @@ protected DynamoModel(IStartConfiguration config)
if (migrator != null)
{
var isFirstRun = PreferenceSettings.IsFirstRun;
PreferenceSettings = migrator.PreferenceSettings;
PreferenceSettings.Instance = migrator.PreferenceSettings;

// Preserve the preference settings for IsFirstRun as this needs to be set
// only by UsageReportingManager
Expand Down
Loading