Core - Fix data model and event condition bugs found by the tests (#910)

* Core - Fix removing dynamic children and checking which properties are in use

- DataModel.RemoveDynamicChild compared the value of each child with the
  child that was passed in, so it never removed anything.
- IsPropertyInUse with includeChildren matched paths by their start, so
  a path to "Integers" made "Integer" count as in use. Children are now
  matched on what follows a dot.

* Core - Fix data model paths around saving, removed children and stored types

- The constructors saved the path before setting it up, while it was
  still invalid, so a new path stored nothing and didn't follow its data
  model leaving and returning to the store until it was saved.
- Removing the dynamic child a path points to left the path valid and
  reading a default value, it now becomes invalid like when the child
  was never there.
- The stored type of a path is now also looked up in all loaded
  assemblies, Type.GetType only found it in Artemis.Core and the .NET
  libraries, not in plugins or SkiaSharp.

* Core - Only let event conditions react to what happens after they start watching

- A trigger from before the condition watched its event, for example
  long before the profile was activated, counted as new and played the
  element once. Triggers now count from the moment the condition starts
  watching its path.
- A condition on a plain value treated the first value it saw as a
  change and played the element on the first frame. The first value is
  now taken as the starting value.

src/Artemis.Core.Tests/Conditions/EventConditionTests.cs

@@ -79,16 +79,24 @@ public class EventConditionTests : RenderTestBase
     }
 
     [Fact]
-    public void Play_EventTriggeredBeforeTheConditionExisted_StillPlaysTheLayer()
+    public void Play_EventTriggeredBeforeTheConditionExisted_DoesNotPlayTheLayer()
     {
         DataModel.Event.Trigger();
         (Layer layer, _) = AddLayer();
 
         using RenderFrame frame = Render(layer.Profile);
 
-        // Pinned as-is: the condition starts out having seen no triggers at all, so a trigger from before it was created
-        // (for example long before the profile was activated) still counts as new and plays the layer once. Looks like
-        // the condition should only react to triggers that happen after it started watching.
+        ShouldBeOff(frame);
+    }
+
+    [Fact]
+    public void Play_EventTriggeredAfterTheConditionExisted_PlaysTheLayer_EvenBeforeTheFirstFrame()
+    {
+        (Layer layer, _) = AddLayer();
+        DataModel.Event.Trigger();
+
+        using RenderFrame frame = Render(layer.Profile);
+
         ShouldBeOn(frame);
     }
 
@@ -418,15 +426,14 @@ public class EventConditionTests : RenderTestBase
     }
 
     [Fact]
-    public void PathToAValue_PlaysOnTheFirstFrame()
+    public void PathToAValue_DoesNotPlayOnTheFirstFrame()
     {
         (Layer layer, _) = AddLayer(eventPath: "Integer");
 
         using RenderFrame frame = Render(layer.Profile);
 
-        // Pinned as-is: the last seen value starts out empty, so the first frame always sees the value as changed (0 is
-        // not empty) and plays the layer even though nothing changed.
-        ShouldBeOn(frame);
+        // The first frame only learns the starting value, nothing changed yet
+        ShouldBeOff(frame);
     }
 
     [Fact]

src/Artemis.Core.Tests/DataModels/DataModelPathTests.cs

@@ -340,7 +340,7 @@ public class DataModelPathTests : IDisposable
     }
 
     [Fact]
-    public void RemovingTheDynamicChild_LeavesThePathValid()
+    public void RemovingTheDynamicChild_MakesThePathInvalid()
     {
         DataModel.AddDynamicChild("Extra", 5);
         using DataModelPath path = new(DataModel, "Extra");
@@ -349,25 +349,34 @@ public class DataModelPathTests : IDisposable
 
         DataModel.RemoveDynamicChildByKey("Extra");
 
-        // Pinned as-is: the segment only invalidates the path when the value of the removed child is the data model the
-        // segment looks in, rather than when the child it points to is removed. So the path stays valid and reads the
-        // default value instead of becoming invalid.
-        path.IsValid.ShouldBeTrue();
-        path.GetValue().ShouldBe(0);
-        invalidated.ShouldBe(0);
+        path.IsValid.ShouldBeFalse();
+        path.GetValue().ShouldBeNull();
+        invalidated.ShouldBe(1);
     }
 
     [Fact]
-    public void RemovingADynamicDataModel_LeavesThePathThroughItValid()
+    public void RemovingTheDynamicChild_AndAddingItAgain_MakesThePathValidAgain()
+    {
+        DataModel.AddDynamicChild("Extra", 5);
+        using DataModelPath path = new(DataModel, "Extra");
+        DataModel.RemoveDynamicChildByKey("Extra");
+
+        DataModel.AddDynamicChild("Extra", 6);
+
+        path.IsValid.ShouldBeTrue();
+        path.GetValue().ShouldBe(6);
+    }
+
+    [Fact]
+    public void RemovingADynamicDataModel_MakesThePathThroughItInvalid()
     {
         DataModel.AddDynamicChild("Extra", new TestChildDataModel {Integer = 4}, "Extra data");
         using DataModelPath path = new(DataModel, "Extra.Integer");
 
         DataModel.RemoveDynamicChildByKey("Extra");
 
-        // Pinned as-is, see RemovingTheDynamicChild_LeavesThePathValid
-        path.IsValid.ShouldBeTrue();
-        path.GetValue().ShouldBe(0);
+        path.IsValid.ShouldBeFalse();
+        path.GetValue().ShouldBeNull();
     }
 
     [Fact]
