DYN-6470 Improve Dynamo PreferenceSettings.Instance singleton behavior. #14658
Conversation
src/DynamoCore/Models/DynamoModel.cs
Outdated
| @@ -492,6 +492,9 @@ protected virtual void ShutDownCore(bool shutdownHost) | |||
| Dispose(); | |||
| PreferenceSettings.SaveInternal(pathManager.PreferenceFilePath); | |||
|
|
|||
| //Remove reference on shutdown | |||
| PreferenceSettings.dynamoModelRuntimePreferenceSettings = null; | |||
There was a problem hiding this comment.
Not sure if we need to do this but may be good due to not being able to properly discuss the DynamoModel on shutdown.
There was a problem hiding this comment.
I think this is likely safe on DynamoModel if you ensure that dynamoModelRuntimePreferenceSettings is always reset to something valid on startup.
Can you add a test for that?
| internal readonly static Lazy<PreferenceSettings> | ||
| lazy = new Lazy<PreferenceSettings> | ||
| (() => PreferenceSettings.Load(PathManager.Instance.PreferenceFilePath)); | ||
|
|
||
| [XmlIgnore] | ||
| public static PreferenceSettings Instance { get { return lazy.Value; } } | ||
| public static PreferenceSettings Instance { get { return dynamoModelRuntimePreferenceSettings ?? lazy.Value; } } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I can give it a shot. I believe one a lazy value is set it doesn't recall the into function over and over.
There was a problem hiding this comment.
I think the main issue will be all the special handling we do for testing but let's see
There was a problem hiding this comment.
Also we currently don't let anyone just set the DynamoModel.Prefence it is readonly
There was a problem hiding this comment.
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;} }
| @@ -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")] | |||
There was a problem hiding this comment.
I just meant 4.0 as an example, maybe just mention a future release of Dynamo instead of committing to a specific release.
|
One failing test is expected. Merging now |
Purpose
The purpose of the PR is to improve the singleton behavior of the
PreferenceSettings.Instance. Currently it is very easy to utilize the Instance with the assumption that it is connected to theDynamoModel.PrefenceSettings(previously how you would access runtime Preferences via API) and will stay in sync. This is not the case and they can diverge in state and data can be lost regardless of how often the Preference are saved to disk by the API user. This PR improve the Instance method synchronization by connecting directly to the DynamoModel.PreferenceSettings when it is available. There are still scenarios where the two access points can get out of sync but this should change cover most scenarios until we can unify the access points to a single reference object.Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@QilongTang @mjkkirschner @BogdanZavu
FYIs
@twastvedt