Generate shorter expression animation strings from ExpressionNode #4112
Conversation
|
Had to re-trigger PR build with org name change |
Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionNodes/ExpressionNode.cs
Outdated
Show resolved
Hide resolved
|
FYI @Sergio0694 as he did the previous PR called out in the linked issue. I feel like there was another place or two we used this same pattern, do we need to update things there as well? |
There was a problem hiding this comment.
Looks good, thanks @arcadiogarcia! 🙌
Just left a small suggestion performance-wise, but the change makes sense! 😄
@michael-hawker To answer your question about potentially using this elsewhere, that's doable but since those cases aren't using expression animations there's less value in doing so, plus we'd have to slightly refactor code to enable this as we don't have a loop like this where we can just iteratively increment a local counter to produce all inputs. I think we can do that in a follow up PR if we wanted to make the behavior consistent, but I don't think that should be considered blocking for this PR 🙂
Microsoft.Toolkit.Uwp.UI.Animations/Expressions/ExpressionNodes/ExpressionNode.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good, thanks again @arcadiogarcia for the idea and the fix! 😄
…s/ExpressionNode.cs Co-authored-by: Rosario Pulella <[email protected]>
Sergio0694
force-pushed
the
user/arcadiog/shorterExpressionStrings
branch
from
c5fc961
to
50008c0
Compare
|
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
michael-hawker
added
the
next preview ✈️
|
Looks like the CI failed, but I don't have the right permissions to see the log. Is there a way to request access to that ADO tenant? |
|
@arcadiogarcia it should be public if you use incognito? DevOps is weird with credentials for some reason. 🙁 Should have checked and merged this in earlier, meant it to be part of our preview. Looks like it got confused again, we've had a lot of troubles with the SmokeTests picking up the wrong version still. FYI @azchohfi Which causes a problem as this was moved in the older bits (but it's good that it fails in this case to let us know). I'll re-run the smoke tests. |
|
I just ran the CI clean, not sure why it's not reporting properly on GitHub: |
Fixes
#4049
PR Type
What kind of change does this PR introduce?
Bugfix
What is the current behavior?
When ExpressionNodes are converted to expression animation strings, guids are used as the unique name for each parameter. While this guarantees uniqueness and works fine in theory, it makes it relatively easy to hit the 1000-character limit imposed on animation strings, which makes it impossible to express more complex animations.
What is the new behavior?
Instead of generating a random guid, we use sequential alphabetic identifiers, e.g.
A,B,C..., Z, AA , AB..., ZZ, AAA, AAB....
which also guarantees uniqueness and scales to provide any number of identifiers (well beyond what will fit in the 1000 character animation string), while outputting more compact strings. Virtually all animations willl have <25 parameters, so this changes the average parameter name length from 36 characters to 1, which is a big deal if your animation needs to repeat the same parameters in a few places.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information