diff --git a/src/impl/collection_change_builder.cpp b/src/impl/collection_change_builder.cpp index 2fa652f9..c81cc502 100644 --- a/src/impl/collection_change_builder.cpp +++ b/src/impl/collection_change_builder.cpp @@ -550,9 +550,9 @@ void calculate_moves_sorted(std::vector& rows, CollectionChangeSet& cha CollectionChangeBuilder CollectionChangeBuilder::calculate(std::vector const& prev_rows, std::vector const& next_rows, std::function row_did_change, - bool sort) + bool rows_are_in_table_order) { - REALM_ASSERT_DEBUG(sort || std::is_sorted(begin(next_rows), end(next_rows))); + REALM_ASSERT_DEBUG(!rows_are_in_table_order || std::is_sorted(begin(next_rows), end(next_rows))); CollectionChangeBuilder ret; @@ -627,7 +627,7 @@ CollectionChangeBuilder CollectionChangeBuilder::calculate(std::vector c } } - if (sort) { + if (!rows_are_in_table_order) { calculate_moves_sorted(new_rows, ret); } else { diff --git a/src/impl/results_notifier.cpp b/src/impl/results_notifier.cpp index c72fdd0a..6e91f3d0 100644 --- a/src/impl/results_notifier.cpp +++ b/src/impl/results_notifier.cpp @@ -27,7 +27,7 @@ ResultsNotifier::ResultsNotifier(Results& target) : CollectionNotifier(target.get_realm()) , m_target_results(&target) , m_sort(target.get_sort()) -, m_from_linkview(target.get_linkview().get() != nullptr) +, m_target_is_in_table_order(target.is_in_table_order()) { Query q = target.get_query(); set_table(*q.get_table()); @@ -87,12 +87,8 @@ bool ResultsNotifier::need_to_run() } // If we've run previously, check if we need to rerun - if (m_initial_run_complete) { - // Make an empty tableview from the query to get the table version, since - // Query doesn't expose it - if (m_query->find_all(0, 0, 0).sync_if_needed() == m_last_seen_version) { - return false; - } + if (m_initial_run_complete && m_query->sync_view_if_needed() == m_last_seen_version) { + return false; } return true; @@ -125,7 +121,7 @@ void ResultsNotifier::calculate_changes() m_changes = CollectionChangeBuilder::calculate(m_previous_rows, next_rows, get_modification_checker(*m_info, *m_query->get_table()), - m_sort || m_from_linkview); + m_target_is_in_table_order && !m_sort); m_previous_rows = std::move(next_rows); } @@ -141,6 +137,7 @@ void ResultsNotifier::run() if (!need_to_run()) return; + m_query->sync_view_if_needed(); m_tv = m_query->find_all(); if (m_sort) { m_tv.sort(m_sort.column_indices, m_sort.ascending); diff --git a/src/impl/results_notifier.hpp b/src/impl/results_notifier.hpp index de0b2d65..8028fbd0 100644 --- a/src/impl/results_notifier.hpp +++ b/src/impl/results_notifier.hpp @@ -36,7 +36,7 @@ private: Results* m_target_results; const SortOrder m_sort; - bool m_from_linkview; + bool m_target_is_in_table_order; // The source Query, in handover form iff m_sg is null std::unique_ptr> m_query_handover; diff --git a/src/results.cpp b/src/results.cpp index 1095731a..5e9064f9 100644 --- a/src/results.cpp +++ b/src/results.cpp @@ -72,6 +72,17 @@ Results::Results(SharedRealm r, const ObjectSchema& o, LinkViewRef lv, util::Opt } } +Results::Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s) +: m_realm(std::move(r)) +, m_object_schema(&o) +, m_table_view(std::move(tv)) +, m_table(&m_table_view.get_parent()) +, m_sort(std::move(s)) +, m_mode(Mode::TableView) +{ + REALM_ASSERT(m_sort.column_indices.size() == m_sort.ascending.size()); +} + Results::~Results() { if (m_notifier) { @@ -85,7 +96,7 @@ void Results::validate_read() const m_realm->verify_thread(); if (m_table && !m_table->is_attached()) throw InvalidatedException(); - if (m_mode == Mode::TableView && !m_table_view.is_attached()) + if (m_mode == Mode::TableView && (!m_table_view.is_attached() || m_table_view.depends_on_deleted_object())) throw InvalidatedException(); if (m_mode == Mode::LinkView && !m_link_view->is_attached()) throw InvalidatedException(); @@ -115,8 +126,10 @@ size_t Results::size() switch (m_mode) { case Mode::Empty: return 0; case Mode::Table: return m_table->size(); - case Mode::Query: return m_query.count(); case Mode::LinkView: return m_link_view->size(); + case Mode::Query: + m_query.sync_view_if_needed(); + return m_query.count(); case Mode::TableView: update_tableview(); return m_table_view.size(); @@ -218,6 +231,7 @@ void Results::update_tableview() case Mode::LinkView: return; case Mode::Query: + m_query.sync_view_if_needed(); m_table_view = m_query.find_all(); if (m_sort) { m_table_view.sort(m_sort.column_indices, m_sort.ascending); @@ -384,8 +398,19 @@ Query Results::get_query() const case Mode::Empty: case Mode::Query: return m_query; - case Mode::TableView: - return m_table_view.get_query(); + case Mode::TableView: { + // A TableView has an associated Query if it was produced by Query::find_all. This is indicated + // by TableView::get_query returning a Query with a non-null table. + Query query = m_table_view.get_query(); + if (query.get_table()) { + return query; + } + + // The TableView has no associated query so create one with no conditions that is restricted + // to the rows in the TableView. + m_table_view.sync_if_needed(); + return Query(*m_table, std::make_unique(m_table_view)); + } case Mode::LinkView: return m_table->where(m_link_view); case Mode::Table: @@ -417,15 +442,11 @@ TableView Results::get_tableview() Results Results::sort(realm::SortOrder&& sort) const { REALM_ASSERT(sort.column_indices.size() == sort.ascending.size()); - if (m_link_view) - return Results(m_realm, *m_object_schema, m_link_view, m_query, std::move(sort)); return Results(m_realm, *m_object_schema, get_query(), std::move(sort)); } Results Results::filter(Query&& q) const { - if (m_link_view) - return Results(m_realm, *m_object_schema, m_link_view, get_query().and_query(std::move(q)), m_sort); return Results(m_realm, *m_object_schema, get_query().and_query(std::move(q)), m_sort); } @@ -457,6 +478,21 @@ NotificationToken Results::add_notification_callback(CollectionChangeCallback cb return {m_notifier, m_notifier->add_callback(std::move(cb))}; } +bool Results::is_in_table_order() const +{ + switch (m_mode) { + case Mode::Empty: + case Mode::Table: + return true; + case Mode::LinkView: + return false; + case Mode::Query: + return m_query.produces_results_in_table_order() && !m_sort; + case Mode::TableView: + return m_table_view.is_in_table_order(); + } +} + void Results::Internal::set_table_view(Results& results, realm::TableView &&tv) { // If the previous TableView was never actually used, then stop generating diff --git a/src/results.hpp b/src/results.hpp index 388b85fe..6b25982a 100644 --- a/src/results.hpp +++ b/src/results.hpp @@ -51,6 +51,7 @@ public: Results() = default; Results(SharedRealm r, const ObjectSchema& o, Table& table); Results(SharedRealm r, const ObjectSchema& o, Query q, SortOrder s = {}); + Results(SharedRealm r, const ObjectSchema& o, TableView tv, SortOrder s); Results(SharedRealm r, const ObjectSchema& o, LinkViewRef lv, util::Optional q = {}, SortOrder s = {}); ~Results(); @@ -182,6 +183,9 @@ public: bool wants_background_updates() const { return m_wants_background_updates; } + // Returns whether the rows are guaranteed to be in table order. + bool is_in_table_order() const; + // Helper type to let ResultsNotifier update the tableview without giving access // to any other privates or letting anyone else do so class Internal { diff --git a/tests/collection_change_indices.cpp b/tests/collection_change_indices.cpp index 42625bfb..d0f9ec92 100644 --- a/tests/collection_change_indices.cpp +++ b/tests/collection_change_indices.cpp @@ -338,97 +338,6 @@ TEST_CASE("[collection_change] calculate() unsorted") { auto none_modified = [](size_t) { return false; }; const auto npos = size_t(-1); - SECTION("returns an empty set when input and output are identical") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false); - REQUIRE(c.empty()); - } - - SECTION("marks all as inserted when prev is empty") { - c = _impl::CollectionChangeBuilder::calculate({}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.insertions, 0, 1, 2); - } - - SECTION("marks all as deleted when new is empty") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {}, all_modified, false); - REQUIRE_INDICES(c.deletions, 0, 1, 2); - } - - SECTION("marks npos rows in prev as deleted") { - c = _impl::CollectionChangeBuilder::calculate({npos, 1, 2, 3, npos}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.deletions, 0, 4); - } - - SECTION("marks modified rows which do not move as modified") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.modifications, 0, 1, 2); - } - - SECTION("does not mark unmodified rows as modified") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false); - REQUIRE(c.modifications.empty()); - } - - SECTION("marks newly added rows as insertions") { - c = _impl::CollectionChangeBuilder::calculate({2, 3}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.insertions, 0); - - c = _impl::CollectionChangeBuilder::calculate({1, 3}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.insertions, 1); - - c = _impl::CollectionChangeBuilder::calculate({1, 2}, {1, 2, 3}, all_modified, false); - REQUIRE_INDICES(c.insertions, 2); - } - - SECTION("marks removed rows as deleted") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2}, all_modified, false); - REQUIRE_INDICES(c.deletions, 2); - - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3}, all_modified, false); - REQUIRE_INDICES(c.deletions, 1); - - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3}, all_modified, false); - REQUIRE_INDICES(c.deletions, 0); - } - - SECTION("marks rows as both inserted and deleted") { - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 4}, all_modified, false); - REQUIRE_INDICES(c.deletions, 1); - REQUIRE_INDICES(c.insertions, 2); - REQUIRE(c.moves.empty()); - } - - SECTION("marks rows as modified even if they moved") { - c = _impl::CollectionChangeBuilder::calculate({5, 3}, {3, 5}, all_modified, false); - REQUIRE_MOVES(c, {1, 0}); - REQUIRE_INDICES(c.modifications, 0, 1); - } - - SECTION("does not mark rows as modified if they are new") { - c = _impl::CollectionChangeBuilder::calculate({3}, {3, 5}, all_modified, false); - REQUIRE_INDICES(c.modifications, 0); - } - - SECTION("reports moves which can be produced by move_last_over()") { - auto calc = [&](std::vector values) { - return _impl::CollectionChangeBuilder::calculate(values, {1, 2, 3}, none_modified, false); - }; - - REQUIRE(calc({1, 2, 3}).empty()); - REQUIRE_MOVES(calc({1, 3, 2}), {2, 1}); - REQUIRE_MOVES(calc({2, 1, 3}), {1, 0}); - REQUIRE_MOVES(calc({2, 3, 1}), {2, 0}); - REQUIRE_MOVES(calc({3, 1, 2}), {1, 0}, {2, 1}); - REQUIRE_MOVES(calc({3, 2, 1}), {2, 0}, {1, 1}); - } -} - -TEST_CASE("[collection_change] calculate() sorted") { - _impl::CollectionChangeBuilder c; - - auto all_modified = [](size_t) { return true; }; - auto none_modified = [](size_t) { return false; }; - const auto npos = size_t(-1); - SECTION("returns an empty set when input and output are identical") { c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, true); REQUIRE(c.empty()); @@ -489,9 +398,8 @@ TEST_CASE("[collection_change] calculate() sorted") { } SECTION("marks rows as modified even if they moved") { - c = _impl::CollectionChangeBuilder::calculate({3, 5}, {5, 3}, all_modified, true); - REQUIRE_INDICES(c.deletions, 1); - REQUIRE_INDICES(c.insertions, 0); + c = _impl::CollectionChangeBuilder::calculate({5, 3}, {3, 5}, all_modified, true); + REQUIRE_MOVES(c, {1, 0}); REQUIRE_INDICES(c.modifications, 0, 1); } @@ -500,9 +408,101 @@ TEST_CASE("[collection_change] calculate() sorted") { REQUIRE_INDICES(c.modifications, 0); } + SECTION("reports moves which can be produced by move_last_over()") { + auto calc = [&](std::vector values) { + return _impl::CollectionChangeBuilder::calculate(values, {1, 2, 3}, none_modified, true); + }; + + REQUIRE(calc({1, 2, 3}).empty()); + REQUIRE_MOVES(calc({1, 3, 2}), {2, 1}); + REQUIRE_MOVES(calc({2, 1, 3}), {1, 0}); + REQUIRE_MOVES(calc({2, 3, 1}), {2, 0}); + REQUIRE_MOVES(calc({3, 1, 2}), {1, 0}, {2, 1}); + REQUIRE_MOVES(calc({3, 2, 1}), {2, 0}, {1, 1}); + } +} + +TEST_CASE("[collection_change] calculate() sorted") { + _impl::CollectionChangeBuilder c; + + auto all_modified = [](size_t) { return true; }; + auto none_modified = [](size_t) { return false; }; + const auto npos = size_t(-1); + + SECTION("returns an empty set when input and output are identical") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false); + REQUIRE(c.empty()); + } + + SECTION("marks all as inserted when prev is empty") { + c = _impl::CollectionChangeBuilder::calculate({}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.insertions, 0, 1, 2); + } + + SECTION("marks all as deleted when new is empty") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {}, all_modified, false); + REQUIRE_INDICES(c.deletions, 0, 1, 2); + } + + SECTION("marks npos rows in prev as deleted") { + c = _impl::CollectionChangeBuilder::calculate({npos, 1, 2, 3, npos}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.deletions, 0, 4); + } + + SECTION("marks modified rows which do not move as modified") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.modifications, 0, 1, 2); + } + + SECTION("does not mark unmodified rows as modified") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2, 3}, none_modified, false); + REQUIRE(c.modifications.empty()); + } + + SECTION("marks newly added rows as insertions") { + c = _impl::CollectionChangeBuilder::calculate({2, 3}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.insertions, 0); + + c = _impl::CollectionChangeBuilder::calculate({1, 3}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.insertions, 1); + + c = _impl::CollectionChangeBuilder::calculate({1, 2}, {1, 2, 3}, all_modified, false); + REQUIRE_INDICES(c.insertions, 2); + } + + SECTION("marks removed rows as deleted") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 2}, all_modified, false); + REQUIRE_INDICES(c.deletions, 2); + + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3}, all_modified, false); + REQUIRE_INDICES(c.deletions, 1); + + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3}, all_modified, false); + REQUIRE_INDICES(c.deletions, 0); + } + + SECTION("marks rows as both inserted and deleted") { + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 4}, all_modified, false); + REQUIRE_INDICES(c.deletions, 1); + REQUIRE_INDICES(c.insertions, 2); + REQUIRE(c.moves.empty()); + } + + SECTION("marks rows as modified even if they moved") { + c = _impl::CollectionChangeBuilder::calculate({3, 5}, {5, 3}, all_modified, false); + REQUIRE_INDICES(c.deletions, 1); + REQUIRE_INDICES(c.insertions, 0); + REQUIRE_INDICES(c.modifications, 0, 1); + } + + SECTION("does not mark rows as modified if they are new") { + c = _impl::CollectionChangeBuilder::calculate({3}, {3, 5}, all_modified, false); + REQUIRE_INDICES(c.modifications, 0); + } + SECTION("reports inserts/deletes for simple reorderings") { auto calc = [&](std::vector old_rows, std::vector new_rows) { - return _impl::CollectionChangeBuilder::calculate(old_rows, new_rows, none_modified, true); + return _impl::CollectionChangeBuilder::calculate(old_rows, new_rows, none_modified, false); }; c = calc({1, 2, 3}, {1, 2, 3}); @@ -652,19 +652,19 @@ TEST_CASE("[collection_change] calculate() sorted") { SECTION("prefers to produce diffs where modified rows are the ones to move when it is ambiguous") { auto two_modified = [](size_t ndx) { return ndx == 2; }; - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, two_modified, true); + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, two_modified, false); REQUIRE_INDICES(c.deletions, 1); REQUIRE_INDICES(c.insertions, 2); auto three_modified = [](size_t ndx) { return ndx == 3; }; - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, three_modified, true); + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {1, 3, 2}, three_modified, false); REQUIRE_INDICES(c.deletions, 2); REQUIRE_INDICES(c.insertions, 1); } SECTION("prefers smaller diffs over larger diffs moving only modified rows") { auto two_modified = [](size_t ndx) { return ndx == 2; }; - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3, 1}, two_modified, true); + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, {2, 3, 1}, two_modified, false); REQUIRE_INDICES(c.deletions, 0); REQUIRE_INDICES(c.insertions, 2); } @@ -672,7 +672,7 @@ TEST_CASE("[collection_change] calculate() sorted") { SECTION("supports duplicate indices") { c = _impl::CollectionChangeBuilder::calculate({1, 1, 2, 2, 3, 3}, {1, 2, 3, 1, 2, 3}, - all_modified, true); + all_modified, false); REQUIRE_INDICES(c.deletions, 3, 5); REQUIRE_INDICES(c.insertions, 1, 2); } @@ -680,7 +680,7 @@ TEST_CASE("[collection_change] calculate() sorted") { SECTION("deletes and inserts the last option when any in a range could be deleted") { c = _impl::CollectionChangeBuilder::calculate({3, 2, 1, 1, 2, 3}, {1, 1, 2, 2, 3, 3}, - all_modified, true); + all_modified, false); REQUIRE_INDICES(c.deletions, 0, 1); REQUIRE_INDICES(c.insertions, 3, 5); } @@ -688,13 +688,13 @@ TEST_CASE("[collection_change] calculate() sorted") { SECTION("reports insertions/deletions when the number of duplicate entries changes") { c = _impl::CollectionChangeBuilder::calculate({1, 1, 1, 1, 2, 3}, {1, 2, 3, 1}, - all_modified, true); + all_modified, false); REQUIRE_INDICES(c.deletions, 1, 2, 3); REQUIRE_INDICES(c.insertions, 3); c = _impl::CollectionChangeBuilder::calculate({1, 2, 3, 1}, {1, 1, 1, 1, 2, 3}, - all_modified, true); + all_modified, false); REQUIRE_INDICES(c.deletions, 3); REQUIRE_INDICES(c.insertions, 1, 2, 3); } @@ -702,7 +702,7 @@ TEST_CASE("[collection_change] calculate() sorted") { SECTION("properly recurses into smaller subblocks") { std::vector prev = {10, 1, 2, 11, 3, 4, 5, 12, 6, 7, 13}; std::vector next = {13, 1, 2, 12, 3, 4, 5, 11, 6, 7, 10}; - c = _impl::CollectionChangeBuilder::calculate(prev, next, all_modified, true); + c = _impl::CollectionChangeBuilder::calculate(prev, next, all_modified, false); REQUIRE_INDICES(c.deletions, 0, 3, 7, 10); REQUIRE_INDICES(c.insertions, 0, 3, 7, 10); } @@ -718,13 +718,13 @@ TEST_CASE("[collection_change] calculate() sorted") { std::vector after_insert = {1, 2, 3}; after_insert.insert(after_insert.begin() + insert_pos, 4); - c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, after_insert, four_modified, true); + c = _impl::CollectionChangeBuilder::calculate({1, 2, 3}, after_insert, four_modified, false); std::vector after_move = {1, 2, 3}; after_move.insert(after_move.begin() + move_to_pos, 4); - c.merge(_impl::CollectionChangeBuilder::calculate(after_insert, after_move, four_modified, true)); + c.merge(_impl::CollectionChangeBuilder::calculate(after_insert, after_move, four_modified, false)); - c.merge(_impl::CollectionChangeBuilder::calculate(after_move, {1, 2, 3}, four_modified, true)); + c.merge(_impl::CollectionChangeBuilder::calculate(after_move, {1, 2, 3}, four_modified, false)); REQUIRE(c.empty()); } }