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

ConfigurationManager: Applied and Updated events are clumsy #2871

Open
tig opened this issue Oct 2, 2023 · 0 comments
Open

ConfigurationManager: Applied and Updated events are clumsy #2871

tig opened this issue Oct 2, 2023 · 0 comments
Labels
enhancement v2 For discussions, issues, etc... relavant for v2

Comments

@tig
Copy link
Collaborator

tig commented Oct 2, 2023

Currently ConfigurationManager uses an event pattern so that classes that want to know if the config was updated (or been applied) can update their state. This lets apps update in-real-time whenever config changes.

However, this pattern means classes like LineCanvas have to subscribe to an event:

		public LineCanvas()
		{
			ConfigurationManager.Applied += ConfigurationManager_Applied;
		}

And THIS means they also have to implement IDisposable to correctly unsubscribe. This is what led to a big memory leak reported in #2870. IMO, if the disposable pattern can be avoided, it should be as it relies on developers remembering to do things.

I propose that in addition to having these events classes can implement an ISupportConfigChanges interface:

/// <summary>
/// <para>
/// Classes should implement this interface on any class that should be notified when the configuration changes.
/// This is an alternative to using the <see cref="ConfigurationManager.Updated"/> and
/// <see cref="ConfigurationManager.Applied"/> events.
/// <para>
/// When the configuration has changed, the <see cref="ConfigurationManager"/> will call the <see cref="OnUpdated"/>
/// method. When the configuration is being applied, the <see cref="OnApplied"/> method will be called.
/// 
/// </para>
/// </summary>
public interface ISupportConfigChanges {

	/// <summary>
	/// Called when the configuration has been updated.
	/// </summary>
	public void OnUpdated ();

	/// <summary>
	/// Called when the configuration is being applied.
	/// </summary>
	public void OnApplied ();
}

ConfigurationManager.OnApplied() currently looks like this:

	/// <summary>
	/// Called when an updated configuration has been applied to the  
	/// application. Fires the <see cref="Applied"/> event.
	/// </summary>
	public static void OnApplied ()
	{
		Debug.WriteLine ($"ConfigurationManager.OnApplied()");
		Applied?.Invoke (null, new ConfigurationManagerEventArgs ());
	}

We'll use reflection after the call to Applied?.Invoke() to find all instances of classes implementing ISupportConfigChanges and call the OnApplied() method on them.

I may be missing a better pattern for this, so i'd love to hear from others on this concept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement v2 For discussions, issues, etc... relavant for v2
Projects
Status: No status
Status: 🆕 Not Triaged
Development

No branches or pull requests

1 participant