bugfix: honour the windows that the ranges represent to avoid mismatc…

…hed DELETE/INSERT operations

Previously, we would just focus on finding _any_ window boundary and then
assume that was the boundary which matched the window for the purposes of
DELETE/INSERT move operations. However, this wasn't always true, especially
in the following case:
```
0..9 [10..20] 21...29 [30...40]
then move 30 to 10
0..9 [30,10...19] 20...28 [29,31...40]

expect:
 - DELETE 30, INSERT 30 (val=29)
 - DELETE 20, INSERT 10 (val=30)

but we would get:
 - DELETE 30, INSERT 20 (val=19)
 - DELETE 20, INSERT 10 (val=30)

because the code assumed that there was a window range [20,30] which there wasn't.
```

sync3/ops_test.go

@@ -433,6 +433,22 @@ func TestCalculateListOps_MultipleWindowOperations(t *testing.T) {
 			},
 			wantSubs: []string{"a", "e"},
 		},
+		{
+			name: "jump from lower window edge to upper window edge",
+			//                0    1    2    3    4    5    6    7    8
+			before: []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"},
+			after:  []string{"a", "f", "b", "c", "d", "e", "g", "h", "i"},
+			ranges: SliceRanges{{1, 3}, {5, 7}},
+			roomID: "f",
+			listOp: ListOpChange,
+			wantOps: []ResponseOp{
+				&ResponseOpSingle{Operation: OpDelete, Index: ptr(5)},
+				&ResponseOpSingle{Operation: OpInsert, Index: ptr(5), RoomID: "e"},
+				&ResponseOpSingle{Operation: OpDelete, Index: ptr(3)},
+				&ResponseOpSingle{Operation: OpInsert, Index: ptr(1), RoomID: "f"},
+			},
+			wantSubs: []string{"e"},
+		},
 		{
 			name: "addition moving multiple windows",
 			//                0    1    2    3    4    5    6    7    8
@@ -634,6 +650,94 @@ func TestCalculateListOps_TortureSingleWindowMiddle_Move(t *testing.T) {
 	}
 }
 
+/*
+func TestCalculateListOps_TortureMultipleWindows_Move(t *testing.T) {
+	rand.Seed(41)
+	ranges := SliceRanges{{0, 5}, {9, 11}}
+	inRangeIDs := map[string]bool{
+		"a": true, "b": true, "c": true, "d": true, "e": true, "f": true, "j": true, "k": true, "l": true,
+	}
+	for i := 0; i < 10000; i++ {
+		before := []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n"}
+		after, roomID, fromIndex, toIndex := testutils.MoveRandomElement(before)
+		t.Logf("move %s from %v to %v -> %v", roomID, fromIndex, toIndex, after)
+
+		sl := newStringList(before)
+		sl.sortedRoomIDs = after
+		gotOps, gotSubs := CalculateListOps(&RequestList{
+			Ranges: ranges,
+		}, sl, roomID, ListOpChange)
+		b, _ := json.Marshal(gotOps)
+		fmt.Println(string(b))
+		for _, sub := range gotSubs {
+			if inRangeIDs[sub] {
+				t.Fatalf("CalculateListOps: got sub %v but it was already in the range", sub)
+			}
+		}
+		// if there is a swap between the middle of the window OR at the end of the window, we expect no ops
+		isSwapInBetweenWindows := (fromIndex > int(ranges[0][1]) && fromIndex < int(ranges[1][0]) &&
+			toIndex > int(ranges[0][1]) && toIndex < int(ranges[1][0]))
+		isSwapEndOfWindow := (fromIndex > int(ranges[1][1])) && (toIndex > int(ranges[1][1]))
+		if isSwapInBetweenWindows || isSwapEndOfWindow {
+			// the swap happens outside the range, so we expect nothing
+			if len(gotOps) != 0 {
+				t.Errorf("CalculateLisOps: swap outside range got ops wanted none: %+v", gotOps)
+			}
+			continue
+		}
+		if fromIndex == toIndex {
+			// we want 0 ops
+			if len(gotOps) > 0 {
+				t.Errorf("CalculateLisOps: from==to got ops wanted none: %+v", gotOps)
+			}
+			continue
+		}
+		// if the swap occurs over multiple windows we expect 4 ops, not 2
+		insideFirstWindow := fromIndex <= int(ranges[0][1]) || toIndex <= int(ranges[0][1])
+		insideSecondWindow := (fromIndex >= int(ranges[1][0])) || (toIndex >= int(ranges[1][0]))
+		wantOps := 2
+		if insideFirstWindow && insideSecondWindow {
+			wantOps = 4
+		}
+		if len(gotOps) != wantOps {
+			t.Fatalf("CalculateListOps: wanted %v ops, got %+v", wantOps, gotOps)
+			continue
+		}
+
+			if int64(fromIndex) > ranges[0][1] {
+				// should inject a different room at the start of the range
+				fromIndex = int(ranges[0][1])
+			} else if int64(fromIndex) < ranges[0][0] {
+				// should inject a different room at the start of the range
+				fromIndex = int(ranges[0][0])
+			}
+			assertSingleOp(t, gotOps[0], "DELETE", fromIndex, "")
+			if int64(toIndex) > ranges[0][1] {
+				// should inject a different room at the end of the range
+				toIndex = int(ranges[0][1])
+				roomID = after[toIndex]
+			} else if int64(toIndex) < ranges[0][0] {
+				// should inject a different room at the end of the range
+				toIndex = int(ranges[0][0])
+				roomID = after[toIndex]
+			}
+			if !inRangeIDs[roomID] {
+				// it should now be in-range
+				inRange := false
+				for _, sub := range gotSubs {
+					if sub == roomID {
+						inRange = true
+						break
+					}
+				}
+				if !inRange {
+					t.Errorf("got subs %v missing room %v", gotSubs, roomID)
+				}
+			}
+			assertSingleOp(t, gotOps[1], "INSERT", toIndex, roomID)
+	}
+} */
+
 func assertSingleOp(t *testing.T, op ResponseOp, opName string, index int, optRoomID string) {
 	t.Helper()
 	singleOp, ok := op.(*ResponseOpSingle)

sync3/range.go

@@ -39,6 +39,8 @@ func (r SliceRanges) Inside(i int64) ([2]int64, bool) {
 //   [0,20] i=25,towardsZero=false => -1
 //   [0,20],[40,60] i=25,towardsZero=true => 20
 //   [0,20],[40,60] i=25,towardsZero=false => 40
+//   [0,20],[40,60] i=40,towardsZero=true => 40
+//   [20,40] i=40,towardsZero=true => 20
 func (r SliceRanges) ClosestInDirection(i int64, towardsZero bool) (closestIndex int64) {
 	// sort all range boundaries in ascending order
 	indexes := make([]int64, 0, len(r)*2)
@@ -59,7 +61,12 @@ func (r SliceRanges) ClosestInDirection(i int64, towardsZero bool) (closestIndex
 					}
 					return -1
 				}
-				return indexes[j-1]
+				// if the start of the window IS the index value, return the start of the window range, NOT an earlier range
+				if j%2 == 0 && indexes[j] == i {
+					return indexes[j]
+				} else {
+					return indexes[j-1]
+				}
 			}
 		}
 		return indexes[len(indexes)-1] // all values are lower than i, choose the highest
