Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

`ConfigurationManager` only adds, doesn't remove from `Dictionary`-based items

Open tig opened this issue 2 years ago • 0 comments

When I coded this, I failed to consider what happens if an item is removed from a dictionary.

To repro:

  • Run UI Catalog
  • While running, add a new Theme named "UI Catalog Theme 2" to ./.tui/config.json
  • Note Themes menu now also shows "UI Catalog Theme 2"
  • Now remove that added Theme and save the config file

Expected Behavior: "UI Catalog Theme 2" disappears

Actual Behavior: "UI Catalog Theme 2" is still in the Themes dictionary.

The problem is in ConfigProperty.cs where Update does a deep memberwise copy:

	internal object? UpdateValueFrom (object source)
	{
		if (source == null) {
			return PropertyValue;
		}

		var ut = Nullable.GetUnderlyingType (PropertyInfo!.PropertyType);
		if (source.GetType () != PropertyInfo!.PropertyType && (ut != null && source.GetType () != ut)) {
			throw new ArgumentException ($"The source object ({PropertyInfo!.DeclaringType}.{PropertyInfo!.Name}) is not of type {PropertyInfo!.PropertyType}.");
		}
		if (PropertyValue != null && source != null) {
			PropertyValue = ConfigurationManager.DeepMemberwiseCopy (source, PropertyValue);
		} else {
			PropertyValue = source;
		}

		return PropertyValue;
	}

DeepMemberwiseCopy is really an "append" operation:

	/// <summary>
	/// System.Text.Json does not support copying a deserialized object to an existing instance.
	/// To work around this, we implement a 'deep, memberwise copy' method. 
	/// </summary>
	/// <remarks>
	/// TOOD: When System.Text.Json implements `PopulateObject` revisit
	///	https://github.com/dotnet/corefx/issues/37627
	/// </remarks>
	/// <param name="source"></param>
	/// <param name="destination"></param>
	/// <returns><paramref name="destination"/> updated from <paramref name="source"/></returns>
	internal static object? DeepMemberwiseCopy (object? source, object? destination)
	{
		if (destination == null) {
			throw new ArgumentNullException (nameof (destination));
		}

		if (source == null) {
			return null!;
		}

		if (source.GetType () == typeof (SettingsScope)) {
			return ((SettingsScope)destination).Update ((SettingsScope)source);
		}
		if (source.GetType () == typeof (ThemeScope)) {
			return ((ThemeScope)destination).Update ((ThemeScope)source);
		}
		if (source.GetType () == typeof (AppScope)) {
			return ((AppScope)destination).Update ((AppScope)source);
		}

		// If value type, just use copy constructor.
		if (source.GetType ().IsValueType || source.GetType () == typeof (string)) {
			return source;
		}

		// Dictionary
		if (source.GetType ().IsGenericType && source.GetType ().GetGenericTypeDefinition ().IsAssignableFrom (typeof (Dictionary<,>))) {
			foreach (var srcKey in ((IDictionary)source).Keys) {
				if (srcKey is string) {

				}
				if (((IDictionary)destination).Contains (srcKey))
					((IDictionary)destination) [srcKey] = DeepMemberwiseCopy (((IDictionary)source) [srcKey], ((IDictionary)destination) [srcKey]);
				else {
					((IDictionary)destination).Add (srcKey, ((IDictionary)source) [srcKey]);
				}
			}
			return destination;
		}

		// ALl other object types
		var sourceProps = source?.GetType ().GetProperties ().Where (x => x.CanRead).ToList ();
		var destProps = destination?.GetType ().GetProperties ().Where (x => x.CanWrite).ToList ()!;
		foreach (var (sourceProp, destProp) in
			from sourceProp in sourceProps
			where destProps.Any (x => x.Name == sourceProp.Name)
			let destProp = destProps.First (x => x.Name == sourceProp.Name)
			where destProp.CanWrite
			select (sourceProp, destProp)) {

			var sourceVal = sourceProp.GetValue (source);
			var destVal = destProp.GetValue (destination);
			if (sourceVal != null) {
				if (destVal != null) {
					// Recurse
					destProp.SetValue (destination, DeepMemberwiseCopy (sourceVal, destVal));
				} else {
					destProp.SetValue (destination, sourceVal);
				}
			}
		}
		return destination!;
	}

It really should be a "merge" operation, but my brain hurts when thinking how to make that work.

tig avatar Nov 02 '23 16:11 tig