microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.34k stars 677 forks source link

ReorderThemeTransition doesn't work #1503

Open stmoy opened 4 years ago

stmoy commented 4 years ago

Describe the bug

(Originally filed by @DRAirey1 here: https://github.com/microsoft/uwp-experiences/issues/52)

There are no examples anywhere in the internet that show ReorderThemeTransition working. Tried to wire it using this example:

<Setter Property="ItemContainerTransitions">
    <Setter.Value>
        <TransitionCollection>
            <AddDeleteThemeTransition/>
            <ContentThemeTransition/>
            <ReorderThemeTransition/>
            <EntranceThemeTransition IsStaggeringEnabled="False"/>
        </TransitionCollection>
    </Setter.Value>
</Setter>

The insert and delete animation work just fine, but when I execute: observableCollection.Move(5, 2); from a button, I get a reset of the list (it blinks and you see the list with the new order: no animation effects). iOS has this feature, so why doesn't UWP?

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'

Expected behavior

Screenshots

Version Info

NuGet package version:

Windows 10 version Saw the problem?
Insider Build (xxxxx) Yes
May 2019 Update (18362) Yes
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop
Mobile
Xbox
Surface Hub
IoT

Additional context

Doc bug also included: https://github.com/MicrosoftDocs/winrt-api/issues/1278

codendone commented 4 years ago

The problem here is that observableCollection.Move() does not send good notifications to its observers -- it sends CollectionChange_Reset. This causes the list control (particularly ModernCollectionBasePanel::TransitionContextManager::NotifyOfItemsChanging) to mark the entire collection as having changed, which means it can't ask ReorderThemeTransition to do intelligent animations.

ReorderThemeTransition does work in the special case it supports, but I agree that the obvious and common case of observableCollection.Move() should be supported. In order to do this, either the notifications sent from Move() need to be changed, or ModernCollectionBasePanel needs to be changed to not trust the message and do something different (such as manual diffing to compute better change information, though this would likely be a performance-expensive operation).

ranjeshj commented 4 years ago

Just FYI. This is essentially what IKeyIndexMapping in ItemsRepeater solves. Being able to do a collection reset and still be able to figure out what changed based on the unique keys. https://docs.microsoft.com/en-us/uwp/api/microsoft.ui.xaml.controls.itemsrepeater.itemssource?view=winui-2.3

DRAirey1 commented 4 years ago

ReorderThemeTransition does work in the special case it supports, but I agree that the obvious and common case of observableCollection.Move() should be supported. In order to do this, either the notifications sent from Move() need to be changed, or ModernCollectionBasePanel needs to be changed to not trust the message and do something different (such as manual diffing to compute better change information, though this would likely be a performance-expensive operation).

Are you saying that if I implement my own System.Collections.Specialized.INotifyCollectionChanged class, and support the NotifyCollectionChangedAction.Move action, then I'll see the animations as I would expect (e.g. animated movement of the item from position 2 to position 5)?

codendone commented 4 years ago

Unfortunately, I don't believe so. What the XAML code here gets is just one of the limited values of Windows.Foundation.Collections.CollectionChange, which doesn't provide info that there was a move. Without this, ReorderThemeTransition is only told there was a Reset and therefore does nothing. For the most part, ReorderThemeTransition currently requires the ListView/GridView to be directly responsible for doing a reorder/sort, so that those controls can instruct it how to run its animation.

All of this certainly limits the utility of ReorderThemeTransition, and we should fix this.

DRAirey1 commented 4 years ago

@codendone Is there any workaround?

jevansaks commented 4 years ago

@DRAirey1 I understand your frustration, but please be respectful. I believe you're trying to ask "is there any workaround?" and so I've edited your comment as such.

DRAirey1 commented 4 years ago

What was the path this took to production? There must be some code, somewhere to test the library if it made it into production. Is not possible to find the code they used to test this animation?

SavoySchuler commented 4 years ago

@DRAirey1 Thanks for filing this and providing the additional context. It helps us better understand what's required on our end to resolve this issue. 🙂 Please be kind while being constructive and your messages won't require an additional coating of empathy. Unfortunately, I've had to edit your follow up response as well but I believe I have accurately captured your core question. Please let us know if I have missed anything.

@jevansaks was right to reduce the stressful tone presented. We understand this exasperation comes from a real place of pain or frustration. I apologize that we have made you feel bad by presenting this issue, but we can work toward a resolution without making others feel bad as well. I encourage you to consider approaching this issue patience because we would be excited to continue working with you in a more respectful tone.

@codendone acknowledged that this code did serve its original purpose - as limited as that was - and that we owe the original implementation a refresh to logically extend its functionality. The next step, as I understand it, would be to start discussing how we implement this or if there are any other considerations we should take into account at this time.

I am not the expert for this area, so I will leave it to @codendone and @ranjeshj's capable hands unless I am otherwise needed. Thanks for working together to resolve this issue, everyone! 🙂

ranjeshj commented 4 years ago

@DRAirey1 ListView provides certain behavior which is quite hard to change or customize. I would suggest looking at ItemsRepeater sorting/filtering sample, which might be what you are looking for. You can provide a unique id for each item in the collection and let the control figure out the diff and animate. You can also provide a custom animator and create animations to suite your own needs. You can find some documentation on ItemsRepeater here.

DRAirey1 commented 4 years ago

Yeah, Move doesn't work when an observable collection is bound to this control: "Not Implemented"

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

kmgallahan commented 1 year ago

Keep alive

JJBrychell commented 1 year ago

One of two things need to happen here. Either we need to support the broader range of collection changed types (e.g. Move) or we need to convert the "move" to a remove/insert and recognize it in the control. Either of the options would be beyond the scope of a bug fix and so converting this to a feature proposal.