Rework handling of mixed move_last_over() and modifications to actually work

src/collection_notifications.cpp

@@ -150,14 +150,7 @@ void CollectionChangeIndices::merge(realm::CollectionChangeIndices&& c)
 
 void CollectionChangeIndices::modify(size_t ndx)
 {
-    if (!insertions.contains(ndx))
     modifications.add(ndx);
-    // FIXME: this breaks mapping old row indices to new
-    // FIXME: is that a problem?
-    // If this row was previously moved, unmark it as a move
-    moves.erase(remove_if(begin(moves), end(moves),
-                          [&](auto move) { return move.to == ndx; }),
-                end(moves));
 }
 
 void CollectionChangeIndices::insert(size_t index, size_t count)
@@ -221,31 +214,30 @@ void CollectionChangeIndices::move(size_t from, size_t to)
 
         // Collapse A -> B, B -> C into a single A -> C move
         move.to = to;
-        modifications.erase_at(from);
-        insertions.erase_at(from);
-
-        modifications.shift_for_insert_at(to);
-        insertions.insert_at(to);
         updated_existing_move = true;
+
+        insertions.erase_at(from);
+        insertions.insert_at(to);
     }
-    if (updated_existing_move)
-        return;
 
-    if (!insertions.contains(from)) {
+    if (!updated_existing_move) {
-        auto shifted_from = insertions.unshift(from);
+        auto shifted_from = insertions.erase_and_unshift(from);
+        insertions.insert_at(to);
+
+        // Don't report deletions/moves for newly inserted rows
+        if (shifted_from != npos) {
             shifted_from = deletions.add_shifted(shifted_from);
-
-        // Don't record it as a move if the source row was newly inserted or
-        // was previously changed
-        if (!modifications.contains(from))
             moves.push_back({shifted_from, to});
         }
+    }
 
+    bool modified = modifications.contains(from);
     modifications.erase_at(from);
-    insertions.erase_at(from);
 
+    if (modified)
+        modifications.insert_at(to);
+    else
         modifications.shift_for_insert_at(to);
-    insertions.insert_at(to);
 }
 
 void CollectionChangeIndices::move_over(size_t row_ndx, size_t last_row)
@@ -255,6 +247,9 @@ void CollectionChangeIndices::move_over(size_t row_ndx, size_t last_row)
         erase(row_ndx);
         return;
     }
+    move(last_row, row_ndx);
+    erase(row_ndx + 1);
+    return;
 
     bool updated_existing_move = false;
     for (size_t i = 0; i < moves.size(); ++i) {
@@ -278,24 +273,19 @@ void CollectionChangeIndices::move_over(size_t row_ndx, size_t last_row)
         moves.push_back({last_row, row_ndx});
     }
 
-    if (insertions.contains(row_ndx)) {
     insertions.remove(row_ndx);
-    }
-    else {
-        if (modifications.contains(row_ndx)) {
     modifications.remove(row_ndx);
-        }
-        deletions.add(row_ndx);
-    }
 
-    if (insertions.contains(last_row)) {
+    // not add_shifted() because unordered removal does not shift
-        insertions.remove(last_row);
+    // mixed ordered/unordered removal currently not supported
-        insertions.add(row_ndx);
+    deletions.add(row_ndx);
-    }
+
-    else if (modifications.contains(last_row)) {
+    if (modifications.contains(last_row)) {
         modifications.remove(last_row);
         modifications.add(row_ndx);
     }
+
+    insertions.add(row_ndx);
 }
 
 void CollectionChangeIndices::verify()
@@ -305,10 +295,10 @@ void CollectionChangeIndices::verify()
         REALM_ASSERT(deletions.contains(move.from));
         REALM_ASSERT(insertions.contains(move.to));
     }
-    for (auto index : modifications.as_indexes())
+//    for (auto index : modifications.as_indexes())
-        REALM_ASSERT(!insertions.contains(index));
+//        REALM_ASSERT(!insertions.contains(index));
-    for (auto index : insertions.as_indexes())
+//    for (auto index : insertions.as_indexes())
-        REALM_ASSERT(!modifications.contains(index));
+//        REALM_ASSERT(!modifications.contains(index));
 #endif
 }
 

src/impl/async_query.cpp

@@ -121,13 +121,15 @@ void AsyncQuery::run()
 
         if (changes) {
             for (auto& idx : m_previous_rows) {
+                if (!map_moves(idx, *changes)) {
                     if (changes->deletions.contains(idx))
                         idx = npos;
-                else
+                    else {
-                    map_moves(idx, *changes);
                         REALM_ASSERT_DEBUG(!changes->insertions.contains(idx));
                     }
                 }
+            }
+        }
 
         m_changes = CollectionChangeIndices::calculate(m_previous_rows, next_rows,
                                                        [&](size_t row) { return m_info->row_did_change(*m_query->get_table(), row); },

src/impl/list_notifier.cpp

@@ -97,6 +97,11 @@ void ListNotifier::run()
         return;
     }
 
