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

[Feature] Introduce more compact style for ExpressionNode strings #4049

Closed
arcadiogarcia opened this issue May 27, 2021 · 4 comments
Closed

Comments

@arcadiogarcia
Copy link
Member

arcadiogarcia commented May 27, 2021

Describe the problem this feature would solve

Currently ExpressionNodes are resolved to strings that look like this:

(((InteractionTracker_1.NaturalRestingPosition.X > InteractionTracker_1.MinPosition.X)  && CompositionPropertySet_2.s.X  ...

This verbose style is great for being able to read the generated expressions but it also causes the expression size to grow faster, which can be problematic for complex animations that reach the 1000 character limit. Being able to read the generated expression is not that useful for consumers of the toolkit since the string is not exposed publicly, the only place I have seen it surface is when the Windows APIs throw an exception because of its length.

Describe the solution

It would be nice if it automatically generated (or at least, we could choose to generate) compact strings that are functionally equivalent but shorter, e.g

(((a.NaturalRestingPosition.X > a.MinPosition.X)  && b.s.X  ...

Looks like in the recent PR #4014 these identifiers were changed to guids, which are also relatively long but don't refer directly to class names either, so adopting shorter identifiers shouldn't have negative side effects.

As far as I can tell, it would be a matter of replacing this line https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/529624cf1af8f034bed9c4f9927d0e0d62f1a6db/Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionNodes/ExpressionNode.cs#L288

with logic that returns unique, sequential, minimal-length ids (e.g. 'a', 'b', ... 'z', 'aa', 'ab' ...)

Describe alternatives you've considered

The current design of the animation API, which exposes public extension methods that take the ExpressionNode but keeps ExpressionNode.ToExpressionString() as an internal method, makes it impossible to manipulate/transform the generated animation strings from outside the toolkit. If ToExpressionString() were to be made public, developers would be able to transform the strings if they want/need to.

@arcadiogarcia arcadiogarcia added the feature request 📬 A request for new changes to improve functionality label May 27, 2021
@ghost
Copy link

ghost commented May 27, 2021

Hello, 'arcadiogarcia! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

@Kyaa-dost
Copy link
Contributor

@arcadiogarcia Thanks for highlighting your interest in this area. Opening this for further discussion to gain more input from the community,

@michael-hawker
Copy link
Member

@arcadiogarcia your PR addressed this right, we can close the issue?

@arcadiogarcia
Copy link
Member Author

@michael-hawker Sure, closing it!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants