From aef84cf4409adfbd2dc18887ece2131bc6aa4690 Mon Sep 17 00:00:00 2001 From: Kitsune Ral <> Date: Fri, 12 Jan 2024 15:59:57 +0100 Subject: [PATCH] Make sure events get loaded if the timeline is empty After fixing the race in the room switching procedure (see 4ce59f7e) another race showed up, this time between the model reset and updating root.room property in QML. Because this property is distinct from messageModel.room, it gets updated later, specifically after onModelReset() is called. onModelReset() itself already uses messageModel.room instead of root.room exactly for that reason but ensurePreviousContent() called from there still operates on root.room. That alone wouldn't be much of a hurdle (ensurePreviousContent() could be rewired to messageModel.room just as well) but at the moment onModelReset() is called chatView has not created any delegates for the model yet - meaning that contentY and originY used in ensurePreviousContent() are 0 even when the model has plenty of events loaded, leading to a false-positive condition and an unneeded request to the homeserver for more messages. This commit moves requesting the initial batch of historical events to the model. It does not request a lot, therefore the turnaround is short; and that solves the timeline bootstrapping after a room switch. Eventually this might even move to libQuotient, because events have to be loaded to the room when it is displayed, regardless of the client - but that's something to ponder separately. And while we're at it, the property tracking the number of requested historical events has been moved to QuaternionRoom, anticipating its further move to libQuotient. --- client/models/messageeventmodel.cpp | 3 +++ client/qml/Timeline.qml | 30 +++++++++++------------------ client/quaternionroom.cpp | 13 +++++++++++++ client/quaternionroom.h | 9 +++++++++ 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/client/models/messageeventmodel.cpp b/client/models/messageeventmodel.cpp index 4e330fdd..270fe3a7 100644 --- a/client/models/messageeventmodel.cpp +++ b/client/models/messageeventmodel.cpp @@ -181,6 +181,9 @@ void MessageEventModel::changeRoom(QuaternionRoom* room) qCDebug(EVENTMODEL) << "Event model connected to room" << room->objectName() // << "as" << room->localUser()->id(); + // If the timeline isn't loaded, ask for at least something right away + if (room->timelineSize() == 0) + room->getHistory(30); } endResetModel(); emit readMarkerUpdated(); diff --git a/client/qml/Timeline.qml b/client/qml/Timeline.qml index 9830b282..81f7034f 100644 --- a/client/qml/Timeline.qml +++ b/client/qml/Timeline.qml @@ -256,10 +256,6 @@ Page { contentHeight > 0 && count > 0 ? count / contentHeight : 0.03 // 0.03 is just an arbitrary reasonable number - property int lastRequestedEvents: 0 - readonly property int currentRequestedEvents: - room && room.eventsHistoryJob ? lastRequestedEvents : 0 - property var textEditWithSelection property real readMarkerContentPos: originY readonly property real readMarkerViewportPos: @@ -287,10 +283,8 @@ Page { // Check if we're about to bump into the ceiling in // 2 seconds and if yes, request the amount of messages // enough to scroll at this rate for 3 more seconds - if (velocity > 0 && contentY - velocity*2 < originY) { - lastRequestedEvents = velocity * eventDensity * 3 - room.getPreviousContent(lastRequestedEvents) - } + if (velocity > 0 && contentY - velocity*2 < originY) + room.getHistory(velocity * eventDensity * 3) } onContentYChanged: ensurePreviousContent() onContentHeightChanged: ensurePreviousContent() @@ -346,11 +340,9 @@ Page { chatView.saveViewport(true) } function onModelReset() { - if (messageModel.room) { - // Load events if there are not enough of them - chatView.ensurePreviousContent() - chatView.positionViewAtBeginning() - } + // NB: at this point, the actual delegates are not instantiated + // yet, so defer all actions to when at least some are + scrollFinisher.scrollViewTo(0, ListView.Contain) } } @@ -479,7 +471,7 @@ Page { // A proxy property for animation property int requestedHistoryEventsCount: - chatView.currentRequestedEvents + room ? room.requestedEventsCount : 0 AnimationBehavior on requestedHistoryEventsCount { NormalNumberAnimation { } } @@ -619,8 +611,8 @@ Page { color: palette.alternateBase property bool shown: (chatView.bottommostVisibleIndex >= 0 - && (scrollerArea.containsMouse || scrollAnimation.running)) - || chatView.currentRequestedEvents > 0 + && (scrollerArea.containsMouse || scrollAnimation.running)) + || (room && room.requestedEventsCount > 0) onShownChanged: { if (shown) { @@ -648,10 +640,10 @@ Page { chatView.bottommostVisibleIndex)) + "\n" + qsTr("%Ln events cached", "", chatView.count) : "") - + (chatView.currentRequestedEvents > 0 + + (room && room.requestedEventsCount > 0 ? (chatView.count > 0 ? "\n" : "") + qsTr("%Ln events requested from the server", - "", chatView.currentRequestedEvents) + "", room.requestedEventsCount) : "") horizontalAlignment: Label.AlignRight } @@ -720,7 +712,7 @@ Page { scrollFinisher.scrollViewTo(messageModel.readMarkerVisualIndex, ListView.Center) else - room.getPreviousContent(chatView.count / 2) // FIXME, #799 + room.getHistory(chatView.count / 2) // FIXME, #799 } } } diff --git a/client/quaternionroom.cpp b/client/quaternionroom.cpp index 0bc315e1..2fd666b7 100644 --- a/client/quaternionroom.cpp +++ b/client/quaternionroom.cpp @@ -22,6 +22,8 @@ QuaternionRoom::QuaternionRoom(Connection* connection, QString roomId, { connect(this, &Room::namesChanged, this, &QuaternionRoom::htmlSafeDisplayNameChanged); + connect(this, &Room::eventsHistoryJobChanged, + this, &QuaternionRoom::requestedEventsCountChanged); } const QString& QuaternionRoom::cachedUserFilter() const @@ -80,6 +82,17 @@ QString QuaternionRoom::htmlSafeDisplayName() const return displayName().toHtmlEscaped(); } +int QuaternionRoom::requestedEventsCount() const +{ + return eventsHistoryJob() != nullptr ? m_requestedEventsCount : 0; +} + +void QuaternionRoom::getHistory(int limit) +{ + m_requestedEventsCount = limit; + getPreviousContent(m_requestedEventsCount); +} + void QuaternionRoom::onAddNewTimelineEvents(timeline_iter_t from) { std::for_each(from, messageEvents().cend(), diff --git a/client/quaternionroom.h b/client/quaternionroom.h index ce4566e4..6d1007e9 100644 --- a/client/quaternionroom.h +++ b/client/quaternionroom.h @@ -14,6 +14,7 @@ class QuaternionRoom: public Quotient::Room { Q_OBJECT Q_PROPERTY(QString htmlSafeDisplayName READ htmlSafeDisplayName NOTIFY htmlSafeDisplayNameChanged) + Q_PROPERTY(int requestedEventsCount READ requestedEventsCount NOTIFY requestedEventsCountChanged) public: QuaternionRoom(Quotient::Connection* connection, QString roomId, Quotient::JoinState joinState); @@ -29,16 +30,24 @@ class QuaternionRoom: public Quotient::Room bool force = false); QString htmlSafeDisplayName() const; + int requestedEventsCount() const; + + public slots: + // TODO, 0.0.96: move logic to libQuotient 0.9 and get rid of it here + void getHistory(int limit); signals: // Gotta wrap the Room::namesChanged signal because it has parameters // and moc cannot use signals with parameters defined in the parent // class as NOTIFY targets void htmlSafeDisplayNameChanged(); + // TODO, 0.0.96: same as for getHistory() + void requestedEventsCountChanged(); private: QSet highlights; QString m_cachedUserFilter; + int m_requestedEventsCount = 0; void onAddNewTimelineEvents(timeline_iter_t from) override; void onAddHistoricalTimelineEvents(rev_iter_t from) override;