fabulous-dev / Fabulous

Declarative UI framework for cross-platform mobile & desktop apps, using MVU and F# functional programming
https://fabulous.dev
Apache License 2.0
1.15k stars 122 forks source link

Fix for v0.55 collections diffing #775

Closed TimLariviere closed 4 years ago

TimLariviere commented 4 years ago

Reverted to a diffing that returns all changes necessary to move from state A to state B.

The main reason we did what we did before is that Xamarin.Forms needs we change ObservableCollection step by step, which was making the "old indices" really hard to calculate (on insert, all indices after are shifted by +1; on delete, shifted by -1).

So instead a new function is responsible to compensate for that, it's easier to understand what's going on.

vshapenko commented 4 years ago

I think we can make the key attribute for collections mandatory. Something like items:(ViewElement*string) list. That will give some pain on update, but will let us have more proper collection diff.

Happypig375 commented 4 years ago

A Map<string, ViewElement> instead could be more efficient.

TimLariviere commented 4 years ago

@vshapenko Yeah, maybe. Today, I think it wouldn't make sense API-wise though. There would be 2 ways to declare keys and it would make no sense from the developer's point of view.

View.CollectionView([
    "Item1", View.Label(key = "Item2") // What's supposed to happen here?
])

Also I'm not a fan of having "dangling" values in the middle of controls. I find it harder to understand. It feels loosely coupled in my opinion. For example ListViewGrouped, without knowing the actual signature it's hard to understand what is the purpose of the different parts.

View.ListViewGrouped([
    "A", View.TextCell("A"), [
        View.TextCell("Alphabet")
    ]
])

As far as I know React doesn't require a key for collections, but strongly encourage it. SwiftUI took a different approach by requiring the collection of data to have some kind of identifiable part, either by having the data type to implement a Identifiable protocol, or passing a lambda to the key as part of the List signature.

List(landmarkData, id: \.id) { landmark in  //\.id here means fun data -> data.id
    LandmarkRow(landmark: landmark)
}

Which could be translated to

let items = [ { Id = 1; SomeData = "Hello" }; { Id = 2; SomeData = "World" } ]

View.CollectionView(
    itemsSource = items,
    itemId = (fun item -> item.Id),
    itemTemplate = (fun item ->
        View.Label(item.SomeData)
    )
)

Maybe it can be something we add to the new architecture as part of #738?

vshapenko commented 4 years ago

I think yes. FYI, we have our own implementations of collection views. PM me if you need details.

пн, 13 июл. 2020 г., 12:52 Timothé Larivière notifications@github.com:

@vshapenko https://github.com/vshapenko Yeah, maybe. Today, I think it wouldn't make sense API-wise though. There would be 2 ways to declare keys and it would make no sense from the developer's point of view.

View.CollectionView([ "Item1", View.Label(key = "Item2") // What's supposed to happen here?])

Also I'm not a fan of having "dangling" values in the middle of controls. I find it harder to understand. It feels loosely coupled in my opinion. For example ListViewGrouped, without knowing the actual signature it's hard to understand what is the purpose of the different parts.

View.ListViewGrouped([ "A", View.TextCell("A"), [ View.TextCell("Alphabet") ]])

As far as I know React doesn't require a key for collections, but strongly encourage it. SwiftUI took a different approach by requiring the collection of data to have some kind of identifiable part, either by having the data type to implement a Identifiable protocol, or passing a lambda to the key as part of the List signature.

List(landmarkData, id: .id) { landmark in //.id here means fun data -> data.id LandmarkRow(landmark: landmark) }

Which could be translated to

let items = [ { Id = 1; SomeData = "Hello" }; { Id = 2; SomeData = "World" } ]

View.CollectionView( itemsSource = items, itemId = (fun item -> item.Id), itemTemplate = (fun item -> View.Label(item.SomeData) ) )

Maybe it can be something we add to the new architecture as part of #738 https://github.com/fsprojects/Fabulous/issues/738?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsprojects/Fabulous/pull/775#issuecomment-657455175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEKNDOVMKT6SRAFDHWSWKCLR3LKOZANCNFSM4OWRQUWQ .

TimLariviere commented 4 years ago

I added a new constraint to prevent reuse in some conditions. A previous element with a key that is not found in the new list won't be reused by a new element.

Even though nothing prevents us to reuse the abandoned keyed element, on some platforms, an animation might play on some properties when changed. This is the case for iOS Button. When the text changes, it plays a fade-in/fade-out animation (#308). Same with Android Button. When clicked, a ripple animation plays.

But due to Fabulous reusing those controls "aggressively" without the developer being able to say anything, it resulted in undesired animations playing.

Now, by default Fabulous will reuse an unkeyed control, but if the developer notices an undesired animation due to control reuse, he can disable it by setting a key.

vshapenko commented 4 years ago

@TimLariviere , looks like we badly need your help