Fix edge cases in GenerateMovements() and OptimizeMovements() interaction

Now GenerateMovements() generate all movements blindly, and depend on the
optimization done by OptimizeMovements() to remove reduntant actions.

Author: kklimonda-cl

assets/pango/movement/movement.go

@@ -254,8 +254,6 @@ func (o PositionBefore) Move(entries []Movable, existing []Movable) ([]MoveActio
 		return nil, err
 	}
 
-	slog.Debug("PositionBefore.Move()", "existing", existing, "expected", expected, "entries", entries)
-
 	actions, err := GenerateMovements(existing, expected, entries, ActionWhereBefore, o.Pivot, o.Directly)
 	if err != nil {
 		return nil, err
@@ -292,23 +290,11 @@ func updateSimulatedIdxMap(idxMap *map[Movable]int, moved Movable, startingIdx i
 func OptimizeMovements(existing []Movable, expected []Movable, entries []Movable, actions []MoveAction, position Position) []MoveAction {
 	simulated := make([]Movable, len(existing))
 	copy(simulated, existing)
-
 	simulatedIdxMap := createIdxMapFor(simulated)
-	expectedIdxMap := createIdxMapFor(expected)
 
 	var optimized []MoveAction
-
-	switch position.(type) {
-	case PositionBefore, PositionAfter:
-	default:
-		return actions
-	}
-
 	for _, action := range actions {
 		currentIdx := simulatedIdxMap[action.Movable]
-		if currentIdx == expectedIdxMap[action.Movable] {
-			continue
-		}
 
 		var targetIdx int
 		switch action.Where {
@@ -317,11 +303,13 @@ func OptimizeMovements(existing []Movable, expected []Movable, entries []Movable
 		case ActionWhereBottom:
 			targetIdx = len(simulated) - 1
 		case ActionWhereBefore:
-			targetIdx = simulatedIdxMap[action.Destination] - 1
+			targetIdx = simulatedIdxMap[action.Destination]
 		case ActionWhereAfter:
 			targetIdx = simulatedIdxMap[action.Destination] + 1
 		}
 
+		slog.Debug("OptimizeMovements()", "action", action, "currentIdx", currentIdx, "targetIdx", targetIdx)
+
 		if targetIdx != currentIdx {
 			optimized = append(optimized, action)
 			simulatedIdxMap[action.Movable] = targetIdx
@@ -346,11 +334,7 @@ func GenerateMovements(existing []Movable, expected []Movable, entries []Movable
 	var movements []MoveAction
 	var previous Movable
 	for _, elt := range entries {
-		slog.Debug("GenerateMovements()", "elt", elt, "existing", existingIdxMap[elt], "expected", expectedIdxMap[elt], "len(expected)", len(expected))
-		// If existing index for the element matches the expected one, skip it over
-		if existingIdxMap[elt] == expectedIdxMap[elt] {
-			continue
-		}
+		slog.Debug("GeneraveMovements()", "elt", elt, "existing", existingIdxMap[elt], "expected", expectedIdxMap[elt])
 
 		if previous != nil {
 			movements = append(movements, MoveAction{
@@ -392,10 +376,10 @@ func GenerateMovements(existing []Movable, expected []Movable, entries []Movable
 				eltExpectedIdx := expectedIdxMap[elt]
 				pivot = expected[eltExpectedIdx+1]
 				where = ActionWhereBefore
-				// if previous was nil (we are processing the first element in entries set)
-				// and selected pivot is part of the entries set it means the order of entries
-				// changes between existing and expected sets. If direct move has been requested,
-				// we need to find the correct pivot point for the move.
+				// If previous was nil (we are processing the first element in entries set)
+				// and selected pivot is part of the entries set, it means the order of elements
+				// changes between existing adn expected sets. In this case the actual pivot
+				// is element from expected set that follows all moved elements.
 				if _, ok := entriesIdxMap[pivot]; ok && directly {
 					// The actual pivot for the move is the element that follows all elements
 					// from the existing set.
@@ -406,6 +390,7 @@ func GenerateMovements(existing []Movable, expected []Movable, entries []Movable
 						return nil, ErrInvalidMovementPlan
 					}
 					pivot = expected[pivotIdx]
+
 				}
 			}
 
@@ -419,7 +404,7 @@ func GenerateMovements(existing []Movable, expected []Movable, entries []Movable
 
 	}
 
-	slog.Debug("GeneraveMovements()", "movements", movements)
+	slog.Debug("GenerateMovements()", "movements", movements)
 
 	return movements, nil
 }
@@ -464,14 +449,14 @@ func (o PositionBottom) GetExpected(entries []Movable, existing []Movable) ([]Mo
 }
 
 func (o PositionBottom) Move(entries []Movable, existing []Movable) ([]MoveAction, error) {
-	slog.Debug("PositionBottom.Move())", "entries", entries, "existing", existing)
 	expected, err := o.GetExpected(entries, existing)
 	if err != nil {
 		return nil, err
 	}
 
 	actions, err := GenerateMovements(existing, expected, entries, ActionWhereBottom, nil, false)
 	if err != nil {
+		slog.Debug("PositionBottom()", "err", err)
 		return nil, err
 	}
 	return OptimizeMovements(existing, expected, entries, actions, o), nil
@@ -487,7 +472,6 @@ func MoveGroups(existing []Movable, movements []Movement) ([]MoveAction, error)
 	for idx := range len(movements) - 1 {
 		position := movements[idx].Position
 		entries := movements[idx].Entries
-		slog.Debug("MoveGroups()", "position", position, "existing", existing, "entries", entries)
 		result, err := position.GetExpected(entries, expected)
 		if err != nil {
 			if !errors.Is(err, errNoMovements) {
@@ -500,7 +484,6 @@ func MoveGroups(existing []Movable, movements []Movement) ([]MoveAction, error)
 
 	entries := movements[len(movements)-1].Entries
 	position := movements[len(movements)-1].Position
-	slog.Debug("MoveGroups()", "position", position, "expected", expected, "entries", entries)
 	return position.Move(entries, expected)
 }
 

assets/pango/movement/movement_test.go

@@ -71,6 +71,8 @@ var _ = Describe("MoveGroup()", func() {
 				moves, err := movement.MoveGroup(movement.PositionTop{}, entries, existing)
 				Expect(err).ToNot(HaveOccurred())
 
+				// '((E 'top nil)(B 'after E)(C 'after B)(D 'after C))
+				// 'A element stays in place
 				Expect(moves).To(HaveLen(4))
 			})
 		})
@@ -189,9 +191,54 @@ var _ = Describe("MoveGroup()", func() {
 			})
 		})
 	})
+
+	// '(A E B C D) -> '(A B C D E) => '(E 'bottom nil) / '(E 'after D)
+
+	// PositionSomewhereBefore PositionDirectlyBefore
+	// '(C B 'before E, directly)
+	// '(A B C D E) -> '(A D C B E) -> '(B 'before E)
+	// '(A B C D E) -> '(A C B D E) -> '(B 'after C)
+
 	Context("With PositionBefore used as position", func() {
 		existing := asMovable([]string{"A", "B", "C", "D", "E"})
+		Context("when doing a direct move with entries reordering", func() {
+			It("should put reordered entries directly before pivot point", func() {
+				// '(A B C D E) -> '(A D C B E)
+				entries := asMovable([]string{"C", "B"})
+				moves, err := movement.MoveGroup(
+					movement.PositionBefore{Directly: true, Pivot: Mock{"E"}},
+					entries, existing,
+				)
+
+				Expect(err).ToNot(HaveOccurred())
+				Expect(moves).To(HaveLen(2))
+
+				Expect(moves[0].Movable.EntryName()).To(Equal("C"))
+				Expect(moves[0].Where).To(Equal(movement.ActionWhereBefore))
+				Expect(moves[0].Destination.EntryName()).To(Equal("E"))
+
+				Expect(moves[1].Movable.EntryName()).To(Equal("B"))
+				Expect(moves[1].Where).To(Equal(movement.ActionWhereAfter))
+				Expect(moves[1].Destination.EntryName()).To(Equal("C"))
+			})
+		})
+		Context("when doing a non direct move with entries reordering", func() {
+			It("should reorder entries in-place without moving them around", func() {
+				// '(A B C D E) -> '(A C B D E)
+				entries := asMovable([]string{"C", "B"})
+				moves, err := movement.MoveGroup(
+					movement.PositionBefore{Directly: false, Pivot: Mock{"E"}},
+					entries, existing,
+				)
+
+				Expect(err).ToNot(HaveOccurred())
+				Expect(moves).To(HaveLen(1))
 
+				Expect(moves[0].Movable.EntryName()).To(Equal("C"))
+				Expect(moves[0].Where).To(Equal(movement.ActionWhereBefore))
+				Expect(moves[0].Destination.EntryName()).To(Equal("B"))
+			})
+		})
 		Context("when direct position relative to the pivot is not required", func() {
 			Context("and moved entries are already before pivot point", func() {
 				It("should not generate any move actions", func() {
@@ -249,8 +296,8 @@ var _ = Describe("MoveGroup()", func() {
 					Expect(moves).To(HaveLen(1))
 
 					Expect(moves[0].Movable.EntryName()).To(Equal("C"))
-					Expect(moves[0].Where).To(Equal(movement.ActionWhereBefore))
-					Expect(moves[0].Destination.EntryName()).To(Equal("B"))
+					Expect(moves[0].Where).To(Equal(movement.ActionWhereAfter))
+					Expect(moves[0].Destination.EntryName()).To(Equal("A"))
 				})
 			})
 		})
@@ -355,6 +402,10 @@ var _ = Describe("Movement benchmarks", func() {
 
 			Expect(err).ToNot(HaveOccurred())
 			Expect(moves).To(HaveLen(6))
+
+			Expect(moves[0].Movable.EntryName()).To(Equal("90"))
+			Expect(moves[0].Where).To(Equal(movement.ActionWhereBefore))
+			Expect(moves[0].Destination.EntryName()).To(Equal("100"))
 		})
 	})
 })