@@ -450,20 +459,17 @@ public class DataModelPathTests : IDisposable
     }
 
     [Fact]
-    public void NewPath_DoesNotFollowTheStoreUntilItIsSaved()
+    public void NewPath_FollowsTheStoreWithoutBeingSaved()
     {
         using DataModelPath path = new(DataModel, "Integer");
 
-        // Pinned as-is: the constructors call Save before the path is set up, while it is still invalid, so nothing is
-        // saved and the entity has no data model ID. The path only reacts to its data model leaving and returning to the
-        // store once Save is called. Looks like Save should be called after Initialize.
-        path.Entity.DataModelId.ShouldBeNull();
-        path.Entity.Path.ShouldBe("");
+        path.Entity.DataModelId.ShouldBe(_module.Id);
+        path.Entity.Path.ShouldBe("Integer");
 
         RemoveFromStore();
 
-        path.IsValid.ShouldBeTrue();
-        path.Target.ShouldBeSameAs(DataModel);
+        path.IsValid.ShouldBeFalse();
+        path.Target.ShouldBeNull();
     }
 
     [Fact]
@@ -540,7 +546,7 @@ public class DataModelPathTests : IDisposable
     }
 
     [Fact]
-    public void InvalidPath_CannotFallBackToATypeOutsideTheCoreLibraries()
+    public void InvalidPath_FallsBackToATypeOutsideTheCoreLibraries()
     {
         using DataModelPath valid = new(DataModel, "Enum");
         valid.Save();
@@ -548,9 +554,15 @@ public class DataModelPathTests : IDisposable
 
         using DataModelPath path = new(new DataModelPathEntity {Path = "Enum", DataModelId = "Missing-12345678", Type = valid.Entity.Type});
 
-        // Pinned as-is: the type is saved by its full name without the assembly, which Type.GetType can only find in
-        // Artemis.Core and the .NET libraries, so types from plugins (and SkiaSharp, like SKColor) can't be found again.
-        path.GetPropertyType().ShouldBeNull();
+        path.GetPropertyType().ShouldBe(typeof(TestEnum));
+    }
+
+    [Fact]
+    public void InvalidPath_FallsBackToASkiaSharpType()
+    {
+        using DataModelPath path = new(new DataModelPathEntity {Path = "Color", DataModelId = "Missing-12345678", Type = typeof(SKColor).FullName});
+
+        path.GetPropertyType().ShouldBe(typeof(SKColor));
     }
 
     [Fact]

src/Artemis.Core.Tests/DataModels/DataModelTests.cs

@@ -184,7 +184,7 @@ public class DataModelTests : IDisposable
     }
 
     [Fact]
-    public void RemovingADynamicChild_DoesNotRemoveIt()
+    public void RemovingADynamicChild_RemovesIt()
     {
         DynamicChild<int> child = DataModel.AddDynamicChild("Extra", 5);
         int removed = 0;
@@ -192,11 +192,8 @@ public class DataModelTests : IDisposable
 
         DataModel.RemoveDynamicChild(child);
 
-        // Pinned as-is: RemoveDynamicChild compares the value of each child (BaseValue) with the child that was passed in,
-        // rather than the child itself, so it only removes a child whose value is the child object. Looks like it should
-        // compare the children themselves; RemoveDynamicChildByKey works.
-        DataModel.DynamicChildren.ShouldContainKey("Extra");
-        removed.ShouldBe(0);
+        DataModel.DynamicChildren.ShouldNotContainKey("Extra");
+        removed.ShouldBe(1);
     }
 
     [Fact]
@@ -304,14 +301,14 @@ public class DataModelTests : IDisposable
     }
 
     [Fact]
-    public void IncludingChildren_AlsoMatchesPropertiesThatStartWithTheSameName()
+    public void IncludingChildren_DoesNotMatchPropertiesThatStartWithTheSameName()
     {
         using DataModelPath path = new(DataModel, "Integers");
 
-        // Pinned as-is: children are matched by checking whether a path starts with the given text, so "Integer" counts as
-        // in use because of a path to "Integers", a different property. Looks like it should check for "Integer."
-        _module.IsPropertyInUse("Integer", true).ShouldBeTrue();
+        // Integers is a different property than Integer, not a child of it
+        _module.IsPropertyInUse("Integer", true).ShouldBeFalse();
         _module.IsPropertyInUse("Integer", false).ShouldBeFalse();
+        _module.IsPropertyInUse("Integers", true).ShouldBeTrue();
     }
 
     [Fact]

src/Artemis.Core/Models/Profile/Conditions/EventCondition.cs

