From 2ad759b36f2e9964cd5a9ab57fa6acf53da4bcc6 Mon Sep 17 00:00:00 2001 From: Volker Christian Date: Sat, 22 Aug 2026 16:10:20 +0200 Subject: [PATCH] Preserve keyed timeline segments during reconciliation Replace prefix-only segment compatibility with an exhaustive keyed diff so unchanged presentation keys retain their QWidget identity across insertion and reorder. Remove only absent segments and keep viewport anchors attached to surviving widgets. Call-site audit: the single segment reconciliation site changes; turn-level compatibility, render call structure, and the exact-content fast path are deliberately unchanged. Deletion census: 86 lines removed under src/. The removed block was the prefix-alignment and whole-turn teardown algorithm (longer than five lines), replaced by keyed removal and indexed survivor reuse; the remaining removed layout calls are superseded by indexed insertion. Runtime proof covers live activity growth, middle insertion, exhaustive reorder identity, selective lookup removal plus deferred destruction, and viewport-anchor lifetime. No golden hash or protocol fingerprint changed. --- src/ui/ConversationWidget.cpp | 153 +++++++-------- src/ui/ConversationWidget.h | 3 + tests/ConversationLayoutTest.cpp | 327 +++++++++++++++++++++++++++++++ 3 files changed, 397 insertions(+), 86 deletions(-) diff --git a/src/ui/ConversationWidget.cpp b/src/ui/ConversationWidget.cpp index afb353a..e8ac8ee 100644 --- a/src/ui/ConversationWidget.cpp +++ b/src/ui/ConversationWidget.cpp @@ -3092,6 +3092,8 @@ void ConversationWidget::render(const sdk::State& state, renderedSegmentItemIds.clear(); renderedSegmentKeys.clear(); renderedSegmentWidgets.clear(); + renderedActivityRows.clear(); + renderedActivityRowSegments.clear(); clearLayout(timeline); }; @@ -3261,14 +3263,37 @@ void ConversationWidget::render(const sdk::State& state, for (const VisibleTimelineTurn& visibleTurn : visibleTurns) visibleTurnIds.append(fromUtf8(visibleTurn.turn->id.value)); - const auto removeRenderedTurn = [this, &timelineShrank, &timelineGeometryChanged](const QString& turnId) + const auto forgetRenderedActivityRows = [this](const QString& storage) + { + for (auto row = renderedActivityRowSegments.begin(); + row != renderedActivityRowSegments.end();) + { + if (row.value() != storage) + { + ++row; + continue; + } + renderedActivityRows.remove(row.key()); + row = renderedActivityRowSegments.erase(row); + } + }; + const auto forgetRenderedSegment = + [this, &forgetRenderedActivityRows](const QString& storage) + { + forgetRenderedActivityRows(storage); + renderedSegmentItemIds.remove(storage); + renderedSegmentKeys.remove(storage); + renderedSegmentWidgets.remove(storage); + }; + const auto removeRenderedTurn = [this, + &forgetRenderedSegment, + &timelineShrank, + &timelineGeometryChanged](const QString& turnId) { for (const QString& segmentId : renderedSegmentIds.take(turnId)) { const QString storage = segmentStorageKey(turnId, segmentId); - renderedSegmentItemIds.remove(storage); - renderedSegmentKeys.remove(storage); - renderedSegmentWidgets.remove(storage); + forgetRenderedSegment(storage); } renderedTurnLabels.remove(turnId); renderedTurnStatusLabels.remove(turnId); @@ -3384,90 +3409,34 @@ void ConversationWidget::render(const sdk::State& state, segmentIds.append(segment->id); const QStringList oldSegmentIds = renderedSegmentIds.value(turnId); - bool compatibleSegments = true; - qsizetype removedSegmentPrefix = 0; - if (!oldSegmentIds.isEmpty()) + const QSet nextSegmentIds( + segmentIds.cbegin(), segmentIds.cend()); + for (const QString& oldId : oldSegmentIds) { - if (segmentIds.isEmpty()) - { - compatibleSegments = false; - } - else - { - removedSegmentPrefix = oldSegmentIds.indexOf(segmentIds.front()); - compatibleSegments = removedSegmentPrefix >= 0 - && oldSegmentIds.size() - removedSegmentPrefix <= segmentIds.size(); - for (qsizetype segmentIndex = 0; - compatibleSegments && segmentIndex < oldSegmentIds.size() - removedSegmentPrefix; - ++segmentIndex) - { - const QString oldId = oldSegmentIds.at(removedSegmentPrefix + segmentIndex); - const QString storage = segmentStorageKey(turnId, oldId); - compatibleSegments = oldId == segmentIds.at(segmentIndex) - && renderedSegmentWidgets.contains(storage); - } - } - } - if (!compatibleSegments) - { - if (QWidget* turnWidget = renderedTurnWidgets.value(turnId); - pendingViewportAnchor && turnWidget - && (pendingViewportAnchor == turnWidget - || turnWidget->isAncestorOf(pendingViewportAnchor))) - pendingViewportAnchor.clear(); - clearLayout(itemLayout); - timelineShrank = timelineShrank || !oldSegmentIds.isEmpty(); - for (const QString& oldId : oldSegmentIds) - { - const QString storage = segmentStorageKey(turnId, oldId); - renderedSegmentItemIds.remove(storage); - renderedSegmentKeys.remove(storage); - renderedSegmentWidgets.remove(storage); - } - } - else - { - if (removedSegmentPrefix > 0 && pendingViewportAnchor) - { - bool anchorRemoved = false; - for (qsizetype segmentIndex = 0; segmentIndex < removedSegmentPrefix; ++segmentIndex) - { - QWidget* removed = renderedSegmentWidgets.value( - segmentStorageKey(turnId, oldSegmentIds.at(segmentIndex))); - anchorRemoved = anchorRemoved || pendingViewportAnchor == removed - || (removed && removed->isAncestorOf(pendingViewportAnchor)); - } - if (anchorRemoved && removedSegmentPrefix < oldSegmentIds.size()) - { - QWidget* survivor = renderedSegmentWidgets.value( - segmentStorageKey(turnId, oldSegmentIds.at(removedSegmentPrefix))); - pendingViewportAnchor = survivor; - if (survivor) - { - pendingViewportAnchorY = scrollArea->viewport() - ->mapFromGlobal(survivor->mapToGlobal(QPoint{})) - .y(); - } - } - } - for (qsizetype segmentIndex = 0; segmentIndex < removedSegmentPrefix; ++segmentIndex) + if (nextSegmentIds.contains(oldId)) + continue; + const QString storage = segmentStorageKey(turnId, oldId); + if (QWidget* widget = renderedSegmentWidgets.value(storage)) { - const QString storage = segmentStorageKey(turnId, oldSegmentIds.at(segmentIndex)); - if (QWidget* widget = renderedSegmentWidgets.take(storage)) - { - itemLayout->removeWidget(widget); - widget->hide(); - widget->deleteLater(); - timelineShrank = true; - timelineGeometryChanged = true; - } - renderedSegmentKeys.remove(storage); - renderedSegmentItemIds.remove(storage); + if (pendingViewportAnchor == widget + || (pendingViewportAnchor + && widget->isAncestorOf(pendingViewportAnchor))) + pendingViewportAnchor.clear(); + itemLayout->removeWidget(widget); + widget->hide(); + widget->deleteLater(); + timelineShrank = true; + timelineGeometryChanged = true; } + forgetRenderedSegment(storage); } - for (const TimelineSegment* segment : visibleTurn.segments) + for (qsizetype segmentIndex = 0; + segmentIndex < static_cast(visibleTurn.segments.size()); + ++segmentIndex) { + const TimelineSegment* segment = visibleTurn.segments.at( + static_cast(segmentIndex)); const QString storage = segmentStorageKey(turnId, segment->id); QStringList itemIds; itemIds.reserve(static_cast(segment->items.size())); @@ -3478,6 +3447,16 @@ void ConversationWidget::render(const sdk::State& state, } renderedSegmentItemIds.insert(storage, std::move(itemIds)); QWidget* oldWidget = renderedSegmentWidgets.value(storage); + if (oldWidget) + oldWidget->setProperty( + "timelineItemCount", timelineItemCount(*segment)); + if (oldWidget && itemLayout->indexOf(oldWidget) != segmentIndex) + { + itemLayout->removeWidget(oldWidget); + itemLayout->insertWidget( + segmentIndex, oldWidget, 0, Qt::AlignTop); + timelineGeometryChanged = true; + } const ConversationContentUpdates* segmentContentChanges = nullptr; ConversationContentUpdates segmentContentStorage; bool explicitlyAffected = false; @@ -3564,19 +3543,21 @@ void ConversationWidget::render(const sdk::State& state, const bool replacesAnchor = pendingViewportAnchor == oldWidget || (pendingViewportAnchor && oldWidget->isAncestorOf(pendingViewportAnchor)); - const int position = itemLayout->indexOf(oldWidget); itemLayout->removeWidget(oldWidget); oldWidget->hide(); oldWidget->deleteLater(); - itemLayout->insertWidget(position, newWidget, 0, Qt::AlignTop); + forgetRenderedActivityRows(storage); + itemLayout->insertWidget( + segmentIndex, newWidget, 0, Qt::AlignTop); if (replacesAnchor) - pendingViewportAnchor = newWidget; + pendingViewportAnchor.clear(); timelineShrank = true; timelineGeometryChanged = true; } else { - itemLayout->addWidget(newWidget, 0, Qt::AlignTop); + itemLayout->insertWidget( + segmentIndex, newWidget, 0, Qt::AlignTop); timelineGeometryChanged = true; } renderedSegmentWidgets.insert(storage, newWidget); diff --git a/src/ui/ConversationWidget.h b/src/ui/ConversationWidget.h index 9cb50bd..1f0c335 100644 --- a/src/ui/ConversationWidget.h +++ b/src/ui/ConversationWidget.h @@ -39,6 +39,7 @@ namespace codexui { class AnchoredTurnSurface; class UpcomingTurnDock; struct UpcomingTurnDraft; +struct ConversationWidgetTestAccess; struct ConversationContentAppend { @@ -104,6 +105,8 @@ class ConversationWidget : public QWidget void resizeEvent(QResizeEvent* event) override; private: + friend struct ConversationWidgetTestAccess; + void scheduleTimelineLayout(int previousScroll, bool followLatest, bool threadChanged, diff --git a/tests/ConversationLayoutTest.cpp b/tests/ConversationLayoutTest.cpp index b35923f..b6feb4f 100644 --- a/tests/ConversationLayoutTest.cpp +++ b/tests/ConversationLayoutTest.cpp @@ -32,6 +32,57 @@ #include #include +namespace codexui { + +struct ConversationWidgetTestAccess +{ + static bool primeViewportAnchor( + ConversationWidget& conversation, QWidget* anchor, int anchorY) + { + auto* scroll = conversation.scrollArea->verticalScrollBar(); + scroll->setValue(scroll->minimum()); + conversation.followingLatest = false; + conversation.pinLatestDuringLayout = true; + conversation.layoutSettleTimer->start(60'000); + conversation.pendingViewportAnchor = anchor; + conversation.pendingViewportAnchorY = anchorY; + return scroll->maximum() - scroll->value() > 72; + } + + static QWidget* viewportAnchor(const ConversationWidget& conversation) + { + return conversation.pendingViewportAnchor.data(); + } + + static int viewportAnchorY(const ConversationWidget& conversation) + { + return conversation.pendingViewportAnchorY; + } + + static bool tracksSegment( + const ConversationWidget& conversation, + const QString& turnId, + const QString& segmentId) + { + return conversation.renderedSegmentWidgets.contains( + turnId + QChar(0x1f) + segmentId); + } + + static void finishAnchoredReconciliation(ConversationWidget& conversation) + { + conversation.layoutSettleTimer->stop(); + conversation.layoutSettleTimer->setInterval(16); + conversation.pinLatestDuringLayout = false; + conversation.pendingViewportAnchor.clear(); + conversation.pendingFollowLatest = false; + conversation.pendingThreadChanged = false; + conversation.pendingTimelineShrink = false; + conversation.scrollArea->viewport()->setUpdatesEnabled(true); + } +}; + +} // namespace codexui + namespace { namespace frontend = ai::openai::codex::frontend; @@ -1057,6 +1108,281 @@ bool testPointerPreservingAppend() return passed; } +bool testKeyedSegmentInsertion() +{ + ThreadFixture activityFixture{ + "keyed-activity-growth", + {{"turn-keyed-activity-growth", + {{"activity-keyed-0", + frontend::ThreadItemKind::Reasoning, + "first activity"}, + {"activity-keyed-1", + frontend::ThreadItemKind::Reasoning, + "second activity"}, + {"message-keyed-tail", + frontend::ThreadItemKind::UserMessage, + "stable tail"}}}}}; + activityFixture.turns.front().status = "inProgress"; + activityFixture.turns.front().active = true; + activityFixture.turns.front().terminal = false; + for (int index = 0; index < 18; ++index) + { + activityFixture.turns.front().messages.push_back( + {"message-keyed-padding-" + std::to_string(index), + frontend::ThreadItemKind::UserMessage, + "padding row " + std::to_string(index)}); + } + codexui::ConversationWidget activityConversation; + activityConversation.resize(900, 420); + activityConversation.show(); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + settleTimeline(); + + QPointer activity = segment( + activityConversation, + QStringLiteral("activities:activity-keyed-0")); + QPointer activityTail = segment( + activityConversation, + QStringLiteral("message:message-keyed-tail")); + QWidget* const activityAddress = activity.data(); + QWidget* const activityTailAddress = activityTail.data(); + + const auto activityTailFixturePosition = std::find_if( + activityFixture.turns.front().messages.begin(), + activityFixture.turns.front().messages.end(), + [](const MessageFixture& message) + { + return message.id == "message-keyed-tail"; + }); + activityFixture.turns.front().messages.insert( + activityTailFixturePosition, + {"activity-keyed-2", + frontend::ThreadItemKind::Reasoning, + "third activity"}); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + settleTimeline(); + + bool passed = expect( + activity && activity.data() == activityAddress + && activity.data() + == segment( + activityConversation, + QStringLiteral("activities:activity-keyed-0")) + && activity->findChildren( + QStringLiteral("conversationActivityRow")) + .size() + == 3 + && activityTail && activityTail.data() == activityTailAddress + && activityTail.data() + == segment( + activityConversation, + QStringLiteral("message:message-keyed-tail")), + "an activity bucket that gains a row must retain its segment and every unchanged trailing widget"); + + QHash> unchangedActivitySegments; + unchangedActivitySegments.insert( + QStringLiteral("activities:activity-keyed-0"), activity); + unchangedActivitySegments.insert( + QStringLiteral("message:message-keyed-tail"), activityTail); + for (int index = 0; index < 18; ++index) + { + const QString segmentId = QStringLiteral("message:message-keyed-padding-%1") + .arg(index); + unchangedActivitySegments.insert( + segmentId, segment(activityConversation, segmentId)); + } + + auto& activityMessages = activityFixture.turns.front().messages; + std::rotate( + activityMessages.begin(), + activityMessages.begin() + 3, + activityMessages.begin() + 4); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + settleTimeline(); + + QLayout* activityLayout = activityTail && activityTail->parentWidget() + ? activityTail->parentWidget()->layout() + : nullptr; + bool allReorderedSegmentsRetained = true; + for (auto retained = unchangedActivitySegments.cbegin(); + retained != unchangedActivitySegments.cend(); + ++retained) + { + allReorderedSegmentsRetained = + allReorderedSegmentsRetained && retained.value() + && retained.value().data() + == segment(activityConversation, retained.key()); + } + passed &= expect( + allReorderedSegmentsRetained + && activity && activity.data() == activityAddress + && activity.data() + == segment( + activityConversation, + QStringLiteral("activities:activity-keyed-0")) + && activityTail && activityTail.data() == activityTailAddress + && activityTail.data() + == segment( + activityConversation, + QStringLiteral("message:message-keyed-tail")) + && activityLayout && activityLayout->indexOf(activityTail.data()) == 0 + && activityLayout->indexOf(activity.data()) == 1, + "reordering unchanged segment keys must move the original QWidgets into the new order"); + + constexpr int anchoredY = 37; + const bool anchorPathAvailable = + codexui::ConversationWidgetTestAccess::primeViewportAnchor( + activityConversation, activity.data(), anchoredY); + std::rotate( + activityMessages.begin(), + activityMessages.begin() + 1, + activityMessages.begin() + 4); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + passed &= expect( + anchorPathAvailable + && codexui::ConversationWidgetTestAccess::viewportAnchor( + activityConversation) + == activity.data() + && codexui::ConversationWidgetTestAccess::viewportAnchorY( + activityConversation) + == anchoredY, + "reordering a surviving segment must preserve its pending viewport anchor and offset"); + + codexui::ConversationWidgetTestAccess::primeViewportAnchor( + activityConversation, activity.data(), anchoredY); + const auto tailPosition = std::find_if( + activityMessages.begin(), + activityMessages.end(), + [](const MessageFixture& message) + { + return message.id == "message-keyed-tail"; + }); + activityMessages.erase(tailPosition); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + const bool removedTailUntracked = + !codexui::ConversationWidgetTestAccess::tracksSegment( + activityConversation, + QStringLiteral("turn-keyed-activity-growth"), + QStringLiteral("message:message-keyed-tail")); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + QCoreApplication::processEvents(); + passed &= expect( + removedTailUntracked && !activityTail + && activity && activity.data() == activityAddress + && codexui::ConversationWidgetTestAccess::viewportAnchor( + activityConversation) + == activity.data() + && codexui::ConversationWidgetTestAccess::viewportAnchorY( + activityConversation) + == anchoredY, + "removing a different segment must destroy only that widget and retain the surviving anchor"); + + codexui::ConversationWidgetTestAccess::primeViewportAnchor( + activityConversation, activity.data(), anchoredY); + activityMessages.erase( + std::remove_if( + activityMessages.begin(), + activityMessages.end(), + [](const MessageFixture& message) + { + return message.id.starts_with("activity-keyed-"); + }), + activityMessages.end()); + activityConversation.render( + makeState({activityFixture}), + QStringLiteral("keyed-activity-growth")); + const bool removedAnchorCleared = + !codexui::ConversationWidgetTestAccess::viewportAnchor( + activityConversation); + const bool removedAnchorUntracked = + !codexui::ConversationWidgetTestAccess::tracksSegment( + activityConversation, + QStringLiteral("turn-keyed-activity-growth"), + QStringLiteral("activities:activity-keyed-0")); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + QCoreApplication::processEvents(); + passed &= expect( + removedAnchorCleared && removedAnchorUntracked && !activity, + "destroying the anchor segment must clear its viewport anchor and delete the widget"); + codexui::ConversationWidgetTestAccess::finishAnchoredReconciliation( + activityConversation); + + ThreadFixture insertionFixture{ + "keyed-middle-insertion", + {{"turn-keyed-middle-insertion", + {{"message-keyed-left", + frontend::ThreadItemKind::UserMessage, + "left"}, + {"message-keyed-right", + frontend::ThreadItemKind::UserMessage, + "right"}}}}}; + insertionFixture.turns.front().status = "inProgress"; + insertionFixture.turns.front().active = true; + insertionFixture.turns.front().terminal = false; + codexui::ConversationWidget insertionConversation; + insertionConversation.resize(900, 700); + insertionConversation.show(); + insertionConversation.render( + makeState({insertionFixture}), + QStringLiteral("keyed-middle-insertion")); + settleTimeline(); + + QPointer left = segment( + insertionConversation, + QStringLiteral("message:message-keyed-left")); + QPointer right = segment( + insertionConversation, + QStringLiteral("message:message-keyed-right")); + QWidget* const leftAddress = left.data(); + QWidget* const rightAddress = right.data(); + + insertionFixture.turns.front().messages.insert( + insertionFixture.turns.front().messages.begin() + 1, + {"message-keyed-middle", + frontend::ThreadItemKind::UserMessage, + "middle"}); + insertionConversation.render( + makeState({insertionFixture}), + QStringLiteral("keyed-middle-insertion")); + settleTimeline(); + + QWidget* const middle = segment( + insertionConversation, + QStringLiteral("message:message-keyed-middle")); + QLayout* const orderedLayout = middle && middle->parentWidget() + ? middle->parentWidget()->layout() + : nullptr; + passed &= expect( + left && left.data() == leftAddress + && left.data() + == segment( + insertionConversation, + QStringLiteral("message:message-keyed-left")) + && right && right.data() == rightAddress + && right.data() + == segment( + insertionConversation, + QStringLiteral("message:message-keyed-right")), + "inserting a segment in the middle must preserve every unchanged keyed QWidget"); + passed &= expect( + orderedLayout && middle + && orderedLayout->indexOf(left.data()) == 0 + && orderedLayout->indexOf(middle) == 1 + && orderedLayout->indexOf(right.data()) == 2, + "a middle insertion must place the new segment between its surviving neighbors"); + return passed; +} + bool testInPlaceMessageReplacement() { ThreadFixture agentFixture{"in-place-agent", @@ -2587,6 +2913,7 @@ int main(int argc, char** argv) passed &= testHotTurnWindow(); passed &= testActivityDisclosureAndFullOutput(); passed &= testPointerPreservingAppend(); + passed &= testKeyedSegmentInsertion(); passed &= testInPlaceMessageReplacement(); passed &= testIncompleteThreadPresentation(); passed &= testIncompleteReplacementPreservesRenderedTimeline();