+    m_change.moves.erase(remove_if(begin(m_change.moves), end(m_change.moves),
+                                   [&](auto move) { return m_change.modifications.contains(move.to); }),
+                         end(m_change.moves));
+    m_change.modifications.remove(m_change.insertions);
+
     for (size_t i = 0; i < m_lv->size(); ++i) {
         if (m_change.insertions.contains(i) || m_change.modifications.contains(i))
             continue;

tests/collection_change_indices.cpp

@@ -39,10 +39,11 @@ TEST_CASE("collection change indices") {
             REQUIRE_INDICES(c.modifications, 3, 4);
         }
 
-        SECTION("modify() on an inserted row is a no-op") {
+        SECTION("modify() on an inserted row marks it as both inserted and modified") {
             c.insert(3);
             c.modify(3);
-            REQUIRE(c.modifications.empty());
+            REQUIRE_INDICES(c.insertions, 3);
+            REQUIRE_INDICES(c.modifications, 3);
         }
 
         SECTION("modify() doesn't interact with deleted rows") {
@@ -133,13 +134,13 @@ TEST_CASE("collection change indices") {
             REQUIRE_MOVES(c, {8, 5});
         }
 
-        SECTION("move_over() removes previous insertions for that row") {
+        SECTION("move_over() does not mark the old last row as moved if it was newly inserted") {
-            c.insert(5);
+            c.insert(8);
             c.move_over(5, 8);
-            REQUIRE(c.insertions.empty());
+            REQUIRE(c.moves.empty());
         }
 
-        SECTION("move_over() removes previous modifications for that row") {
+        SECTION("move_over() removes previous modifications for the removed row") {
             c.modify(5);
             c.move_over(5, 8);
             REQUIRE(c.modifications.empty());
@@ -160,7 +161,7 @@ TEST_CASE("collection change indices") {
         SECTION("move_over() removes moves to the target") {
             c.move(3, 5);
             c.move_over(5, 8);
-            REQUIRE(c.moves.empty());
+            REQUIRE_MOVES(c, {8, 5});
         }
 
         SECTION("move_over() updates moves to the source") {
@@ -172,7 +173,8 @@ TEST_CASE("collection change indices") {
         SECTION("move_over() is not shifted by previous calls to move_over()") {
             c.move_over(5, 10);
             c.move_over(6, 9);
-            REQUIRE_INDICES(c.deletions, 5, 6);
+            REQUIRE_INDICES(c.deletions, 5, 6, 9, 10);
+            REQUIRE_INDICES(c.insertions, 5, 6);
             REQUIRE_MOVES(c, {10, 5}, {9, 6});
         }
     }

tests/results.cpp

@@ -204,12 +204,13 @@ TEST_CASE("Results") {
             REQUIRE_INDICES(change.deletions, 1);
         }
 
-        SECTION("modifying a non-matching row to match marks that row as inserted") {
+        SECTION("modifying a non-matching row to match marks that row as inserted, but not modified") {
             write([&] {
                 table->set_int(0, 7, 3);
             });
             REQUIRE(notification_calls == 2);
             REQUIRE_INDICES(change.insertions, 4);
+            REQUIRE(change.modifications.empty());
         }
 
         SECTION("deleting a matching row marks that row as deleted") {

tests/transaction_log_parsing.cpp

@@ -97,9 +97,12 @@ private:
             CHECK(m_initial[i] == m_linkview->get(i).get_int(0));
 
         // Verify that everything marked as a move actually is one
-        for (size_t i = 0; i < move_sources.size(); ++i)
+        for (size_t i = 0; i < move_sources.size(); ++i) {
+            if (!info.modifications.contains(info.moves[i].to)) {
                 CHECK(m_linkview->get(info.moves[i].to).get_int(0) == move_sources[i]);
             }
+        }
+    }
 };
 
 TEST_CASE("Transaction log parsing") {
@@ -386,14 +389,14 @@ TEST_CASE("Transaction log parsing") {
             REQUIRE(info.tables[2].deletions.empty());
         }
 
-        SECTION("modifying newly added rows is not reported as a modification") {
+        SECTION("modifying newly added rows is reported as a modification") {
             auto info = track_changes({false, false, true}, [&] {
                 table.add_empty_row();
                 table.set_int(0, 10, 10);
             });
             REQUIRE(info.tables.size() == 3);
             REQUIRE_INDICES(info.tables[2].insertions, 10);
-            REQUIRE(info.tables[2].modifications.empty());
+            REQUIRE_INDICES(info.tables[2].modifications, 10);
         }
 
         SECTION("move_last_over() does not shift rows other than the last one") {
@@ -402,7 +405,8 @@ TEST_CASE("Transaction log parsing") {
                 table.move_last_over(3);
             });
             REQUIRE(info.tables.size() == 3);
-            REQUIRE_INDICES(info.tables[2].deletions, 2, 3);
+            REQUIRE_INDICES(info.tables[2].deletions, 2, 3, 8, 9);
+            REQUIRE_INDICES(info.tables[2].insertions, 2, 3);
             REQUIRE_MOVES(info.tables[2], {9, 2}, {8, 3});
         }
     }
@@ -651,7 +655,7 @@ TEST_CASE("Transaction log parsing") {
                     lv->set(5, 1);
                 }
                 REQUIRE_INDICES(changes.insertions, 5);
-                REQUIRE(changes.modifications.size() == 0);
+                REQUIRE_INDICES(changes.modifications, 5);
 
                 VALIDATE_CHANGES(changes) {
                     lv->insert(5, 0);
@@ -808,7 +812,7 @@ TEST_CASE("Transaction log parsing") {
 
                 REQUIRE_INDICES(changes.deletions, 5);
                 REQUIRE_INDICES(changes.insertions, 0);
-                REQUIRE(changes.moves.empty());
+                REQUIRE_MOVES(changes, {5, 0});
             }
 
             SECTION("set after move is just insert+delete") {
@@ -819,7 +823,7 @@ TEST_CASE("Transaction log parsing") {
 
                 REQUIRE_INDICES(changes.deletions, 5);
                 REQUIRE_INDICES(changes.insertions, 0);
-                REQUIRE(changes.moves.empty());
+                REQUIRE_MOVES(changes, {5, 0});
             }
 
             SECTION("delete after move removes original row") {