Skip to content

Fixed expression nodes created with custom param names incorrectly clearing during internal cleanup #4756

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

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Fixed expression nodes created with custom param names incorrectly clearing during internal cleanup #4756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e045893
Fixed an issue where expression nodes created with custom param names…
Arlodotexe Sep 6, 2022
556fbff
Merge branch 'main' into fix/issue4595
Arlodotexe Sep 8, 2022
5762b30
Merge branch 'main' into fix/issue4595
Arlodotexe Oct 10, 2022
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
4 changes: 2 additions & 2 deletions Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,11 +1199,11 @@ internal static T Function<T>(ExpressionNodeType nodeType, params ExpressionNode
where T : ExpressionNode
{
T newNode = ExpressionNode.CreateExpressionNode<T>();
newNode.NodeType = nodeType;

(newNode as ExpressionNode).NodeType = nodeType;
foreach (var param in expressionFunctionParams)
{
(newNode as ExpressionNode).Children.Add(param);
newNode.Children.Add(param);
}

return newNode;
Expand Down
50 changes: 28 additions & 22 deletions Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionNodes/ExpressionNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Animations.Expressions
public abstract class ExpressionNode : IDisposable
{
private List<ReferenceInfo> _objRefList = null;
private Dictionary<CompositionObject, string> _compObjToParamNameMap = null;
private Dictionary<CompositionObject, string> _compObjToNodeNameMap = null;
private Dictionary<string, object> _constParamMap = new Dictionary<string, object>(StringComparer.CurrentCultureIgnoreCase);

/// <summary>
Expand Down Expand Up @@ -144,7 +144,7 @@ public void SetMatrix4x4Parameter(string parameterName, Matrix4x4 value)
public void Dispose()
{
_objRefList = null;
_compObjToParamNameMap = null;
this._compObjToNodeNameMap = null;
_constParamMap = null;
Subchannels = null;
PropertyName = null;
Expand Down Expand Up @@ -227,16 +227,16 @@ internal static T CreateValueKeyword<T>(ValueKeywordKind keywordKind)
{
T node = CreateExpressionNode<T>();

(node as ExpressionNode).ParamName = null;
node.ParamName = null;

switch (keywordKind)
{
case ValueKeywordKind.CurrentValue:
(node as ExpressionNode).NodeType = ExpressionNodeType.CurrentValueProperty;
node.NodeType = ExpressionNodeType.CurrentValueProperty;
break;

case ValueKeywordKind.StartingValue:
(node as ExpressionNode).NodeType = ExpressionNodeType.StartingValueProperty;
node.NodeType = ExpressionNodeType.StartingValueProperty;
break;

default:
Expand All @@ -263,11 +263,11 @@ internal string ToExpressionString()
/// <summary>
/// Clears the reference information.
/// </summary>
/// <exception cref="System.Exception">Reference and paramName can't both be null</exception>
/// <exception cref="Exception">Reference and paramName can't both be null</exception>
internal void ClearReferenceInfo()
{
_objRefList = null;
ParamName = null;
this.NodeName = null;
foreach (var child in Children)
{
child.ClearReferenceInfo();
Expand All @@ -277,7 +277,7 @@ internal void ClearReferenceInfo()
/// <summary>
/// Ensures the reference information.
/// </summary>
/// <exception cref="System.Exception">Reference and paramName can't both be null</exception>
/// <exception cref="Exception">Reference and paramName can't both be null</exception>
internal void EnsureReferenceInfo()
{
if (_objRefList == null)
Expand All @@ -290,42 +290,42 @@ internal void EnsureReferenceInfo()
HashSet<CompositionObject> compObjects = new HashSet<CompositionObject>();
foreach (var refNode in referenceNodes)
{
if ((refNode.Reference != null) && (refNode.GetReferenceParamString() == null))
if ((refNode.Reference != null) && (refNode.GetReferenceNodeString() == null))
{
compObjects.Add(refNode.Reference);
}
}

// Create a map to store the generated paramNames for each CompObj
_compObjToParamNameMap = new Dictionary<CompositionObject, string>();
this._compObjToNodeNameMap = new Dictionary<CompositionObject, string>();
var paramCount = 0u;
foreach (var compObj in compObjects)
{
string paramName = CreateUniqueParamNameFromIndex(paramCount++);
string nodeName = !string.IsNullOrWhiteSpace(ParamName) ? ParamName : CreateUniqueNodeNameFromIndex(paramCount++);

_compObjToParamNameMap.Add(compObj, paramName);
this._compObjToNodeNameMap.Add(compObj, nodeName);
}

// Go through all reference nodes again to create our full list of referenceInfo. This time, if
// the param name is null, look it up from our new map and store it.
_objRefList = new List<ReferenceInfo>();
foreach (var refNode in referenceNodes)
{
string paramName = refNode.GetReferenceParamString();
string nodeName = refNode.GetReferenceNodeString();

if ((refNode.Reference == null) && (paramName == null))
if ((refNode.Reference == null) && (nodeName == null))
{
// This can't happen - if the ref is null it must be because it's a named param
throw new Exception("Reference and paramName can't both be null");
throw new Exception($"{nameof(refNode.Reference)} and {nameof(nodeName)} can't both be null");
}

if (paramName == null)
if (nodeName == null)
{
paramName = _compObjToParamNameMap[refNode.Reference];
nodeName = this._compObjToNodeNameMap[refNode.Reference];
}

_objRefList.Add(new ReferenceInfo(paramName, refNode.Reference));
refNode.ParamName = paramName;
_objRefList.Add(new ReferenceInfo(nodeName, refNode.Reference));
refNode.NodeName = nodeName;
}
}

Expand All @@ -335,7 +335,7 @@ internal void EnsureReferenceInfo()
// important in this context as the only critical property to maintain is to have
// a unique mapping to each input value to the resulting sequence of letters.
[SkipLocalsInit]
static unsafe string CreateUniqueParamNameFromIndex(uint i)
static unsafe string CreateUniqueNodeNameFromIndex(uint i)
{
const int alphabetLength = 'Z' - 'A' + 1;

Expand Down Expand Up @@ -592,7 +592,7 @@ private string ToExpressionStringInternal()
throw new Exception("References cannot have children");
}

ret = (this as ReferenceNode).GetReferenceParamString();
ret = (this as ReferenceNode).GetReferenceNodeString();
}
else if (NodeType == ExpressionNodeType.ReferenceProperty)
{
Expand Down Expand Up @@ -700,11 +700,17 @@ public ReferenceInfo(string paramName, CompositionObject compObj)
internal List<ExpressionNode> Children { get; set; } = new List<ExpressionNode>();

/// <summary>
/// Gets or sets the name of the parameter.
/// Gets or sets the user-defined name of the parameter.
/// </summary>
/// <value>The name of the parameter.</value>
internal string ParamName { get; set; }

/// <summary>
/// Gets or sets the unique name for the expression node. Can be user-defined or generated.
/// </summary>
/// <value>The name of the parameter.</value>
internal string NodeName { get; set; }

/// <summary>
/// Gets or sets the expression animation.
/// </summary>
Expand Down
4 changes: 2 additions & 2 deletions Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ReferenceNodes/ReferenceNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,15 @@ public Matrix4x4Node GetMatrix4x4Property(string propertyName)
/// Gets the reference parameter string.
/// </summary>
/// <returns>System.String.</returns>
internal string GetReferenceParamString()
internal string GetReferenceNodeString()
{
if (NodeType == ExpressionNodeType.TargetReference)
{
return "this.target";
}
else
{
return ParamName;
return NodeName;
}
}

Expand Down