diff --git a/include/session/client.hpp b/include/session/client.hpp index ba543d7c..e7ec0008 100644 --- a/include/session/client.hpp +++ b/include/session/client.hpp @@ -1204,7 +1204,11 @@ class Client { void _emit_conversation_added(const ConversationId& id); void _emit_conversation_removed(const ConversationId& id); - void _emit_lists_replaced(); + // Reports both lists as wholly changed -- a row added, removed, or moved to a new position -- + // through whichever handlers the subscriber registered. A replacement carries the order, so + // this cannot send one without also considering the order event, or a subscriber holding only + // that handler hears nothing. + void _report_lists_replaced(bool convos, bool requests); void _emit_history_replaced(const ConversationId& id); // Reports a message, and then reports every message that replies to it. // @@ -1231,6 +1235,26 @@ class Client { bool _flush_scheduled = false; void _touch(const ConversationId& id); void _flush_pending(); + // Reports one list, if a row in it changed and the subscriber asked for it. The rows are the + // expensive part, so nothing is read for a handler that is not there. + // Which list each conversation was last *reported* in. The subscriber's belief rather than + // the database's state, which is the point: it is what the subscriber has to take the row out + // of before putting it where it now belongs. + // + // Keyed by ConversationId and not by row id, because a removal is reported *after* the row is + // deleted -- `_delete_contact` commits the DELETE first -- so there is nothing left to look a + // row id up from, and an entry keyed that way could never be erased. + std::unordered_map _placed; + // Where a conversation was and where it belongs now, and records the latter. + ListPlacement _place(const AnyConversation& convo); + void _report_list( + bool changed, + ConversationList list_kind, + std::vector (Client::*rows)(), + std::function&&)> callbacks::* replaced); + // Reports both lists, given for each whether a row in it changed. Called by `_flush_pending`, + // once per batch. + void _report_lists(bool convos_changed, bool requests_changed); public: /// The account state this Client is built on: keys, device group, configs, polling. A diff --git a/include/session/client/callbacks.hpp b/include/session/client/callbacks.hpp index 4a6316d4..18dbfc5f 100644 --- a/include/session/client/callbacks.hpp +++ b/include/session/client/callbacks.hpp @@ -11,6 +11,41 @@ #include namespace session::client { +/// Which of the two lists a conversation sits in. +/// +/// `none` is a real answer rather than a missing one: a hidden conversation is in neither list, and +/// so is one the subscriber has not been shown. +enum class ConversationList { + none, + conversations, + requests, +}; + +/// Where a conversation was, and where it belongs now. +/// +/// Enough to apply on its own, and it reads as the two steps it is: +/// +/// if (p.from != ConversationList::none) remove(p.from, convo.id()); +/// if (p.to != ConversationList::none) insert(p.to, std::move(convo), p.after); +/// +/// `from` is the list the subscriber was last *told* this row was in, which is what it is holding +/// rather than what the database now says. It saves searching the list the row did not come from; +/// it does not save finding the row, which is a lookup by id either way. +/// +/// The two lists are ordered differently -- conversations by `priority DESC, last_activity DESC, +/// id` and requests by `last_activity DESC, id`, with no priority term -- so which list a position +/// is in is part of the position rather than a detail. +struct ListPlacement { + /// Where the subscriber is holding this row, so it knows which list to take it out of. + /// `none` when it is holding it nowhere: a row it has not been shown, or one that was hidden. + ConversationList from = ConversationList::none; + /// Where it belongs now. `none` means neither list, which is what hiding does -- then it is + /// only removed. + ConversationList to = ConversationList::none; + /// The row it now follows in `to`; unset means first in that list. Meaningless, and always + /// unset, when `to` is `none`. + std::optional after; +}; /// Notifications of everything the conversation layer changes, so that an application never has to /// ask. A caller sets the handlers it cares about and leaves the rest empty; an unset handler is @@ -39,21 +74,37 @@ namespace session::client { /// is not a template on its argument — it is a promise by the caller that the object is spent /// afterwards. /// -/// The conversation list an application maintains from these is expected to be *complete*: ordering -/// is a comparison against every other conversation, so a partial list cannot be sorted. Showing -/// only part of it is fine, holding only part of it is not. +/// The conversation list an application maintains from these is expected to be *complete*: the +/// order is given as a whole list, so a partial one cannot be placed in it. Showing only part of +/// it is fine, holding only part of it is not. +/// +/// The order itself is **ours, not the application's**. Every handler that carries a list carries +/// it already ordered, so an application never has to sort, and should not: the two lists are not +/// sorted the same way and a comparator copied from one gets the other wrong. struct callbacks { /// A conversation now exists that did not before. - std::function conversation_added; + /// + /// Carries where it belongs, on the same terms as `conversation_updated`: `from` is normally + /// `none`, since a row that did not exist was not being held anywhere. + std::function conversation_added; /// A conversation's contents changed: a new or edited message, a name, an unread count, its /// last activity. Fired once with the conversation's settled state rather than once per /// underlying change, so a poll that delivers fifty messages to one conversation fires this /// once. - std::function conversation_updated; + /// + /// The second argument says where the row was and where it belongs now, which is enough to + /// apply without consulting anything: remove it from `from`, insert it into `to`. + /// + /// **Applying these in order is what keeps a list correct.** Each one places a row relative to + /// another, so one applied out of order, or skipped, leaves the list wrong with nothing to + /// detect it. A subscriber that has not read the list once with `conversations()` has nothing + /// to place rows into. + std::function conversation_updated; - /// A conversation is gone and should be dropped from the list. - std::function conversation_removed; + /// A conversation is gone and should be dropped from the list it is in, which is the second + /// argument -- `none` if it was never shown in one. Saves searching both. + std::function conversation_removed; /// Priorities changed — a pin, unpin, hide or unhide — carrying the whole list in its new /// order. A replacement rather than a description of what moved, because one config update @@ -71,6 +122,7 @@ struct callbacks { /// belongs to — and `Conversation::request` is what says which one a given handler is about. std::function&&)> request_list_replaced; + /// A message was added, whether received or sent from here. std::function message_added; diff --git a/src/client/client.cpp b/src/client/client.cpp index 6cfb4172..f87029ca 100644 --- a/src/client/client.cpp +++ b/src/client/client.cpp @@ -473,22 +473,37 @@ void Client::_dispatch_out(std::function job) { } void Client::_emit_conversation_added(const ConversationId& id) { + if (!_cbs->conversation_added) + return; auto convo = _conversation(id); if (!convo) return; + // Placed here rather than left to the `conversation_updated` that follows: an add that says + // where the row goes is applicable on its own, and the alternative is a guarantee about the + // order of two callbacks that nothing enforces. + auto placement = _place(*convo); // Mutable so the value moves out: each _emit job runs once, and the handler owns what it gets. - _emit([convo = std::move(*convo)](const callbacks& cbs) mutable { + _emit([convo = std::move(*convo), + placement = std::move(placement)](const callbacks& cbs) mutable { if (cbs.conversation_added) - cbs.conversation_added(std::move(convo)); + cbs.conversation_added(std::move(convo), std::move(placement)); }); } void Client::_emit_conversation_removed(const ConversationId& id) { + // Where the subscriber is holding it, so it knows which list to take it out of; `none` if it + // was never shown one. Read and then dropped -- keeping it would leak an entry per + // conversation ever deleted, and the row is already gone from the database by now. + auto from = ConversationList::none; + if (auto held = _placed.find(id); held != _placed.end()) { + from = held->second; + _placed.erase(held); + } // `id = id` rather than `id`: a copy-capture of a const lvalue is itself const, which `mutable` // does not undo, and the handler is given the id outright. - _emit([id = id](const callbacks& cbs) mutable { + _emit([id = id, from](const callbacks& cbs) mutable { if (cbs.conversation_removed) - cbs.conversation_removed(std::move(id)); + cbs.conversation_removed(std::move(id), from); }); } @@ -500,6 +515,8 @@ void Client::_emit_history_replaced(const ConversationId& id) { } void Client::_emit_message_alone(bool added, const ConversationId& id, int64_t message_id) { + if (!(added ? _cbs->message_added : _cbs->message_updated)) + return; auto msg = _message(message_id); if (!msg) return; @@ -548,20 +565,64 @@ void Client::_touch(const ConversationId& id) { } } + void Client::_flush_pending() { _flush_scheduled = false; auto dirty = std::move(_dirty); _dirty.clear(); + // Which list each dirty row sits in, read off the row this loop already fetches rather than by + // asking again: only a DM can be a request, and the two lists are complements, so each row + // belongs to exactly one of them -- or, hidden, to neither. + // + // Two questions per list, and they are not the same question. A row whose *contents* changed + // makes the list stale for a subscriber holding whole lists, and most of what dirties a + // conversation does that: a read receipt, a nickname, an expiry. A row that *moved* is the + // narrower case, and only that one can change the order. + bool convos_changed = false, requests_changed = false; + for (const auto& id : dirty) { auto convo = _conversation(id); if (!convo) continue; - _emit([convo = std::move(*convo)](const callbacks& cbs) mutable { + // A hidden row is in neither list, so nothing about it makes either one stale. + // + // This reads the row's priority now, so it cannot tell a row that was always hidden from + // one that has just become hidden -- and the second of those did change both lists, by + // leaving one of them. That case does not arrive here: every write to + // `conversations.priority` emits a replacement on its own path rather than dirtying the + // row and leaving this to report it, so the removal has already been sent by the time the + // row turns up in `_dirty`. Should a fifth writer of that column ever appear, it has to do + // the same, because there is nothing here that could notice the transition. + if (convo->priority() >= 0) { + auto* dm = convo->dm(); + const bool request = dm && dm->request; + (request ? requests_changed : convos_changed) = true; + } + // Read before the emit, and off the row this loop already fetched: the anchor is one + // indexed seek, against reading the whole list to say the same thing. + auto placement = _place(*convo); + _emit([convo = std::move(*convo), + placement = std::move(placement)](const callbacks& cbs) mutable { if (cbs.conversation_updated) - cbs.conversation_updated(std::move(convo)); + cbs.conversation_updated(std::move(convo), std::move(placement)); }); } + + // And then the lists, each through whichever handler asked for it. + // + // A `conversation_updated` carries its row's position, so a subscriber applying those in order + // keeps its lists arranged without ever sorting -- which it could not do correctly anyway, + // since the two lists are not sorted the same way. + // + // Deliberately after that loop rather than before it, which is the guarantee + // `conversation_order_updated` documents: the ids reported here always name conversations the + // subscriber has already been told about, so an id it does not recognise means it missed a + // notification rather than that the two are racing. + // + // Once for the batch, not once per row: a poll delivering fifty messages to one conversation + // reaches here a single time, which is the same reason `_dirty` exists. + _report_lists(convos_changed, requests_changed); } // -- Asynchronous interface --------------------------------------------------------------------- @@ -1315,6 +1376,39 @@ static const auto CONVO_COLUMNS = R"( static constexpr auto IS_REQUEST = "(c.dm IS NOT NULL AND coalesce(ct.approved, 0) = 0 AND a.session_id IS NOT ?1)"sv; +// One fragment per list, shared by both queries that read that list: the one that reads the rows +// and the one that reads only their order. The two have to select and sort identically -- a +// subscriber arranging rows from an order event and one just handed a replacement must arrive at +// the same list -- and nothing would report them drifting apart. +static const auto CONVO_FILTER_ORDER = + // Hidden (negative priority) conversations are not part of the list at all; pinned ones + // lead it, and equal priorities form a block that sorts among itself by recency. + "WHERE c.priority >= 0 AND NOT {} ORDER BY c.priority DESC, c.last_activity DESC, c.id"_format( + IS_REQUEST); +static const auto REQUEST_FILTER_ORDER = + // No priority in the ordering: a request cannot be pinned, since pinning is a property of + // the config entry and there is nothing there to pin until it is approved. Hidden ones are + // still omitted -- hiding is the one thing another device *can* say about a request it does + // not want to see. + "WHERE c.priority >= 0 AND {} ORDER BY c.last_activity DESC, c.id"_format(IS_REQUEST); + +// The row a given one now follows, per list: each list's ordering reversed, taking the first row +// that sorts before it. Must stay in step with the two fragments above -- an anchor read in a +// different order than the list is sorted in places the row in the wrong gap. +// +// One indexed seek against `conversations_order` rather than reading the list: measured at 0.11 ms +// where reading the ordered ids is 1.85 ms and the rows 4.85 ms, at five thousand conversations. +// +// Binds are the self id, then the subject row's own sort key. +static const auto CONVO_ANCHOR = + "WHERE c.priority >= 0 AND NOT {} AND (c.priority > ?2 OR (c.priority = ?2 AND " + "(c.last_activity > ?3 OR (c.last_activity = ?3 AND c.id < ?4)))) " + "ORDER BY c.priority ASC, c.last_activity ASC, c.id DESC LIMIT 1"_format(IS_REQUEST); +static const auto REQUEST_ANCHOR = + "WHERE c.priority >= 0 AND {} AND " + "(c.last_activity > ?2 OR (c.last_activity = ?2 AND c.id < ?3)) " + "ORDER BY c.last_activity ASC, c.id DESC LIMIT 1"_format(IS_REQUEST); + // Fills in the attachment side of the `last_preview` of every conversation that has one. // `previews` pairs the previewed message with the index of the conversation it belongs to. // @@ -1492,32 +1586,28 @@ std::span Client::_self_or_none() { return core.globals.session_id(); } +// Which list a conversation is in, for a caller that has changed one row and needs to name the list +// that made stale rather than both. One indexed row, against the list query it saves. +static bool is_request_row(sqlite::Connection& c, int64_t convo, std::span self) { + return c.prepared_get( + "SELECT {} {} WHERE c.id = ?2"_format(IS_REQUEST, SUBJECT_JOIN), self, convo) != + 0; +} + std::vector Client::_conversations() { auto c = core.database().conn(); return query_conversations( - *this, - c, - // Hidden (negative priority) conversations are not part of the list at all; pinned ones - // lead it, and equal priorities form a block that sorts among itself by recency. - "{} WHERE c.priority >= 0 AND NOT {} ORDER BY c.priority DESC, c.last_activity DESC, c.id"_format( - CONVO_COLUMNS, IS_REQUEST), - _self_or_none()); + *this, c, "{} {}"_format(CONVO_COLUMNS, CONVO_FILTER_ORDER), _self_or_none()); } std::vector Client::_message_requests() { auto c = core.database().conn(); - // No priority ordering: a request cannot be pinned -- pinning is a property of the config entry - // and there is nothing there to pin until it is approved -- so recency is the only order there - // is. Hidden ones are still omitted, since hiding is the one thing another device *can* say - // about a request it does not want to see. return query_conversations( - *this, - c, - "{} WHERE c.priority >= 0 AND {} ORDER BY c.last_activity DESC, c.id"_format( - CONVO_COLUMNS, IS_REQUEST), - _self_or_none()); + *this, c, "{} {}"_format(CONVO_COLUMNS, REQUEST_FILTER_ORDER), _self_or_none()); } + + std::optional Client::_conversation(const ConversationId& id) { auto c = core.database().conn(); auto convo = find_conversation(c, id); @@ -1747,6 +1837,7 @@ void Client::_set_nickname(const ConversationId& id, std::string_view nickname) void Client::_set_priority(const ConversationId& id, int priority) { int changed = 0; + bool request = false; { auto c = core.database().conn(); SQLite::Transaction tx{c.sql}; @@ -1760,12 +1851,17 @@ void Client::_set_priority(const ConversationId& id, int priority) { "UPDATE conversations SET priority = ?1 WHERE id = ?2 AND priority IS NOT ?1", priority, convo); + // Read inside the transaction that changed it, and only when it did: the row stays in + // whichever list it was in -- priority moves it within one, never between the two -- so the + // other list has not changed and does not need reading. + if (changed > 0) + request = is_request_row(c, convo, _self_or_none()); tx.commit(); } if (changed > 0) { _sync_conversation(id); - _emit_lists_replaced(); + _report_lists_replaced(!request, request); } } @@ -1821,7 +1917,7 @@ void Client::_clear_messages(const ConversationId& id) { void Client::_delete_conversation(const ConversationId& id, bool keep_messages) { auto now = clock_now_ms(); - bool emptied = false, hidden = false; + bool emptied = false, hidden = false, request = false; { auto c = core.database().conn(); SQLite::Transaction tx{c.sql}; @@ -1840,6 +1936,10 @@ void Client::_delete_conversation(const ConversationId& id, bool keep_messages) hidden = c.prepared_exec( "UPDATE conversations SET priority = -1 WHERE id = ? AND priority >= 0", *convo) > 0; + // While the row is still readable and only when it went: hiding takes it out of the one + // list it was in, so the other has not changed. + if (hidden) + request = is_request_row(c, *convo, _self_or_none()); tx.commit(); } @@ -1850,11 +1950,11 @@ void Client::_delete_conversation(const ConversationId& id, bool keep_messages) if (emptied) _emit_history_replaced(id); if (hidden) - _emit_lists_replaced(); + _report_lists_replaced(!request, request); } void Client::_delete_contact(const ConversationId& id) { - bool removed = false; + bool removed = false, was_request = false, was_listed = false; { auto c = core.database().conn(); SQLite::Transaction tx{c.sql}; @@ -1864,6 +1964,18 @@ void Client::_delete_contact(const ConversationId& id) { if (!account) return; + // Which list it is in, before either delete below. Not merely before the conversation + // goes: whether it is a request is read from `ct.approved`, which is a column of the + // contact row that is about to be deleted, so asking afterwards answers about a + // relationship that no longer exists and calls every deleted conversation a request. + // + // A hidden one is in neither list, so its going changes neither. + if (auto convo = c.prepared_maybe_get( + "SELECT id FROM conversations WHERE dm = ? AND priority >= 0", *account)) { + was_listed = true; + was_request = is_request_row(c, *convo, _self_or_none()); + } + // The nickname, both approvals and the block are columns of the row being deleted, so // there is nothing to reset first: they exist only for as long as the relationship does. c.prepared_exec("DELETE FROM contacts WHERE account = ?", *account); @@ -1881,7 +1993,9 @@ void Client::_delete_contact(const ConversationId& id) { if (removed) { _emit_conversation_removed(id); - _emit_lists_replaced(); + // The one list it was in. A conversation is in exactly one of the two, so deleting it + // outright leaves the other exactly as it was. + _report_lists_replaced(was_listed && !was_request, was_listed && was_request); } } @@ -2252,20 +2366,94 @@ void Client::_set_delete_before(const ConversationId& id, sys_ms before) { contacts.set(*entry); } +ListPlacement Client::_place(const AnyConversation& convo) { + ListPlacement out; + + auto c = core.database().conn(); + auto row = find_conversation(c, convo.id()); + if (!row) + return out; + + // Where the subscriber is holding it, which is what it has to take the row out of -- not what + // the database says now, which is where the row is going. A row it has never been told about + // has no entry, and nothing to remove. + if (auto held = _placed.find(convo.id()); held != _placed.end()) + out.from = held->second; + + // Hidden is in neither list: taken out, and not put back. + if (convo.priority() < 0) { + _placed.erase(convo.id()); + return out; + } + + auto* dm = convo.dm(); + const bool request = dm && dm->request; + out.to = request ? ConversationList::requests : ConversationList::conversations; + _placed[convo.id()] = out.to; + + auto before = request ? c.prepared_maybe_get( + "SELECT c.id {} {}"_format(SUBJECT_JOIN, REQUEST_ANCHOR), + _self_or_none(), + epoch_ms(convo.last_activity()), + *row) + : c.prepared_maybe_get( + "SELECT c.id {} {}"_format(SUBJECT_JOIN, CONVO_ANCHOR), + _self_or_none(), + convo.priority(), + epoch_ms(convo.last_activity()), + *row); + // Nothing sorts before it, so it goes first -- which an unset anchor is what says. + if (before) + out.after = conversation_id_at(c, *before); + return out; +} + +void Client::_report_list( + bool changed, + ConversationList list_kind, + std::vector (Client::*rows)(), + std::function&&)> callbacks::* replaced) { + // Before the query, not after: reading a list to hand it to nobody is the whole cost of the + // operation, and a client with no requests screen has no use for the request list. + if (!changed || !((*_cbs).*replaced)) + return; + + auto list = (this->*rows)(); + // A replacement places every row it carries, so it is as much a report of where rows are as an + // update is. Without recording it here the next update would offer a `from` describing an + // older belief than the subscriber actually holds. + { + for (const auto& convo : list) + _placed[convo.id()] = list_kind; + } + + _emit([list = std::move(list), replaced](const callbacks& cbs) mutable { + (cbs.*replaced)(std::move(list)); + }); +} + +void Client::_report_lists(bool convos_changed, bool requests_changed) { + _report_list( + convos_changed, + ConversationList::conversations, + &Client::_conversations, + &callbacks::conversation_list_replaced); + _report_list( + requests_changed, + ConversationList::requests, + &Client::_message_requests, + &callbacks::request_list_replaced); +} + // Both lists, always, and deliberately not one or the other: what moves a conversation between them // is approval, what removes it from either is hiding or deletion, and a caller that had to work out // which of those it just did would eventually get it wrong. A replacement is idempotent, so the // cost of sending one nobody needed is a query. -void Client::_emit_lists_replaced() { - auto convos = _conversations(); - auto requests = _message_requests(); - _emit([convos = std::move(convos), - requests = std::move(requests)](const callbacks& cbs) mutable { - if (cbs.conversation_list_replaced) - cbs.conversation_list_replaced(std::move(convos)); - if (cbs.request_list_replaced) - cbs.request_list_replaced(std::move(requests)); - }); +void Client::_report_lists_replaced(bool convos, bool requests) { + // A row appeared, went, or moved to a new position. Named for what the caller knows -- that a + // list is wholly different now -- rather than for the query, which is the one a changed row + // takes as well. + _report_lists(convos, requests); } // -- Config reconciliation ---------------------------------------------------------------------- @@ -2654,8 +2842,11 @@ WHERE id = ?1 _touch(id); for (const auto& id : removed) _emit_conversation_removed(id); + // Each list only if something in it changed. A removal can be from either -- what was taken + // out is gone from whichever list held it -- so it names both. if (order_changed || requests_changed || !removed.empty()) - _emit_lists_replaced(); + _report_lists_replaced( + order_changed || !removed.empty(), requests_changed || !removed.empty()); } void Client::_sync_all_contacts() { @@ -3009,8 +3200,10 @@ WHERE id = ?1 AND (exp_mode, exp_timer) IS NOT (?2, ?3) _touch(me); if (history_changed) _emit_history_replaced(me); + // Note to self only, and it cannot be a request -- we are not our own contact -- so the request + // list cannot have been touched by this. if (order_changed) - _emit_lists_replaced(); + _report_lists_replaced(true, false); } // -- Messages --------------------------------------------------------------------------------- @@ -3498,7 +3691,9 @@ int64_t Client::_send_message(const ConversationId& id, const OutgoingMessage& m if (approved) { _sync_contact(id); - _emit_lists_replaced(); + // Both: approving moves the row out of the requests and into the conversations, so one list + // lost it and the other gained it. + _report_lists_replaced(true, true); } if (created) _emit_conversation_added(id); @@ -3690,7 +3885,9 @@ int64_t Client::_send_message( if (approved) { _sync_contact(id); - _emit_lists_replaced(); + // Both: approving moves the row out of the requests and into the conversations, so one list + // lost it and the other gained it. + _report_lists_replaced(true, true); } if (created) _emit_conversation_added(id); @@ -5101,7 +5298,7 @@ void Client::_on_message_received(core::ReceivedMessage&& msg) { // Approval moves a conversation between the two lists, so both changed and neither changed in a // way that naming one row would describe. if (approved_them) - _emit_lists_replaced(); + _report_lists_replaced(true, true); } void Client::_on_send_status( diff --git a/tests/test_client/common.hpp b/tests/test_client/common.hpp index 2c2e4d37..cc501506 100644 --- a/tests/test_client/common.hpp +++ b/tests/test_client/common.hpp @@ -173,25 +173,32 @@ struct Recorder { std::vector order; std::vector added, updated; std::vector removed; + std::vector removed_from; std::vector> replaced, requests_replaced; std::vector> msg_added, msg_updated; + /// Where each event said its row was and now belongs, kept apart so a test can say which + /// callback it means. + std::vector add_placements, placements; callbacks handlers() { return { .conversation_added = - [this](AnyConversation&& c) { + [this](AnyConversation&& c, ListPlacement&& p) { order.push_back("added"); added.push_back(std::move(c)); + add_placements.push_back(std::move(p)); }, .conversation_updated = - [this](AnyConversation&& c) { + [this](AnyConversation&& c, ListPlacement&& p) { order.push_back("updated"); updated.push_back(std::move(c)); + placements.push_back(std::move(p)); }, .conversation_removed = - [this](const ConversationId& id) { + [this](ConversationId&& id, ConversationList from) { order.push_back("removed"); - removed.push_back(id); + removed.push_back(std::move(id)); + removed_from.push_back(from); }, .conversation_list_replaced = [this](std::vector&& l) { diff --git a/tests/test_client/configs.cpp b/tests/test_client/configs.cpp index f9822dea..ff8c1e9f 100644 --- a/tests/test_client/configs.cpp +++ b/tests/test_client/configs.cpp @@ -72,7 +72,9 @@ TEST_CASE("Client: re-deriving a contact changes nothing", "[client][configs]") TEST_CASE("Client: a contact removed elsewhere takes its history", "[client][configs]") { std::vector gone; callbacks cbs; - cbs.conversation_removed = [&](const ConversationId& id) { gone.push_back(id); }; + cbs.conversation_removed = [&](ConversationId&& id, ConversationList) { + gone.push_back(std::move(id)); + }; TempClient c{cbs}; auto them = "05" + std::string(64, 'c'); @@ -395,7 +397,9 @@ TEST_CASE("Client: hiding note to self keeps what is in it", "[client][configs]" TEST_CASE("Client: deleting a contact takes the entry that held the block", "[client][configs]") { std::vector gone; callbacks cbs; - cbs.conversation_removed = [&](const ConversationId& id) { gone.push_back(id); }; + cbs.conversation_removed = [&](ConversationId&& id, ConversationList) { + gone.push_back(std::move(id)); + }; TempClient c{cbs}; auto them = "05" + std::string(64, '4'); diff --git a/tests/test_client/sending.cpp b/tests/test_client/sending.cpp index 5bbd95f8..24d7ec09 100644 --- a/tests/test_client/sending.cpp +++ b/tests/test_client/sending.cpp @@ -110,7 +110,12 @@ TEST_CASE("Client: the application is told what changed", "[client][signals]") { sync(*c); auto convo = ConversationId::dm(sender.session_id); - CHECK(r.order == std::vector{"added", "message", "updated"}); + // The request list and not the conversation list: an inbound message from someone we have not + // written to is a request, and the two lists are complements, so only the one it sits in is + // stale. A conversation-list replacement here would say something false about the other. + CHECK(r.order == std::vector{"added", "message", "updated", "requests"}); + REQUIRE(r.requests_replaced.size() == 1); + CHECK(r.replaced.empty()); // Every handler is given the state itself, not something to go and look up. REQUIRE(r.added.size() == 1); @@ -122,11 +127,92 @@ TEST_CASE("Client: the application is told what changed", "[client][signals]") { CHECK(preview_body(r.updated[0]) == "ping"); CHECK(r.updated[0].unread() == 1); - // A second message on an existing conversation does not re-announce the conversation. + // A second message on an existing conversation does not re-announce the conversation, but the + // list is stale again -- the row's snippet changed, and the list is what carries it for a + // subscriber that holds no per-row handler. r.order.clear(); + r.requests_replaced.clear(); deliver(*c, sender, "pong", from_epoch_ms(2000), "h2"); sync(*c); - CHECK(r.order == std::vector{"message", "updated"}); + CHECK(r.order == std::vector{"message", "updated", "requests"}); + CHECK(r.requests_replaced.size() == 1); + REQUIRE(r.updated.size() == 2); + CHECK(preview_body(r.updated.back()) == "pong"); +} + +TEST_CASE("Client: a subscriber that wants lists is sent them", "[client][signals]") { + SenderKeys a, b; + Recorder r; + // No order handlers, so the order events have nowhere to go and a whole list is the only way + // this subscriber can learn that one row now sits in front of another. + TempClient c{r.handlers()}; + approve(*c, a.session_id); + approve(*c, b.session_id); + auto ida = ConversationId::dm(a.session_id); + auto idb = ConversationId::dm(b.session_id); + + deliver(*c, a, "first", from_epoch_ms(1000), "h1"); + deliver(*c, b, "second", from_epoch_ms(2000), "h2"); + sync(*c); + r.order.clear(); + r.replaced.clear(); + + deliver(*c, a, "third", from_epoch_ms(3000), "h3"); + sync(*c); + + CHECK(r.order == std::vector{"message", "updated", "replaced"}); + REQUIRE(r.replaced.size() == 1); + REQUIRE(r.replaced[0].size() == 2); + CHECK(r.replaced[0][0].id() == ida); + CHECK(r.replaced[0][1].id() == idb); +} + +TEST_CASE("Client: a replacement is sent even when nothing moved", "[client][signals]") { + SenderKeys sender; + Recorder r; + TempClient c{r.handlers()}; + approve(*c, sender.session_id); + + deliver(*c, sender, "ping", from_epoch_ms(1000), "h1"); + sync(*c); + r.order.clear(); + r.replaced.clear(); + + // A second message to the only conversation moves nothing, so an order event would be + // suppressed -- but the snippet changed, and for this subscriber the list is the only thing + // carrying it. Suppressing the list on an unchanged order would leave it showing "ping". + deliver(*c, sender, "pong", from_epoch_ms(2000), "h2"); + sync(*c); + + REQUIRE(r.replaced.size() == 1); + REQUIRE(r.replaced[0].size() == 1); + CHECK(preview_body(r.replaced[0][0]) == "pong"); +} + +TEST_CASE("Client: a change that moves nothing still replaces the list", "[client][signals]") { + SenderKeys sender; + Recorder r; + TempClient c{r.handlers()}; + approve(*c, sender.session_id); + auto id = ConversationId::dm(sender.session_id); + + deliver(*c, sender, "ping", from_epoch_ms(1000), "h1"); + sync(*c); + REQUIRE(r.replaced.size() >= 1); + CHECK(r.replaced.back()[0].unread() == 1); + r.order.clear(); + r.replaced.clear(); + + // Reading the conversation changes `unread_count` and moves nothing, so it never reaches + // `_dirty_order`. For this subscriber the list is the only thing carrying the count, so + // driving the replacement off what *moved* rather than off what *changed* would leave it + // showing an unread conversation the user has just read. + c->conversation(id, wait)->mark_read(wait); + sync(*c); + + REQUIRE(r.replaced.size() == 1); + REQUIRE(r.replaced[0].size() == 1); + CHECK(r.replaced[0][0].unread() == 0); } TEST_CASE( @@ -175,6 +261,109 @@ TEST_CASE( CHECK(preview_body(r.updated[0]) == "m4"); } +TEST_CASE("Client: an update says where its row was and now belongs", "[client][signals]") { + SenderKeys a, b; + Recorder r; + TempClient c{r.handlers()}; + approve(*c, a.session_id); + approve(*c, b.session_id); + auto ida = ConversationId::dm(a.session_id); + auto idb = ConversationId::dm(b.session_id); + + deliver(*c, a, "first", from_epoch_ms(1000), "h1"); + sync(*c); + + // The add already says where it goes: nothing to remove, into the conversation list, and + // nothing above it -- which an unset anchor is what says. + REQUIRE(r.add_placements.size() == 1); + CHECK(r.add_placements[0].from == ConversationList::none); + CHECK(r.add_placements[0].to == ConversationList::conversations); + CHECK_FALSE(r.add_placements[0].after.has_value()); + + // The update that follows it agrees, and now knows where the add put it. + REQUIRE(r.placements.size() == 1); + CHECK(r.placements[0].from == ConversationList::conversations); + CHECK(r.placements[0].to == ConversationList::conversations); + + // Pinning a puts it above everything unpinned, so the next row to move sits after it rather + // than at the top -- which is the case an anchor exists to express and a bare "moved to front" + // could not. + c->conversation(ida, wait)->set_priority(3, wait); + r.placements.clear(); + + deliver(*c, b, "second", from_epoch_ms(2000), "h2"); + sync(*c); + + REQUIRE(r.placements.size() == 1); + CHECK(r.placements[0].from == ConversationList::conversations); + CHECK(r.placements[0].to == ConversationList::conversations); + REQUIRE(r.placements[0].after.has_value()); + CHECK(*r.placements[0].after == ida); + CHECK(r.updated.back().id() == idb); +} + +TEST_CASE("Client: a hidden row is given no position", "[client][signals]") { + SenderKeys sender; + Recorder r; + TempClient c{r.handlers()}; + approve(*c, sender.session_id); + auto id = ConversationId::dm(sender.session_id); + + deliver(*c, sender, "ping", from_epoch_ms(1000), "h1"); + sync(*c); + c->conversation(id, wait)->set_priority(-1, wait); + r.placements.clear(); + + // Hidden is in neither list, so there is no gap to name. Unset here means "do not place + // this", not "place it first" -- which is why the two are different states. + deliver(*c, sender, "pong", from_epoch_ms(2000), "h2"); + sync(*c); + + // Taken out of where it was and not put back: `to == none` is what hiding looks like, and + // `from` still names the list it has to come out of. + REQUIRE(r.placements.size() == 1); + CHECK(r.placements[0].from == ConversationList::conversations); + CHECK(r.placements[0].to == ConversationList::none); +} + +TEST_CASE("Client: a request is placed in the request list", "[client][signals]") { + SenderKeys stranger; + Recorder r; + TempClient c{r.handlers()}; + + // Nobody approved: a stranger's first message is a request, so the position names that list. + deliver(*c, stranger, "hello", from_epoch_ms(1000), "h1"); + sync(*c); + + // The add places it in the request list; nothing held it before. + REQUIRE(r.add_placements.size() == 1); + CHECK(r.add_placements[0].from == ConversationList::none); + CHECK(r.add_placements[0].to == ConversationList::requests); + CHECK_FALSE(r.add_placements[0].after.has_value()); +} + +TEST_CASE("Client: a removal says which list to take it out of", "[client][signals]") { + SenderKeys sender; + Recorder r; + TempClient c{r.handlers()}; + approve(*c, sender.session_id); + auto id = ConversationId::dm(sender.session_id); + + deliver(*c, sender, "ping", from_epoch_ms(1000), "h1"); + sync(*c); + r.order.clear(); + + // Deleting the contact removes the conversation row outright. The list it was in has to come + // from what the subscriber was last told, not from the database -- by the time this is + // reported the row is already gone, so there is nothing left to look it up from. + c->dm(id, wait)->delete_contact(wait); + + REQUIRE(r.removed.size() == 1); + CHECK(r.removed[0] == id); + REQUIRE(r.removed_from.size() == 1); + CHECK(r.removed_from[0] == ConversationList::conversations); +} + TEST_CASE("Client: state is committed before the handler fires", "[client][signals]") { SenderKeys sender; std::optional body_seen_from_handler; @@ -299,9 +488,12 @@ TEST_CASE("Client: a priority change replaces the whole list", "[client][signals c->conversation(ConversationId::dm(a.session_id), wait)->set_priority(3, wait); // Reported as a replacement, not as an update to the one conversation whose priority changed: - // what moved is the list. Both lists are replaced together, because hiding takes a - // conversation out of whichever one it was in and the caller does not have to work out which. - CHECK(r.order == std::vector{"replaced", "requests"}); + // what moved is the list. And the order alongside it, for this subscriber holding both + // handlers: pinning moved every row that was above the pinned one. + // + // The conversation list only. Priority moves a row within the list it is in and never between + // the two, so the request list did not change and is not read. + CHECK(r.order == std::vector{"replaced"}); REQUIRE(r.replaced.size() == 1); REQUIRE(r.replaced[0].size() == 2); CHECK(r.replaced[0][0].id() == ConversationId::dm(a.session_id)); @@ -311,7 +503,7 @@ TEST_CASE("Client: a priority change replaces the whole list", "[client][signals r.order.clear(); r.replaced.clear(); c->conversation(ConversationId::dm(a.session_id), wait)->set_priority(-1, wait); - CHECK(r.order == std::vector{"replaced", "requests"}); + CHECK(r.order == std::vector{"replaced"}); REQUIRE(r.replaced.size() == 1); REQUIRE(r.replaced[0].size() == 1); CHECK(r.replaced[0][0].id() == ConversationId::dm(b.session_id));