@@ -17,8 +17,10 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
     private NodeScript<bool> _script;
     private DefaultNode _startNode;
     private DataModelPath? _eventPath;
-    private DateTime _lastProcessedTrigger;
+    // Only what happens after the condition starts watching its path counts, see StartWatching
+    private DateTime _lastProcessedTrigger = DateTime.Now;
     private object? _lastProcessedValue;
+    private bool _hasProcessedValue;
     private EventOverlapMode _overlapMode;
     private EventToggleOffMode _toggleOffMode;
     private EventTriggerMode _triggerMode;
@@ -64,7 +66,11 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
     public DataModelPath? EventPath
     {
         get => _eventPath;
-        set => SetAndNotify(ref _eventPath, value);
+        set
+        {
+            if (SetAndNotify(ref _eventPath, value))
+                StartWatching();
+        }
     }
 
     /// <summary>
@@ -243,6 +249,15 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
             Script.AddNode(newStartNode);
     }
 
+    private void StartWatching()
+    {
+        // Triggers and values from before the condition watched the path don't count, otherwise for example an event that
+        // triggered long before the profile was activated would still play the element once
+        _lastProcessedTrigger = DateTime.Now;
+        _lastProcessedValue = null;
+        _hasProcessedValue = false;
+    }
+
     private bool Evaluate()
     {
         if (EventPath == null)
@@ -258,6 +273,14 @@ public class EventCondition : CorePropertyChanged, INodeScriptCondition
         }
         else if (_startNode is EventConditionValueChangedStartNode valueChangedNode)
         {
+            // The first value seen is where the value starts, not a change
+            if (!_hasProcessedValue)
+            {
+                _lastProcessedValue = value;
+                _hasProcessedValue = true;
+                return false;
+            }
+
             if (Equals(value, _lastProcessedValue))
                 return false;
 

src/Artemis.Core/Models/Profile/DataModel/DataModelPath.cs

@@ -29,8 +29,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -47,8 +47,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -67,8 +67,8 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent
 
         _segments = [];
 
-        Save();
         Initialize();
+        Save();
         SubscribeToDataModelStore();
     }
 
@@ -165,11 +165,20 @@ public class DataModelPath : IStorageModel, IDisposable, IPluginFeatureDependent
         
         // Fall back to stored type
         if (!string.IsNullOrWhiteSpace(Entity.Type))
-            return Type.GetType(Entity.Type);
+            return FindType(Entity.Type);
 
         return null;
     }
 
+    private static Type? FindType(string fullName)
+    {
+        // The type is stored without its assembly, Type.GetType only finds it in Artemis.Core and the .NET libraries. Types
+        // of plugins and other libraries (like SKColor) are found by looking through the loaded assemblies
+        return Type.GetType(fullName) ?? AppDomain.CurrentDomain.GetAssemblies()
+            .Select(assembly => assembly.GetType(fullName))
+            .FirstOrDefault(type => type != null);
+    }
+
     /// <summary>
     ///     Gets the property description of the property this path points to
     /// </summary>

src/Artemis.Core/Models/Profile/DataModel/DataModelPathSegment.cs

@@ -286,8 +286,16 @@ public class DataModelPathSegment : IDisposable
 
     private void DynamicChildOnDynamicChildRemoved(object? sender, DynamicDataModelChildEventArgs e)
     {
-        if (e.DynamicChild.BaseValue == _dynamicDataModel)
+        // Like when a child is added, initialize the path again so it no longer points to the removed child
+        if (e.Key == Identifier)
+        {
             DataModelPath.Invalidate();
+            DataModelPath.Initialize();
+        }
+        else if (e.DynamicChild.BaseValue == _dynamicDataModel)
+        {
+            DataModelPath.Invalidate();
+        }
     }
 
     /// <inheritdoc />

src/Artemis.Core/Plugins/Modules/DataModel.cs

@@ -289,7 +289,7 @@ public abstract class DataModel
     public void RemoveDynamicChild(DynamicChild dynamicChild)
     {
         if (dynamicChild == null) throw new ArgumentNullException(nameof(dynamicChild));
-        List<string> keys = _dynamicChildren.Where(kvp => kvp.Value.BaseValue == dynamicChild).Select(kvp => kvp.Key).ToList();
+        List<string> keys = _dynamicChildren.Where(kvp => kvp.Value == dynamicChild).Select(kvp => kvp.Key).ToList();
         foreach (string key in keys)
         {
             _dynamicChildren.Remove(key);
@@ -335,8 +335,10 @@ public abstract class DataModel
     {
         lock (_activePaths)
         {
+            // A child's path continues after a dot, matching on the text alone would also match a property that merely starts
+            // with the same name (Integer and Integers)
             return includeChildren
-                ? _activePathsHashSet.Any(p => p.StartsWith(path, PathsStringComparison))
+                ? _activePathsHashSet.Any(p => string.Equals(p, path, PathsStringComparison) || p.StartsWith(path + ".", PathsStringComparison))
                 : _activePathsHashSet.Contains(path);
         }
     }