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

Fix InlineDictionary Behavior for Single Item Reassignment #17370

Merged
merged 6 commits into from
Oct 31, 2024

Conversation

lindexi
Copy link
Contributor

@lindexi lindexi commented Oct 28, 2024

What does the pull request do?

This PR addresses an issue with the InlineDictionary when handling reassignment of a single item.

What is the current behavior?

Current Behavior:

  • When the Set method was called, it ignored the overwrite parameter if the dictionary contained only one item.
  • This caused the Set method to behave like the Add method, leading to unexpected behavior in composition animations.
  • Specifically, the second composition animation would not play because it could not replace the first animation. Instead, it was added after the first animation, preventing it from being executed.

See

public void OnSetAnimatedValue<T>(CompositionProperty<T> prop, ref T field, TimeSpan committedAt, IAnimationInstance animation) where T : struct
{
if (_owner.IsActive && _animations.TryGetValue(prop, out var oldAnimation))
oldAnimation.Animation.Deactivate();
_animations[prop] = new ServerObjectAnimationInstance<T>(this, animation, prop);
animation.Initialize(committedAt, ExpressionVariant.Create(field), prop);
if(_owner.IsActive)
animation.Activate();
OnSetDirectValue(prop);
}

What is the updated/expected behavior with this PR?

Updated Behavior:

  • The InlineDictionary now respects the overwrite parameter even when it contains only one item.
  • This ensures that the second composition animation can overwrite the first one, allowing it to play correctly.

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #17368

Previous Behavior:

- When the `Set` method was called, it ignored the `overwrite` parameter if the dictionary contained only one item.
- This caused the `Set` method to behave like the `Add` method, leading to unexpected behavior in composition animations.
- Specifically, the second composition animation would not play because it could not replace the first animation. Instead, it was added after the first animation, preventing it from being executed.

Updated Behavior:

- The `InlineDictionary` now respects the `overwrite` parameter even when it contains only one item.
- This ensures that the second composition animation can overwrite the first one, allowing it to play correctly.
@lindexi lindexi force-pushed the t/lindexi/InlineDictionary branch from 2b879dd to 55e84fc Compare October 28, 2024 12:00
@lindexi lindexi marked this pull request as ready for review October 28, 2024 12:00
@MrJul
Copy link
Member

MrJul commented Oct 29, 2024

Thank you! Could you please add a matching unit test to the InlineDictionaryTests? There isn't a lot of test coverage for this class currently, but we should at least cover fixed bugs :)

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052850-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052852-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added bug backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Oct 30, 2024
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test! A couple of minor points and it should be good to go.

src/Avalonia.Base/Utilities/SmallDictionary.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Utilities/SmallDictionary.cs Outdated Show resolved Hide resolved
@lindexi lindexi requested a review from MrJul October 31, 2024 02:30
@lindexi
Copy link
Contributor Author

lindexi commented Oct 31, 2024

@MrJul Thank you for your review. And I updated my code.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052893-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added this pull request to the merge queue Oct 31, 2024
Merged via the queue into AvaloniaUI:master with commit 5398f7a Oct 31, 2024
10 checks passed
maxkatz6 pushed a commit that referenced this pull request Nov 14, 2024
* Fix InlineDictionary Behavior for Single Item Reassignment

Previous Behavior:

- When the `Set` method was called, it ignored the `overwrite` parameter if the dictionary contained only one item.
- This caused the `Set` method to behave like the `Add` method, leading to unexpected behavior in composition animations.
- Specifically, the second composition animation would not play because it could not replace the first animation. Instead, it was added after the first animation, preventing it from being executed.

Updated Behavior:

- The `InlineDictionary` now respects the `overwrite` parameter even when it contains only one item.
- This ensures that the second composition animation can overwrite the first one, allowing it to play correctly.

* Append unit test for InlineDictionary

* keep the `(TKey)_data` in a variable and reuse it below line 90 so we avoid a double cast

* Rename the test method

* Remove the unnecessary assignment code.
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with Animation Playback in Avalonia Composition
4 participants