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.13k stars 122 forks source link

Implemented taking only the latest attribute for a property #981

Closed twop closed 1 year ago

twop commented 1 year ago

Context

In WidgetBuilders we can easily have duplicate attributes like Label("aha!").color("red").color("blue") Diffing algorithm already uses stable sort for diffing attributes Thus we need to leverage that fact

In particular we might have a bug in this context prev: Label("aha!").color("red").color("blue") next: Label("aha!").color("red")

After the diffing the resulting color should be "red", but previously it was interpreted as "removed color property" instead

Solution

I implemented SkipRepeatingScalars.skip functions that skips color("red") fromcolor("red").color("blue")` Effectively taking only the latest attribute for a particular property.

I used in ScalarChangesEnumerator but only for actual diff. It Is possible that it should be used for "AllRemoved" and "AllAdded" but it is unlikely to cause any problems (possible a field will be set twice for example)

TimLariviere commented 1 year ago

Would an if condition be better for the hot path than a while loop given 99.9% of the attributes won't be duplicated? I guess the compiler already optimizes this kind of stuff.

e.g.

if scalars.[pos + 1].Key <> key then
    pos // Quick exit
else
    resultingIndex <- resultingIndex + 1

    // Peek even further ahead
    while (...)

    resultingIndex
twop commented 1 year ago

Would an if condition be better for the hot path than a while loop given 99.9% of the attributes won't be duplicated? I guess the compiler already optimizes this kind of stuff.

e.g.

if scalars.[pos + 1].Key <> key then
    pos // Quick exit
else
    resultingIndex <- resultingIndex + 1

    // Peek even further ahead
    while (...)

    resultingIndex

It is possible, although we need to check out of bounds as well, regardless I couldn't measure any overhead with skipping (relative to what we have today, e.g. no overhead at all). So I think we should probably merge it as is and tinker about more performant implementation on a side "for fun" :)