diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/mainwindow.cpp | 132 | ||||
| -rw-r--r-- | src/mainwindow.h | 6 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 111 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 20 |
4 files changed, 204 insertions, 65 deletions
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index e93898c..4377870 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -44,6 +44,7 @@ #include <QPlainTextEdit> #include <QPushButton> #include <QRegularExpression> +#include <QScopedValueRollback> #include <QSettings> #include <QSplitter> #include <QStandardPaths> @@ -1011,6 +1012,11 @@ void MainWindow::buildUi() // the model rather than on each of the six call sites that apply an // optimistic update, so a new one cannot forget. connect(m_model, &QAbstractItemModel::dataChanged, this, [this]() { + // Inside batchModelWrites() the refresh runs ONCE at the end. Per + // row it is O(selection) for every row written, which made a bulk + // archive cubic and froze the window on ten thousand rows (item 211). + if (m_batchingModelWrites) + return; refreshUnreadAction(); refreshTrashActions(); }); @@ -4863,14 +4869,16 @@ void MainWindow::revertPendingTagChange() // Put the rows back the way they were. Only the model is touched: the // worker never applied the change, so there is nothing to undo there. - for (const QString &threadId : m_pendingThreadIds) { - m_model->applyTagChange(threadId, m_pendingChange.removed, - m_pendingChange.added); - } - for (const QString &messageId : m_pendingChange.messageIds) { - m_model->applyMessageTagChange(messageId, m_pendingChange.removed, - m_pendingChange.added); - } + batchModelWrites([this] { + for (const QString &threadId : m_pendingThreadIds) { + m_model->applyTagChange(threadId, m_pendingChange.removed, + m_pendingChange.added); + } + for (const QString &messageId : m_pendingChange.messageIds) { + m_model->applyMessageTagChange(messageId, m_pendingChange.removed, + m_pendingChange.added); + } + }); // The undo entry describes a change that never landed, so it would apply a // spurious inverse if the user pressed undo. @@ -4894,6 +4902,9 @@ void MainWindow::revertPendingTagChange() void MainWindow::onWorkerError(const QString &message) { + // The error is what the user needs to read, not a "done" still in flight. + ++m_announceTicket; + // Spec: the UI updates optimistically and reverts if the write fails. // Without this the list would keep showing a tag the database never got. // @@ -4950,14 +4961,16 @@ void MainWindow::flushHeldEdits() // rows do not visibly flicker; without this the change is applied // twice and a later revert undoes only one of them, leaving a row // showing a tag the database never got. - for (const QString &threadId : edit.threadIds) { - m_model->applyTagChange(threadId, edit.change.removed, - edit.change.added); - } - for (const QString &messageId : edit.change.messageIds) { - m_model->applyMessageTagChange(messageId, edit.change.removed, - edit.change.added); - } + batchModelWrites([&] { + for (const QString &threadId : edit.threadIds) { + m_model->applyTagChange(threadId, edit.change.removed, + edit.change.added); + } + for (const QString &messageId : edit.change.messageIds) { + m_model->applyMessageTagChange(messageId, edit.change.removed, + edit.change.added); + } + }); // By SCOPE. A held edit is one or the other, never both: a // message-scoped edit carries no thread ids, so sending it through @@ -5867,7 +5880,32 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) void MainWindow::announceAction(const QString &text) { if (!aSyncHoldsTheWriteLock()) { - showTransientStatus(text); + if (!m_worker) { + showTransientStatus(text); + return; + } + + // Item 211. Held until the write has actually landed, then replaced + // by a transient "done": a bulk write can take a while, and a status + // that vanished on a timer left no way to tell whether it finished. + // + // A round trip through the worker's queue rather than a hook on + // tagsApplied(), which deliberately stays silent for a write that + // moved nothing, and a move reports through another signal anyway. + // The worker runs its queue in order, so this lambda runs after the + // write the caller just queued, and its reply is posted behind that + // write's confirmation. The destructor waits on the worker thread + // before `this` goes, so the reply cannot outlive the window. + m_statusTimer->stop(); + m_statusLabel->setText(tr("%1, applying…").arg(text)); + const quint64 ticket = ++m_announceTicket; + QMetaObject::invokeMethod(m_worker, [this, text, ticket]() { + QMetaObject::invokeMethod(this, [this, text, ticket]() { + // A newer announcement, or an error, owns the label now. + if (ticket == m_announceTicket) + showTransientStatus(tr("%1, done").arg(text)); + }, Qt::QueuedConnection); + }, Qt::QueuedConnection); return; } @@ -6511,6 +6549,16 @@ void MainWindow::editTagsOnSelection() tagSelected(add, remove, tr("Edit tags")); } +void MainWindow::batchModelWrites(const std::function<void()> &writes) +{ + { + const QScopedValueRollback<bool> guard(m_batchingModelWrites, true); + writes(); + } + refreshUnreadAction(); + refreshTrashActions(); +} + void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, const QString &description) { @@ -6582,8 +6630,10 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, // this path had no optimistic update at all, and the cost was that Delete // and Toggle unread on a reply moved the pending count and changed nothing // the user could see. The reply's own row is where the feedback belongs. - for (const QString &messageId : messageIds) - m_model->applyMessageTagChange(messageId, add, remove); + batchModelWrites([&] { + for (const QString &messageId : messageIds) + m_model->applyMessageTagChange(messageId, add, remove); + }); // The strip shows the tags of the message ON DISPLAY, so it has to follow // an edit to that message rather than waiting for the next selection. The @@ -6624,6 +6674,7 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, // wrong in both directions. A conversation leaves the view when a // THREAD-scoped write empties its union, which is the other call site. QStringList touchedThreads; + QSet<QString> seen; // a list's contains() made this quadratic (item 211) bool aRowIsMissing = false; for (const QString &messageId : messageIds) { const QString threadId = m_model->threadIdForMessage(messageId); @@ -6633,8 +6684,10 @@ void MainWindow::sendMessageTagChange(const QStringList &messageIds, } if (m_model->threadCountFor(threadId) > 1) continue; - if (!touchedThreads.contains(threadId)) + if (!seen.contains(threadId)) { + seen.insert(threadId); touchedThreads.append(threadId); + } } syncViewMembership(touchedThreads, aRowIsMissing, add, remove); @@ -6889,8 +6942,10 @@ void MainWindow::trashThreads(const QStringList &threadIds) // holding it back is what made a deleted thread sit unchanged until it was // clicked. It also keeps the toggle's direction readable immediately, so a // second press restores rather than deleting again. - for (const QString &threadId : threadIds) - m_model->applyTagChange(threadId, { QStringLiteral("deleted") }, {}); + batchModelWrites([&] { + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, { QStringLiteral("deleted") }, {}); + }); m_pendingThreadScope = threadIds; QMetaObject::invokeMethod(m_worker, "resolveThreadMessages", @@ -6987,9 +7042,11 @@ void MainWindow::spamThreads(const QStringList &threadIds) // synchronously before it: the same shape as trashThreads(), for the same // two reasons. An unexpanded thread's reply ids and paths exist only in // the database; and the card must not wait for that round trip. - for (const QString &threadId : threadIds) - m_model->applyTagChange(threadId, { QStringLiteral("spam") }, - { QStringLiteral("inbox") }); + batchModelWrites([&] { + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, { QStringLiteral("spam") }, + { QStringLiteral("inbox") }); + }); m_pendingThreadScope = threadIds; QMetaObject::invokeMethod(m_worker, "resolveThreadMessages", @@ -7121,8 +7178,10 @@ void MainWindow::untrashThreads(const QStringList &threadIds) return; // Repainted synchronously, as the delete direction is. - for (const QString &threadId : threadIds) - m_model->applyTagChange(threadId, {}, { QStringLiteral("deleted") }); + batchModelWrites([&] { + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, {}, { QStringLiteral("deleted") }); + }); m_pendingThreadScope = threadIds; QMetaObject::invokeMethod( @@ -7177,8 +7236,10 @@ void MainWindow::notSpamThreads(const QStringList &threadIds) // Repainted synchronously, as the mark-spam direction is: the card is what // the user watches, and it must not wait for the worker round trip. - for (const QString &threadId : threadIds) - m_model->applyTagChange(threadId, {}, { QStringLiteral("spam") }); + batchModelWrites([&] { + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, {}, { QStringLiteral("spam") }); + }); m_pendingThreadScope = threadIds; QMetaObject::invokeMethod( @@ -7818,8 +7879,11 @@ void MainWindow::sendMove(const QStringList &messageIds, // while the latter deliberately leaves a multi-message thread's summary // alone because one message's edit does not describe the conversation. if (wholeThreadIds.isEmpty()) { - for (const QString &messageId : messageIds) - m_model->applyMessageTagChange(messageId, displayAdd, displayRemove); + batchModelWrites([&] { + for (const QString &messageId : messageIds) + m_model->applyMessageTagChange(messageId, displayAdd, + displayRemove); + }); } // A row that no longer belongs in the view LEAVES it, rather than sitting @@ -8136,8 +8200,10 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds, { // Optimistic: the rows change now, so a bulk archive of hundreds of threads // feels instant. Recorded so onWorkerError() can put them back. - for (const QString &threadId : threadIds) - m_model->applyTagChange(threadId, add, remove); + batchModelWrites([&] { + for (const QString &threadId : threadIds) + m_model->applyTagChange(threadId, add, remove); + }); // Which accounts this touches, recorded HERE and not in onTagsApplied(): // TagChange carries message ids, while the account is a property of the diff --git a/src/mainwindow.h b/src/mainwindow.h index 87ed171..868964a 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -1179,6 +1179,10 @@ private: /// something, the same rule refreshUnreadAction() applies to the label. void refreshTrashActions(); + /// Runs a loop of optimistic model writes with the dataChanged-driven + /// action refresh held, then refreshes once (item 211). + void batchModelWrites(const std::function<void()> &writes); + /// Whether every selected row's file already sits in its account's trash /// folder. Empty selection answers false. /// @@ -1209,6 +1213,8 @@ private: /// for exit has finished. Stops closeEvent asking a second time, and is /// what lets the deferred close through. bool m_closeApproved = false; + bool m_batchingModelWrites = false; ///< see batchModelWrites() + quint64 m_announceTicket = 0; ///< see announceAction() /// True while a sync started by the exit prompt is running. The window /// stays open until it finishes: killing the process mid-sync is exactly diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index ad1359b..139d592 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -20,12 +20,11 @@ #include "composecontext.h" -#include <QSet> - #include <QBrush> #include <QFont> #include <QGuiApplication> #include <QPalette> +#include <QSet> #include <QFontDatabase> #include <QFontMetrics> @@ -131,6 +130,49 @@ QColor ThreadListModel::readColour() ThreadListModel::ThreadListModel(QObject *parent) : QAbstractItemModel(parent) { + // Keyed on the model's own structural signals rather than on each call + // site that inserts or removes, so a new one cannot forget. + const auto stale = [this]() { m_indexStale = true; }; + connect(this, &QAbstractItemModel::rowsInserted, this, stale); + connect(this, &QAbstractItemModel::rowsRemoved, this, stale); + connect(this, &QAbstractItemModel::rowsMoved, this, stale); + connect(this, &QAbstractItemModel::modelReset, this, stale); + connect(this, &QAbstractItemModel::layoutChanged, this, stale); +} + +void ThreadListModel::rebuildIndex() const +{ + m_rowByThread.clear(); + m_rowByMessage.clear(); + // tryEmplace keeps the FIRST row for an id, matching the walks this + // replaces, which returned at the first hit. + for (int row = 0; row < m_threads.size(); ++row) { + const ThreadNode &node = m_threads.at(row); + m_rowByThread.tryEmplace(node.summary.threadId, row); + if (!node.summary.firstMessageId.isEmpty()) + m_rowByMessage.tryEmplace(node.summary.firstMessageId, row); + if (!node.first.messageId.isEmpty()) + m_rowByMessage.tryEmplace(node.first.messageId, row); + for (const MessageNode &child : node.children) + m_rowByMessage.tryEmplace(child.messageId, row); + } + m_indexStale = false; +} + +int ThreadListModel::rowForThread(const QString &threadId) const +{ + if (m_indexStale) + rebuildIndex(); + return m_rowByThread.value(threadId, -1); +} + +int ThreadListModel::rowForMessage(const QString &messageId) const +{ + if (messageId.isEmpty()) + return -1; + if (m_indexStale) + rebuildIndex(); + return m_rowByMessage.value(messageId, -1); } QModelIndex ThreadListModel::index(int row, int column, @@ -212,11 +254,15 @@ void ThreadListModel::removeThreadsWithoutTag(const QStringList &threadIds, if (tag.isEmpty() || threadIds.isEmpty()) return; + // A set, because this runs over every row with every touched thread: + // a list made a bulk archive quadratic here too (item 211). + const QSet<QString> wanted(threadIds.cbegin(), threadIds.cend()); + // Backwards for the same reason the sweeping form below is: each removal // renumbers everything after it. for (int row = m_threads.size() - 1; row >= 0; --row) { const ThreadNode &node = m_threads.at(row); - if (!threadIds.contains(node.summary.threadId)) + if (!wanted.contains(node.summary.threadId)) continue; // The SUMMARY, which is notmuch's union over the conversation, and @@ -933,6 +979,7 @@ void ThreadListModel::reconcile(const QVector<ThreadSummary> &threads) || m_threads.at(row).summary.totalCount != summary.totalCount || m_threads.at(row).summary.matchedCount != summary.matchedCount) { m_threads[row].summary = summary; + m_indexStale = true; // firstMessageId may have moved emit dataChanged(index(row, 0), index(row, 0)); } } @@ -997,6 +1044,7 @@ void ThreadListModel::setThreadMessages(const QString &threadId, // Kept so the root card can render its own message. It is the card the // user clicks to read the thread's opening message. m_threads[row].first = nodes.isEmpty() ? MessageNode() : nodes.first(); + m_indexStale = true; // a thread of one inserts no rows to say so if (!children.isEmpty()) { beginInsertRows(parent, 0, children.size() - 1); @@ -1057,7 +1105,11 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const QString ThreadListModel::threadIdForMessage(const QString &messageId) const { - for (const ThreadNode &node : m_threads) { + const int row = rowForMessage(messageId); + if (row < 0) + return {}; + { + const ThreadNode &node = m_threads.at(row); // The ROOT first. A thread's first message is not among its children // (item 109: setThreadMessages drops depth 0 because the root row // stands for it), so a search over children alone answers "no thread" @@ -1077,27 +1129,22 @@ QString ThreadListModel::threadIdForMessage(const QString &messageId) const bool ThreadListModel::hasThread(const QString &threadId) const { - return std::any_of(m_threads.cbegin(), m_threads.cend(), - [&threadId](const ThreadNode &node) { - return node.summary.threadId == threadId; - }); + return rowForThread(threadId) >= 0; } int ThreadListModel::threadCountFor(const QString &threadId) const { - for (const ThreadNode &node : m_threads) { - if (node.summary.threadId == threadId) - return node.summary.totalCount; - } - return 0; + const int row = rowForThread(threadId); + return row < 0 ? 0 : m_threads.at(row).summary.totalCount; } MessageNode ThreadListModel::messageById(const QString &messageId) const { - if (messageId.isEmpty()) + const int row = rowForMessage(messageId); + if (row < 0) return {}; - - for (const ThreadNode &node : m_threads) { + { + const ThreadNode &node = m_threads.at(row); // The root's own message first, and it is not among the children: // setThreadMessages drops depth 0 because the root row stands for it. // Searching only the children returned a default-constructed node for @@ -1199,18 +1246,15 @@ ThreadSummary ThreadListModel::threadFor(const QModelIndex &index) const QStringList ThreadListModel::accountKeysForThread(const QString &threadId) const { QStringList keys; - for (const ThreadNode &node : m_threads) { - const ThreadSummary &thread = node.summary; - if (thread.threadId != threadId) + const int row = rowForThread(threadId); + if (row < 0) + return keys; + for (const QString &tag : m_threads.at(row).summary.tags) { + if (!TagColors::isAccountTag(tag)) continue; - for (const QString &tag : thread.tags) { - if (!TagColors::isAccountTag(tag)) - continue; - const QString key = TagColors::accountKeyForTag(tag); - if (!key.isEmpty() && !keys.contains(key)) - keys.append(key); - } - break; + const QString key = TagColors::accountKeyForTag(tag); + if (!key.isEmpty() && !keys.contains(key)) + keys.append(key); } return keys; } @@ -1219,10 +1263,10 @@ void ThreadListModel::applyTagChange(const QString &threadId, const QStringList &added, const QStringList &removed) { - for (int row = 0; row < m_threads.size(); ++row) { - if (m_threads.at(row).summary.threadId != threadId) - continue; - + const int row = rowForThread(threadId); + if (row < 0) + return; + { QStringList &tags = m_threads[row].summary.tags; for (const QString &tag : removed) tags.removeAll(tag); @@ -1285,7 +1329,10 @@ void ThreadListModel::applyMessageTagChange(const QString &messageId, } }; - for (int row = 0; row < m_threads.size(); ++row) { + const int row = rowForMessage(messageId); + if (row < 0) + return; + { ThreadNode &node = m_threads[row]; const QModelIndex threadIndex = index(row, 0, QModelIndex()); diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index bed099d..fc7f65a 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -20,6 +20,7 @@ #include <QAbstractItemModel> #include <QColor> +#include <QHash> #include <QVector> #include "tagcolors.h" @@ -450,7 +451,26 @@ private: /// than claiming the union as one message's. static ThreadNode nodeFor(const ThreadSummary &summary); + /// The top-level row holding a thread or a message, or -1. + /// + /// Answered from a hash rather than a walk, because a bulk action asks + /// once per message and a walk per message made archiving ten thousand + /// rows quadratic in the lookups alone (item 211). The FIRST row in list + /// order wins, which is the row every linear walk here used to stop at, + /// so the two are interchangeable; a flat view can hold one thread on + /// several rows and still answers with the first. + int rowForThread(const QString &threadId) const; + int rowForMessage(const QString &messageId) const; + void rebuildIndex() const; + QVector<ThreadNode> m_threads; + // Rebuilt lazily on the next lookup after anything renumbers rows or + // changes which ids a node carries. Every structural change signals, and + // the two in-place id changes (reconcile, setThreadMessages) mark it by + // hand. + mutable QHash<QString, int> m_rowByThread; + mutable QHash<QString, int> m_rowByMessage; + mutable bool m_indexStale = true; const TagColors *m_tagColors = nullptr; QString m_dateFormat; QStringList m_forwardPrefixes; |