@@ -73,7 +80,12 @@ func (r SliceRanges) ClosestInDirection(i int64, towardsZero bool) (closestIndex
 					}
 					return -1
 				}
-				return indexes[j+1]
+				// if the end of the window IS the index value, return the end of the window range, NOT a later range
+				if j%2 == 1 && indexes[j] == i {
+					return indexes[j]
+				} else {
+					return indexes[j+1]
+				}
 			}
 		}
 		return indexes[0] // all values are higher than i, choose the lowest

sync3/range_test.go

@@ -191,6 +191,18 @@ func TestRangeClosestInDirection(t *testing.T) {
 			towardsZero:      true,
 			wantClosestIndex: 0,
 		},
+		{
+			ranges:           [][2]int64{{0, 20}},
+			i:                0,
+			towardsZero:      true,
+			wantClosestIndex: 0,
+		},
+		{
+			ranges:           [][2]int64{{0, 20}},
+			i:                20,
+			towardsZero:      false,
+			wantClosestIndex: 20,
+		},
 		{
 			ranges:           [][2]int64{{0, 20}},
 			i:                15,
@@ -269,6 +281,24 @@ func TestRangeClosestInDirection(t *testing.T) {
 			towardsZero:      true,
 			wantClosestIndex: 10,
 		},
+		{
+			ranges:           [][2]int64{{20, 40}},
+			i:                40,
+			towardsZero:      true,
+			wantClosestIndex: 20,
+		},
+		{
+			ranges:           [][2]int64{{0, 20}, {40, 60}},
+			i:                40,
+			towardsZero:      true,
+			wantClosestIndex: 40,
+		},
+		{
+			ranges:           [][2]int64{{0, 20}, {40, 60}},
+			i:                20,
+			towardsZero:      false,
+			wantClosestIndex: 20,
+		},
 	}
 	for _, tc := range testCases {
 		gotClosest := tc.ranges.ClosestInDirection(tc.i, tc.towardsZero)

sync3/request_test.go

@@ -832,6 +832,22 @@ func TestRequestList_CalculateMoveIndexes(t *testing.T) {
 			to:          5,
 			wantFromTos: [][2]int{{20, 10}, {40, 30}},
 		},
+		{
+			name: "jump from outside range edge to inside range edge",
+			rl: RequestList{
+				Ranges: [][2]int64{{10, 20}, {30, 40}},
+			},
+			from: 30,
+			to:   10,
+			// Moving from x to y:
+			// [10y...20]   [30x....40]
+			// means the timeline is now:
+			// [30, 10, 11...19] 20, 21 ... 28, [29,31...40]
+			// from a window perspective, this means we lost element 20@i=20 and gained element 30@i=10
+			// AND
+			// we lost element 30@i=30 and gained element 29@i=30.
+			wantFromTos: [][2]int{{20, 10}, {30, 30}},
+		},
 		{
 			name: "jump over 2 ranges towards infinity",
 			rl: RequestList{