From e25e4c2dcd20a704968d2aee6c89e015a2230e4d Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Fri, 4 Mar 2016 11:19:33 -0800 Subject: [PATCH] Rework handling of mixed move_last_over() and modifications to actually work --- src/collection_notifications.cpp | 74 +++++++++++++---------------- src/impl/async_query.cpp | 12 +++-- src/impl/list_notifier.cpp | 5 ++ tests/collection_change_indices.cpp | 18 +++---- tests/results.cpp | 3 +- tests/transaction_log_parsing.cpp | 20 ++++---- 6 files changed, 68 insertions(+), 64 deletions(-) diff --git a/src/collection_notifications.cpp b/src/collection_notifications.cpp index 3f82c913..6087af5d 100644 --- a/src/collection_notifications.cpp +++ b/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)); + modifications.add(ndx); } 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)) { - auto shifted_from = insertions.unshift(from); - shifted_from = deletions.add_shifted(shifted_from); + if (!updated_existing_move) { + auto shifted_from = insertions.erase_and_unshift(from); + insertions.insert_at(to); - // Don't record it as a move if the source row was newly inserted or - // was previously changed - if (!modifications.contains(from)) + // Don't report deletions/moves for newly inserted rows + if (shifted_from != npos) { + shifted_from = deletions.add_shifted(shifted_from); moves.push_back({shifted_from, to}); + } } + bool modified = modifications.contains(from); modifications.erase_at(from); - insertions.erase_at(from); - modifications.shift_for_insert_at(to); - insertions.insert_at(to); + if (modified) + modifications.insert_at(to); + else + modifications.shift_for_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); - } + insertions.remove(row_ndx); + modifications.remove(row_ndx); - if (insertions.contains(last_row)) { - insertions.remove(last_row); - insertions.add(row_ndx); - } - else if (modifications.contains(last_row)) { + // not add_shifted() because unordered removal does not shift + // mixed ordered/unordered removal currently not supported + deletions.add(row_ndx); + + 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()) - REALM_ASSERT(!insertions.contains(index)); - for (auto index : insertions.as_indexes()) - REALM_ASSERT(!modifications.contains(index)); +// for (auto index : modifications.as_indexes()) +// REALM_ASSERT(!insertions.contains(index)); +// for (auto index : insertions.as_indexes()) +// REALM_ASSERT(!modifications.contains(index)); #endif } diff --git a/src/impl/async_query.cpp b/src/impl/async_query.cpp index a91491fa..f1c2845b 100644 --- a/src/impl/async_query.cpp +++ b/src/impl/async_query.cpp @@ -121,11 +121,13 @@ void AsyncQuery::run() if (changes) { for (auto& idx : m_previous_rows) { - if (changes->deletions.contains(idx)) - idx = npos; - else - map_moves(idx, *changes); - REALM_ASSERT_DEBUG(!changes->insertions.contains(idx)); + if (!map_moves(idx, *changes)) { + if (changes->deletions.contains(idx)) + idx = npos; + else { + REALM_ASSERT_DEBUG(!changes->insertions.contains(idx)); + } + } } } diff --git a/src/impl/list_notifier.cpp b/src/impl/list_notifier.cpp index f32b4b49..83d67b7d 100644 --- a/src/impl/list_notifier.cpp +++ b/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; diff --git a/tests/collection_change_indices.cpp b/tests/collection_change_indices.cpp index cab25f58..e30f0593 100644 --- a/tests/collection_change_indices.cpp +++ b/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") { - c.insert(5); + SECTION("move_over() does not mark the old last row as moved if it was newly inserted") { + 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}); } } diff --git a/tests/results.cpp b/tests/results.cpp index be8c13d4..ea7751e3 100644 --- a/tests/results.cpp +++ b/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") { diff --git a/tests/transaction_log_parsing.cpp b/tests/transaction_log_parsing.cpp index dd5406a4..dc8b244c 100644 --- a/tests/transaction_log_parsing.cpp +++ b/tests/transaction_log_parsing.cpp @@ -97,8 +97,11 @@ 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) - CHECK(m_linkview->get(info.moves[i].to).get_int(0) == move_sources[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]); + } + } } }; @@ -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") {