Rename + disallow keyed elements reuse for unkeyed controls

Fabulous.XamarinForms/src/Fabulous.XamarinForms.Core/Collections.fs

@@ -43,13 +43,15 @@ module Collections =
         | ValueSome _, ValueNone -> ClearCollection
         | ValueSome _, ValueSome coll when (coll = null || coll.Length = 0) -> ClearCollection
         | _, ValueSome coll ->
-            // Separate the previous elements into 3 lists
+            // Separate the previous elements into 4 lists
             // The ones whose instances have been reused (dependsOn)
-            // The ones whose keys have been reused
+            // The ones whose keys have been reused and should be updated
+            // The ones whose keys have not been reused and should be discarded
             // The rest which can be reused by any other element
             let identicalElements = Dictionary<'T, int>()
             let keyedElements = Dictionary<string, int * 'T>()
             let reusableElements = ResizeArray<int * 'T>()
+            let discardedElements = ResizeArray<int>()
             if prevCollOpt.IsSome && prevCollOpt.Value.Length > 0 then
                 for prevIndex in 0 .. prevCollOpt.Value.Length - 1 do
                     let prevChild = prevCollOpt.Value.[prevIndex]
@@ -66,8 +68,10 @@ module Collections =
                         match keyOf prevChild with
                         | ValueSome key when canReuseChildOf key ->
                             keyedElements.Add(key, (prevIndex, prevChild))
-                        | _ ->
+                        | ValueNone ->
                             reusableElements.Add((prevIndex, prevChild))
+                        | ValueSome _ ->
+                            discardedElements.Add(prevIndex)
 
             let operations =
                 [ for i in 0 .. coll.Length - 1 do
@@ -103,6 +107,11 @@ module Collections =
 
                             | None ->
                                 yield Insert (i, newChild)
+
+                  // If we have discarded elements, delete them
+                  if discardedElements.Count > 0 then
+                      for prevIndex in discardedElements do
+                          yield Delete prevIndex
 
                   // If we still have old elements that were not reused, delete them
                   if reusableElements.Count > 0 then
@@ -117,71 +126,70 @@ module Collections =
     /// Reduces the operations of the DiffResult to be applicable to an ObservableCollection.
     /// 
     /// diff returns all the operations to move from List A to List B.
-    /// Except with ObservableCollection, we're forced to apply the changes one after the other, changing the indexes
+    /// Except with ObservableCollection, we're forced to apply the changes one after the other, changing the indices
     /// So this algorithm compensates this offsetting
-    let reduceDiff (prevCollLength: int) (diffResult: DiffResult<'T>) : DiffResult<'T> =
+    let adaptDiffForObservableCollection (prevCollLength: int) (diffResult: DiffResult<'T>) : DiffResult<'T> =
         match diffResult with
         | NoChange -> NoChange
         | ClearCollection -> ClearCollection
         | Operations operations ->
-            let prevIndexes = Array.init prevCollLength id
+            let prevIndices = Array.init prevCollLength id
+
+            // Shift all old indices by 1 (down the list) on insert after the inserted position
+            let shiftForInsert index =
+                for i in 0 .. prevIndices.Length - 1 do
+                    if prevIndices.[i] >= index then
+                        prevIndices.[i] <- prevIndices.[i] + 1
+
+            // Shift all old indices by -1 (up the list) on delete after the deleted position
+            let shiftForDelete originalIndexInPrevColl prevIndex =
+                for i in 0 .. prevIndices.Length - 1 do
+                    if prevIndices.[i] > prevIndex then
+                        prevIndices.[i] <- prevIndices.[i] - 1
+                prevIndices.[originalIndexInPrevColl] <- -1 
+
+            // Shift all old indices between the previous and new position on move
+            let shiftForMove originalIndexInPrevColl prevIndex newIndex =
+                for i in 0 .. prevIndices.Length - 1 do
+                    if prevIndex < prevIndices.[i] && prevIndices.[i] <= newIndex then
+                        prevIndices.[i] <- prevIndices.[i] - 1
+                    else if newIndex <= prevIndices.[i] && prevIndices.[i] < prevIndex then
+                        prevIndices.[i] <- prevIndices.[i] + 1
+                prevIndices.[originalIndexInPrevColl] <- newIndex
+
+            // Return an update operation preceded by a move only if actual indices don't match
+            let moveAndUpdate oldIndex prev newIndex curr =
+                let prevIndex = prevIndices.[oldIndex]
+                if prevIndex = newIndex then
+                    Update (newIndex, prev, curr)
+                else
+                    shiftForMove oldIndex prevIndex newIndex
+                    MoveAndUpdate (prevIndex, prev, newIndex, curr)
+
             let operations =
                 [ for op in operations do
                     match op with
                     | Insert (index, element) ->
                         yield Insert (index, element)
+                        shiftForInsert index
 
-                        for i in 0 .. prevIndexes.Length - 1 do
-                            let prevIndex = prevIndexes.[i]
-                            if prevIndex >= index then
-                                prevIndexes.[i] <- prevIndexes.[i] + 1
-
-                    | Move (prevIndex, newIndex) ->
-                        if prevIndexes.[prevIndex] <> newIndex then
-                            yield (Move (prevIndexes.[prevIndex], newIndex))
-
-                            for i in 0 .. prevIndexes.Length - 1 do
-                                if prevIndexes.[prevIndex] < prevIndexes.[i] && prevIndexes.[i] <= newIndex then
-                                    prevIndexes.[i] <- prevIndexes.[i] - 1
-                                else if newIndex <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[prevIndex] then
-                                    prevIndexes.[i] <- prevIndexes.[i] + 1
-                            prevIndexes.[prevIndex] <- newIndex
+                    | Move (oldIndex, newIndex) ->
+                        // Prevent a move if the actual indices match
+                        let prevIndex = prevIndices.[oldIndex]
+                        if prevIndex <> newIndex then
+                            yield (Move (prevIndex, newIndex))
+                            shiftForMove oldIndex prevIndex newIndex
 
                     | Update (index, prev, curr) ->
-                        let realPrevIndex = prevIndexes.[index]
-                        if realPrevIndex <> index then
-                            yield MoveAndUpdate (realPrevIndex, prev, index, curr)
-
-                            for i in 0 .. prevIndexes.Length - 1 do
-                                if prevIndexes.[index] < prevIndexes.[i] && prevIndexes.[i] <= index then
-                                    prevIndexes.[i] <- prevIndexes.[i] - 1
-                                else if index <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[index] then
-                                    prevIndexes.[i] <- prevIndexes.[i] + 1
-                            prevIndexes.[index] <- index
-                        else
-                            yield Update (index, prev, curr)
+                        yield moveAndUpdate index prev index curr
 
-                    | MoveAndUpdate (prevIndex, prev, newIndex, curr) ->
-                        let realPrevIndex = prevIndexes.[prevIndex]
-                        if realPrevIndex = newIndex then
-                            yield Update (newIndex, prev, curr)
-                        else
-                            yield MoveAndUpdate (realPrevIndex, prev, newIndex, curr)
-
-                            for i in 0 .. prevIndexes.Length - 1 do
-                                if prevIndexes.[prevIndex] < prevIndexes.[i] && prevIndexes.[i] <= newIndex then
-                                    prevIndexes.[i] <- prevIndexes.[i] - 1
-                                else if newIndex <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[prevIndex] then
-                                    prevIndexes.[i] <- prevIndexes.[i] + 1
-                            prevIndexes.[prevIndex] <- newIndex
+                    | MoveAndUpdate (oldIndex, prev, newIndex, curr) ->
+                        yield moveAndUpdate oldIndex prev newIndex curr
 
-                    | Delete index ->
-                        yield Delete prevIndexes.[index]
-
-                        for i in 0 .. prevIndexes.Length - 1 do
-                            let prevIndex = prevIndexes.[i]
-                            if prevIndex >= index then
-                                prevIndexes.[i] <- prevIndexes.[i] - 1 ]
+                    | Delete oldIndex ->
+                        let prevIndex = prevIndices.[oldIndex]
+                        yield Delete prevIndex
+                        shiftForDelete oldIndex prevIndex ]
 
             if operations.Length = 0 then
                 NoChange
@@ -204,8 +212,8 @@ module Collections =
 
         let diffResult =
             diff aggressiveReuseMode prevCollOpt collOpt keyOf canReuse
-            |> reduceDiff (match prevCollOpt with ValueNone -> 0 | ValueSome c -> c.Length)
-                
+            |> adaptDiffForObservableCollection (match prevCollOpt with ValueNone -> 0 | ValueSome c -> c.Length)
+
         match diffResult with
         | NoChange -> ()
         | ClearCollection -> targetColl.Clear()

Fabulous.XamarinForms/src/Fabulous.XamarinForms/Xamarin.Forms.Core.json

@@ -1845,7 +1845,7 @@
           "source": null,
           "name": "Items",
           "inputType": "ViewElement list",
-          "updateCode": "Collections.updateItemsViewOfTItems<'T>"
+          "updateCode": "Collections.updateItemsViewOfTItems<Xamarin.Forms.Cell>"
         }
       ],
       "primaryConstructorMembers": [

Fabulous.XamarinForms/tests/Fabulous.XamarinForms.Tests/Collections/AdaptDiffTests.fs

@@ -0,0 +1,143 @@
+namespace Fabulous.XamarinForms.Tests.Collections
+
+open Fabulous
+open Fabulous.XamarinForms
+open Fabulous.XamarinForms.Collections
+open NUnit.Framework
+open FsUnit
+
+module AdaptDiffTests =
+    [<Test>]
+    let ``Test Reduce``() =
+        let previous =
+            [ View.Button(key = "Button0_0")
+              View.Button(key = "Button0_1")
+              View.Button(key = "Button0_2")
+              View.Button(key = "Button1_0")
+              View.Button(key = "Button1_1")
+              View.Button(key = "Button1_2")
+              View.Button(key = "Button2_0")
+              View.Button(key = "Button2_1")
+              View.Button(key = "Button2_2") ]
+
+        let current =
+            [ View.Button(key = "Button0_0")
+              View.Button(key = "Button0_1")
+              View.Button(key = "Button0_2")
+              View.Button(key = "Button1_0")
+              View.Image(key = "Image1_1")
+              View.Button(key = "Button1_2")
+              View.Button(key = "Button2_0")
+              View.Button(key = "Button2_1")
+              View.Button(key = "Button2_2") ]
+
+        let diffResult = DiffResult<ViewElement>.Operations [
+            Update (0, previous.[0], current.[0])
+            Update (1, previous.[1], current.[1])
+            Update (2, previous.[2], current.[2])
+            Update (3, previous.[3], current.[3])
+            Insert (4, current.[4])
+            Update (5, previous.[5], current.[5])
+            Update (6, previous.[6], current.[6])
+            Update (7, previous.[7], current.[7])
+            Update (8, previous.[8], current.[8])
+            Delete 4
+        ]
+
+        Collections.adaptDiffForObservableCollection previous.Length diffResult
+        |> should equal (DiffResult<ViewElement>.Operations [
+            Update (0, previous.[0], current.[0])
+            Update (1, previous.[1], current.[1])
+            Update (2, previous.[2], current.[2])
+            Update (3, previous.[3], current.[3])
+            Insert (4, current.[4])
+            MoveAndUpdate (6, previous.[5], 5, current.[5])
+            MoveAndUpdate (7, previous.[6], 6,current.[6])
+            MoveAndUpdate (8, previous.[7], 7, current.[7])
+            MoveAndUpdate (9, previous.[8], 8, current.[8])
+            Delete 9
+        ])
+
+    [<Test>]
+    let ``Test Reduce 2``() =
+        let previous =
+            [ View.Button(key = "Button0_0")
+              View.Button(key = "Button0_1")
+              View.Button(key = "Button0_2")
+              View.Button(key = "Button1_0")
+              View.Image(key = "Image1_1") ]
+
+        let current =
+            [ View.Label(key = "Button0_0")
+              View.Button(key = "Button0_1")
+              View.Image(key = "Button0_2") ]
+
+        let diffResult = DiffResult<ViewElement>.Operations [
+            Insert (0, current.[0])
+            Update (1, previous.[1], current.[1])
+            MoveAndUpdate (4, previous.[4], 2, current.[2])
+            Delete 0
+            Delete 2
+            Delete 3
+        ]
+
+        Collections.adaptDiffForObservableCollection previous.Length diffResult
+        |> should equal (DiffResult<ViewElement>.Operations [
+            Insert (0, current.[0])
+            MoveAndUpdate (2, previous.[1], 1, current.[1])
+            MoveAndUpdate (5, previous.[4], 2, current.[2])
+            Delete 3
+            Delete 3
+            Delete 3
+        ])
+
+    [<Test>]
+    let ``Test Reduce 3``() =
+        let previous =
+            [ View.Label(key = "0")
+              View.Label()
+              View.Button()
+              View.Button(key = "2")
+              View.Label()
+              View.Label(key = "3")
+              View.Label()
+              View.Button()
+              View.Button()
+              View.Button()
+              View.Button() ]
+
+        let current =
+            [ View.Button(key = "0")
+              View.Label() ]
+
+        let diffResult = DiffResult<ViewElement>.Operations [
+            MoveAndUpdate (2, previous.[2], 0, current.[0])
+            MoveAndUpdate (0, previous.[0], 1, current.[1])
+            Delete 1
+            Delete 3
+            Delete 4
+            Delete 5
+            Delete 6
+            Delete 7
+            Delete 8
+            Delete 9
+            Delete 10
+        ]
+
+        Collections.adaptDiffForObservableCollection previous.Length diffResult
+        |> should equal (DiffResult<ViewElement>.Operations [
+            MoveAndUpdate (2, previous.[2], 0, current.[0])
+            Update (1, previous.[0], current.[1])
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+            Delete 2
+        ])
+
+
+