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 for v0.55 collections diffing #775

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

TimLariviere
Copy link
Member

@TimLariviere TimLariviere commented Jul 10, 2020

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
Copy link

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
Copy link

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

@TimLariviere
Copy link
Member Author

@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
Copy link

vshapenko commented Jul 13, 2020 via email

@TimLariviere
Copy link
Member Author

TimLariviere commented Jul 13, 2020

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
Copy link

@TimLariviere , looks like we badly need your help

@TimLariviere TimLariviere merged commit 58aec20 into fabulous-dev:master Jul 15, 2020
@TimLariviere TimLariviere deleted the fix-collections-diff branch July 15, 2020 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants