diff --git a/retroshare-gui/src/gui/Posted/PostedPostsModel.cpp b/retroshare-gui/src/gui/Posted/PostedPostsModel.cpp index 6809379d3..e827a2fbb 100644 --- a/retroshare-gui/src/gui/Posted/PostedPostsModel.cpp +++ b/retroshare-gui/src/gui/Posted/PostedPostsModel.cpp @@ -79,6 +79,31 @@ void RsPostedPostsModel::handleEvent_main_thread(std::shared_ptr case RsPostedEventCode::MESSAGE_VOTES_UPDATED: case RsPostedEventCode::NEW_MESSAGE: { + // Batched read-status change ("mark all as read", possibly issued by + // another frontend such as the webUI): the event carries the affected + // message ids and their new state, so update the flags locally + // instead of re-reading the posts from the database. + if( e->mPostedEventCode == RsPostedEventCode::READ_STATUS_CHANGED + && !e->mPostedMsgIds.empty() ) + { + if(e->mPostedGroupId != mPostedGroup.mMeta.mGroupId) + return; + + const std::set ids(e->mPostedMsgIds.begin(),e->mPostedMsgIds.end()); + + for(uint32_t i=0;imPostedMsgsRead) + mPosts[i].mMeta.mMsgStatus &= ~(GXS_SERV::GXS_MSG_STATUS_GUI_UNREAD | GXS_SERV::GXS_MSG_STATUS_GUI_NEW); + else + mPosts[i].mMeta.mMsgStatus |= GXS_SERV::GXS_MSG_STATUS_GUI_UNREAD; + } + + emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(mDisplayedNbPosts-1,0,(void*)NULL)); + return; + } + // Normally we should just emit dataChanged() on the index of the data that has changed: // // We need to update the data! @@ -737,20 +762,38 @@ void RsPostedPostsModel::createPostsArray(std::vector& posts) void RsPostedPostsModel::setAllMsgReadStatus(bool read) { - // make a temporary listof pairs + // Collect the posts whose status actually changes, persist them all in a + // single background batch (one thread and one event for the whole set, + // instead of one detached thread + one event per post, which froze the UI + // on large boards), then update the local model and refresh once. - std::list pairs; + std::vector msgIds; + msgIds.reserve(mPosts.size()); for(uint32_t i=0;i(std::move(msgIds)); + + RsThread::async([boardId=mPostedGroup.mMeta.mGroupId, ids, read]() { - rsPosted->setPostReadStatus(p,read); + rsPosted->setPostReadStatus(boardId, *ids, read); + delete ids; } ); + } + + // The model is updated when the resulting READ_STATUS_CHANGED event comes + // back (see handleEvent_main_thread()), through the same path as a batch + // initiated by any other frontend (e.g. the webUI). } void RsPostedPostsModel::setMsgReadStatus(const QModelIndex& i,bool read_status) { diff --git a/retroshare-gui/src/gui/gxschannels/GxsChannelPostsModel.cpp b/retroshare-gui/src/gui/gxschannels/GxsChannelPostsModel.cpp index c5a4b51cc..22d1dbcba 100644 --- a/retroshare-gui/src/gui/gxschannels/GxsChannelPostsModel.cpp +++ b/retroshare-gui/src/gui/gxschannels/GxsChannelPostsModel.cpp @@ -652,13 +652,20 @@ void RsGxsChannelPostsModel::setAllMsgReadStatus(bool read_status) pairs.push_back(RsGxsGrpMsgIdPair(mPosts[i].mMeta.mGroupId,mPosts[i].mMeta.mMsgId)); } - // 2 - then call the async methods + // 2 - then persist them all in a single background batch: one thread and one + // event for the whole set, instead of one detached thread (and one + // event) per post, which froze the UI on large channels. - for(uint32_t i=0;i msgIds; + msgIds.reserve(pairs.size()); + for(const RsGxsGrpMsgIdPair& p : pairs) + msgIds.push_back(p.second); + + if(!msgIds.empty()) + RsThread::async([channelId=mChannelGroup.mMeta.mGroupId, msgIds, read_status]() { - if(!rsGxsChannels->setMessageReadStatus(p,read_status)) - RsErr() << "setAllMsgReadStatus: failed to change status of msg " << p.first << " in group " << p.second << " to status " << read_status << std::endl; + if(!rsGxsChannels->setMessageReadStatus(channelId, msgIds, read_status)) + RsErr() << "setAllMsgReadStatus: failed to change status of " << msgIds.size() << " messages in channel " << channelId << std::endl; }); // 3 - update the local model data, since we don't catch the READ_STATUS_CHANGED event later, to avoid re-loading the msg. diff --git a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp index 70151384c..5dfe21bba 100644 --- a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp +++ b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp @@ -759,8 +759,15 @@ void RsGxsForumModel::setPosts(const RsGxsForumGroup& group, const std::vectorsource mapping. + // The former code also emitted a bogus beginInsertRows()/endInsertRows() on top + // of the reset, double-counting rows and corrupting that mapping — which made a + // freshly arrived post map to an invalid source index (post not shown, title + // left bold when selected). A single reset already tells the views to re-query + // everything, so no explicit row-insertion signal is needed. beginResetModel(); - endResetModel(); mForumGroup = group; mPosts = posts; @@ -783,17 +790,7 @@ void RsGxsForumModel::setPosts(const RsGxsForumGroup& group, const std::vector0) - { - beginInsertRows(QModelIndex(),0,count-1); - endInsertRows(); - } + endResetModel(); postMods(); emit forumLoaded(); @@ -871,19 +868,71 @@ void RsGxsForumModel::setMsgReadStatus(const QModelIndex& i,bool read_status,boo if(!convertRefPointerToTabEntry(ref,entry) || entry >= mPosts.size()) return ; + // Collect the posts whose read status actually changes and update the + // in-memory model right away, but do NOT touch the backend or the view once + // per message: doing so used to spawn one detached thread AND emit one + // dataChanged() per post, which froze the UI for a very long time (and could + // crash) when marking a large forum (thousands of posts) as read. + std::vector changed_msgs; + uint32_t changed_entries = 0; + recursSetMsgReadStatus(entry,read_status,with_children,changed_msgs,changed_entries) ; + bool has_unread_below, has_read_below; - recursSetMsgReadStatus(entry,read_status,with_children) ; recursUpdateReadStatusAndTimes(0,has_unread_below,has_read_below); - // also emit dataChanged() for parents since they need to re-draw + // Persist the change(s) in the background so the GUI thread never blocks. + // A single interactive read (the common case: selecting/opening one post) + // goes through the per-message markRead(), which emits READ_STATUS_CHANGED + // right away, so the unread counters refresh without the extra latency added + // by the batch path (which only emits its event once waitToken() returns). + // Larger operations (mark-all, with-children, versioned posts) use the + // batched call, which persists everything in a single transaction and emits + // a single event when the whole set is done. + if(changed_msgs.size() == 1) + RsThread::async( [grpId=mForumGroup.mMeta.mGroupId,msgId=changed_msgs[0],read_status]() + { + rsGxsForums->markRead(std::make_pair(grpId, msgId), read_status); + }); + else if(!changed_msgs.empty()) + { + // Hand the (possibly long) id list over through a pointer: the lambda + // capture would otherwise deep copy it. + auto* msgs = new std::vector(std::move(changed_msgs)); - for(QModelIndex j = i.parent(); j.isValid(); j = j.parent()) - { - emit dataChanged(j, j.sibling(j.row(), COLUMN_THREAD_NB_COLUMNS - 1)); - } + RsThread::async( [grpId=mForumGroup.mMeta.mGroupId,msgs,read_status]() + { + rsGxsForums->markRead(grpId, *msgs, read_status); + delete msgs; + }); + } + + // How the view is refreshed depends on how many *rows* changed, not on how + // many messages were written. A post keeps every edited version of itself in + // the database and they all share one read status, so marking a single post + // read can queue dozens of message ids while still repainting exactly one + // row. Testing changed_msgs.size() here meant that one click on a post with + // 47 stored versions emitted dataChanged() over the whole model: 1072 ms of + // frozen interface on a 8259 post forum, measured. + if (with_children || changed_entries > 5) + { + // A single refresh of the whole view instead of one dataChanged() per post. + if(mTreeMode == TREE_MODE_FLAT) + emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(mPosts.size(),COLUMN_THREAD_NB_COLUMNS-1,(void*)NULL)); + else + emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(mPosts[0].mChildren.size(),COLUMN_THREAD_NB_COLUMNS-1,(void*)NULL)); + } + else + { + // Emit dataChanged only for the changed message and its parents + emit dataChanged(i, i.sibling(i.row(), COLUMN_THREAD_NB_COLUMNS - 1)); + for(QModelIndex j = i.parent(); j.isValid(); j = j.parent()) + { + emit dataChanged(j, j.sibling(j.row(), COLUMN_THREAD_NB_COLUMNS - 1)); + } + } } -void RsGxsForumModel::recursSetMsgReadStatus(ForumModelIndex i,bool read_status,bool with_children) +void RsGxsForumModel::recursSetMsgReadStatus(ForumModelIndex i,bool read_status,bool with_children,std::vector& changed_msgs,uint32_t& changed_entries) { uint32_t newStatus = (read_status ? mPosts[i].mMsgStatus & ~static_cast(GXS_SERV::GXS_MSG_STATUS_GUI_UNREAD) : mPosts[i].mMsgStatus | static_cast(GXS_SERV::GXS_MSG_STATUS_GUI_UNREAD)); @@ -894,36 +943,28 @@ void RsGxsForumModel::recursSetMsgReadStatus(ForumModelIndex i,bool read_status, if (bChanged) { + // One row changed, whatever the number of stored versions collected just + // below. setMsgReadStatus() sizes its view refresh on this count. + ++changed_entries; + //Don't recurs post versions as this should be done before, if no change. auto s = getPostVersions(mPosts[i].mMsgId) ; + // Just collect the affected message ids here. The backend is updated + // once, in a single batched call issued by setMsgReadStatus(), instead + // of one detached thread (and one dataChanged()) per message. if(!s.empty()) for(auto it(s.begin());it!=s.end();++it) - { - RsThread::async( [grpId=mForumGroup.mMeta.mGroupId,msgId=it->second,original_msg_id=mPosts[i].mMsgId,read_status]() - { - rsGxsForums->markRead(std::make_pair( grpId, msgId ), read_status); - std::cerr << "Setting version " << msgId << " of post " << original_msg_id << " as read." << std::endl; - }); - } + changed_msgs.push_back(it->second); else - RsThread::async( [grpId=mForumGroup.mMeta.mGroupId,original_msg_id=mPosts[i].mMsgId,read_status]() - { - rsGxsForums->markRead(std::make_pair( grpId, original_msg_id), read_status); - }); - - void *ref ; - convertTabEntryToRefPointer(i,ref); // we dont use i+1 here because i is not a row, but an index in the mPosts tab - - QModelIndex itemIndex = (mTreeMode == TREE_MODE_FLAT)?createIndex(i - 1, 0, ref):createIndex(mPosts[i].prow,0,ref); - emit dataChanged(itemIndex, itemIndex.sibling(itemIndex.row(), COLUMN_THREAD_NB_COLUMNS - 1)); + changed_msgs.push_back(mPosts[i].mMsgId); } if(!with_children) return; for(uint32_t j=0;j& messages); void recursUpdateReadStatusAndTimes(ForumModelIndex i,bool& has_unread_below,bool& has_read_below); uint32_t recursUpdateFilterStatus(ForumModelIndex i,int column,const QStringList& strings); - void recursSetMsgReadStatus(ForumModelIndex i,bool read_status,bool with_children); + void recursSetMsgReadStatus(ForumModelIndex i,bool read_status,bool with_children,std::vector& changed_msgs,uint32_t& changed_entries); static void generateMissingItem(const RsGxsMessageId &msgId,ForumModelPostEntry& entry); static ForumModelIndex addEntry(std::vector& posts,const ForumModelPostEntry& entry,ForumModelIndex parent); diff --git a/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.cpp b/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.cpp index f55bbb0c0..e71a400b2 100644 --- a/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.cpp +++ b/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include "util/qtthreadsutils.h" #include "util/misc.h" @@ -253,6 +254,13 @@ GxsForumThreadWidget::GxsForumThreadWidget(const RsGxsGroupId &forumId, QWidget { ui->setupUi(this); + // Single-shot timer used to coalesce the full-forum reloads requested by + // incoming GXS events (see scheduleForumReload()). Created first thing: + // setGroupId(forumId) below reaches updateDisplay(), which touches this timer. + mDeferredReloadTimer = new QTimer(this); + mDeferredReloadTimer->setSingleShot(true); + connect(mDeferredReloadTimer, &QTimer::timeout, this, [this]() { updateDisplay(true); }); + //setUpdateWhenInvisible(true); //mUpdating = false; @@ -402,21 +410,17 @@ void GxsForumThreadWidget::handleEvent_main_thread(std::shared_ptrmForumGroupId == mForumGroup.mMeta.mGroupId) - updateDisplay(true); + scheduleForumReload(); break; case RsForumEventCode::SUBSCRIBE_STATUS_CHANGED: if(e->mForumGroupId == mForumGroup.mMeta.mGroupId) - { - // Toggle subscribe flag locally and refresh UI without GXS request - // to avoid concurrent request with parent dialog's tree rebuild - if(IS_GROUP_SUBSCRIBED(mForumGroup.mMeta.mSubscribeFlags)) - mForumGroup.mMeta.mSubscribeFlags &= ~GXS_SERV::GROUP_SUBSCRIBE_SUBSCRIBED; - else - mForumGroup.mMeta.mSubscribeFlags |= GXS_SERV::GROUP_SUBSCRIBE_SUBSCRIBED; - + // Re-read the group instead of guessing the new flag by flipping + // the current one: the event says the status changed, not that it + // was toggled, and a wrong guess makes the widget believe the + // forum is unsubscribed, which silently disables marking posts + // read or unread until the group data comes back. updateGroupData(); - } break; default: break; @@ -424,6 +428,16 @@ void GxsForumThreadWidget::handleEvent_main_thread(std::shared_ptrstart(300); +} + void GxsForumThreadWidget::showForumInfo() { mThreadId.clear(); @@ -596,6 +610,10 @@ void GxsForumThreadWidget::updateDisplay(bool complete) } if(complete) // need to update the group data, reload the messages etc. { + // We are reloading now, so drop any reload still pending in the coalescing + // timer (e.g. queued by events for a forum we just switched away from). + mDeferredReloadTimer->stop(); + saveExpandedItems(mSavedExpandedMessages); if(groupId() != mThreadModel->currentGroupId()) @@ -1463,7 +1481,6 @@ void GxsForumThreadWidget::markMsgAsReadUnread (bool read, bool children, bool f if (groupId().isNull() || !IS_GROUP_SUBSCRIBED(mForumGroup.mMeta.mSubscribeFlags)) { return; } - saveExpandedItems(mSavedExpandedMessages); QModelIndex src_index; if(forum) @@ -1475,11 +1492,15 @@ void GxsForumThreadWidget::markMsgAsReadUnread (bool read, bool children, bool f else src_index = mThreadModel->getIndexOfMessage(mThreadId); } - mThreadModel->setMsgReadStatus(src_index,read,children); - //Restore Selection - whileBlocking(ui->threadTreeWidget)->setCurrentIndex(mThreadProxyModel->mapFromSource(mThreadModel->getIndexOfMessage(mThreadId))); - recursRestoreExpandedItems(QModelIndex(),mSavedExpandedMessages); + // setMsgReadStatus() only emits dataChanged(): it never resets the model nor + // changes its layout, so neither the expanded items nor the current index are + // lost here. Saving and restoring them was pure overhead -- and not a cheap + // one: restoring walks every expanded item, and each of them costs a linear + // scan of the post array (getIndexOfMessage) plus a linear scan of the view + // items (QTreeView::setExpanded). On a forum with thousands of posts that is + // quadratic work on the GUI thread for every single post marked read. + mThreadModel->setMsgReadStatus(src_index,read,children); } void GxsForumThreadWidget::markMsgAsRead() diff --git a/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.h b/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.h index 8e6b1c01b..130f70bd0 100644 --- a/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.h +++ b/retroshare-gui/src/gui/gxsforums/GxsForumThreadWidget.h @@ -29,6 +29,7 @@ #include "util/FontSizeHandler.h" class QSortFilterProxyModel; +class QTimer; class QTreeWidgetItem; class RSTreeWidgetItemCompareRole; class GxsForumsFillThread; @@ -197,6 +198,13 @@ private: void handleEvent_main_thread(std::shared_ptr event); + // Coalesce the full-forum reloads triggered by incoming GXS events: a sync + // burst delivers many NEW_MESSAGE events in a row, and reloading the whole + // forum for each one froze the UI for seconds and reset the model out from + // under the user's selection. Restart a single-shot timer instead so a burst + // results in one reload once the events settle. + void scheduleForumReload(); + private: void setForumDescriptionLoading(); void clearForumDescription(); @@ -239,6 +247,8 @@ private: Ui::GxsForumThreadWidget *ui; RsEventsHandlerId_t mEventHandlerId; + + QTimer *mDeferredReloadTimer; }; #endif // GXSFORUMTHREADWIDGET_H