fix: take indices to messages as a hint (#5683)

CHANGELOG.md

@@ -63,6 +63,7 @@
 - Bugfix: Fixed double-click selection not working when clicking outside a message. (#5617)
 - Bugfix: Fixed emotes starting with ":" not tab-completing. (#5603)
 - Bugfix: Fixed 7TV emotes messing with Qt's HTML. (#5677)
+- Bugfix: Fixed incorrect messages getting replaced visually. (#5683)
 - Dev: Update Windows build from Qt 6.5.0 to Qt 6.7.1. (#5420)
 - Dev: Update vcpkg build Qt from 6.5.0 to 6.7.0, boost from 1.83.0 to 1.85.0, openssl from 3.1.3 to 3.3.0. (#5422)
 - Dev: Unsingletonize `ISoundController`. (#5462)

src/common/Channel.cpp

@@ -253,21 +253,33 @@ void Channel::fillInMissingMessages(const std::vector<MessagePtr> &messages)
     }
 }
 
-void Channel::replaceMessage(MessagePtr message, MessagePtr replacement)
+void Channel::replaceMessage(const MessagePtr &message,
+                             const MessagePtr &replacement)
 {
     int index = this->messages_.replaceItem(message, replacement);
 
     if (index >= 0)
     {
-        this->messageReplaced.invoke((size_t)index, replacement);
+        this->messageReplaced.invoke((size_t)index, message, replacement);
     }
 }
 
-void Channel::replaceMessage(size_t index, MessagePtr replacement)
+void Channel::replaceMessage(size_t index, const MessagePtr &replacement)
 {
-    if (this->messages_.replaceItem(index, replacement))
+    MessagePtr prev;
+    if (this->messages_.replaceItem(index, replacement, &prev))
     {
-        this->messageReplaced.invoke(index, replacement);
+        this->messageReplaced.invoke(index, prev, replacement);
+    }
+}
+
+void Channel::replaceMessage(size_t hint, const MessagePtr &message,
+                             const MessagePtr &replacement)
+{
+    auto index = this->messages_.replaceItem(hint, message, replacement);
+    if (index >= 0)
+    {
+        this->messageReplaced.invoke(hint, message, replacement);
     }
 }
 

src/common/Channel.hpp

@@ -66,7 +66,9 @@ public:
     pajlada::Signals::Signal<MessagePtr &, std::optional<MessageFlags>>
         messageAppended;
     pajlada::Signals::Signal<std::vector<MessagePtr> &> messagesAddedAtStart;
-    pajlada::Signals::Signal<size_t, MessagePtr &> messageReplaced;
+    /// (index, prev-message, replacement)
+    pajlada::Signals::Signal<size_t, const MessagePtr &, const MessagePtr &>
+        messageReplaced;
     /// Invoked when some number of messages were filled in using time received
     pajlada::Signals::Signal<const std::vector<MessagePtr> &> filledInMessages;
     pajlada::Signals::NoArgSignal destroyed;
@@ -96,8 +98,11 @@ public:
 
     void addOrReplaceTimeout(MessagePtr message);
     void disableAllMessages();
-    void replaceMessage(MessagePtr message, MessagePtr replacement);
-    void replaceMessage(size_t index, MessagePtr replacement);
+    void replaceMessage(const MessagePtr &message,
+                        const MessagePtr &replacement);
+    void replaceMessage(size_t index, const MessagePtr &replacement);
+    void replaceMessage(size_t hint, const MessagePtr &message,
+                        const MessagePtr &replacement);
     void deleteMessage(QString messageID);
 
     /// Removes all messages from this channel and invokes #messagesCleared

src/messages/LimitedQueue.hpp

@@ -8,6 +8,7 @@
 #include <mutex>
 #include <optional>
 #include <shared_mutex>
+#include <utility>
 #include <vector>
 
 namespace chatterino {
@@ -212,9 +213,10 @@ public:
      *
      * @param[in] index the index of the item to replace
      * @param[in] replacement the item to put in place of the item at index
+     * @param[out] prev (optional) the item located at @a index before replacing
      * @return true if a replacement took place
      */
-    bool replaceItem(size_t index, const T &replacement)
+    bool replaceItem(size_t index, const T &replacement, T *prev = nullptr)
     {
         std::unique_lock lock(this->mutex_);
 
@@ -223,10 +225,46 @@ public:
             return false;
         }
 
-        this->buffer_[index] = replacement;
+        if (prev)
+        {
+            *prev = std::exchange(this->buffer_[index], replacement);
+        }
+        else
+        {
+            this->buffer_[index] = replacement;
+        }
         return true;
     }
 
+    /**
+     * @brief Replace the needle with the given item
+     *
+     * @param hint A hint on where the needle _might_ be
+     * @param[in] needle the item to search for
+     * @param[in] replacement the item to replace needle with
+     * @return the index of the replaced item, or -1 if no replacement took place
+     */
+    int replaceItem(size_t hint, const T &needle, const T &replacement)
+    {
+        std::unique_lock lock(this->mutex_);
+
+        if (hint < this->buffer_.size() && this->buffer_[hint] == needle)
+        {
+            this->buffer_[hint] = replacement;
+            return static_cast<int>(hint);
+        }
+
+        for (size_t i = 0; i < this->buffer_.size(); ++i)
+        {
+            if (this->buffer_[i] == needle)
+            {
+                this->buffer_[i] = replacement;
+                return static_cast<int>(i);
+            }
+        }
+        return -1;
+    }
+
     /**
      * @brief Inserts the given item before another item
      * 
@@ -315,6 +353,32 @@ public:
         return std::nullopt;
     }
 
+    /**
+     * @brief Find an item with a hint
+     *
+     * @param hint A hint on where the needle _might_ be
+     * @param predicate that will used to find the item
+     * @return the item and its index or none if it's not found
+     */
+    std::optional<std::pair<size_t, T>> find(size_t hint, auto &&predicate)
+    {
+        std::unique_lock lock(this->mutex_);
+
+        if (hint < this->buffer_.size() && predicate(this->buffer_[hint]))
+        {
+            return std::pair{hint, this->buffer_[hint]};
+        };
+
+        for (size_t i = 0; i < this->buffer_.size(); i++)
+        {
+            if (predicate(this->buffer_[i]))
+            {
+                return std::pair{i, this->buffer_[i]};
+            }
+        }
+        return std::nullopt;
+    }
+
     /**
      * @brief Returns the first item matching a predicate, checking in reverse
      * 

src/widgets/helper/ChannelView.cpp

@@ -967,10 +967,10 @@ void ChannelView::setChannel(const ChannelPtr &underlyingChannel)
 
     this->channelConnections_.managedConnect(
         underlyingChannel->messageReplaced,
-        [this](auto index, const auto &replacement) {
+        [this](auto index, const auto &prev, const auto &replacement) {
             if (this->shouldIncludeMessage(replacement))
             {
-                this->channel_->replaceMessage(index, replacement);
+                this->channel_->replaceMessage(index, prev, replacement);
             }
         });
 
@@ -1051,8 +1051,9 @@ void ChannelView::setChannel(const ChannelPtr &underlyingChannel)
     // on message replaced
     this->channelConnections_.managedConnect(
         this->channel_->messageReplaced,
-        [this](size_t index, MessagePtr replacement) {
-            this->messageReplaced(index, replacement);
+        [this](size_t index, const MessagePtr &prev,
+               const MessagePtr &replacement) {
+            this->messageReplaced(index, prev, replacement);
         });
 
     // on messages filled in
@@ -1258,19 +1259,21 @@ void ChannelView::messageAddedAtStart(std::vector<MessagePtr> &messages)
     this->queueLayout();
 }
 
-void ChannelView::messageReplaced(size_t index, MessagePtr &replacement)
+void ChannelView::messageReplaced(size_t hint, const MessagePtr &prev,
+                                  const MessagePtr &replacement)
 {
-    auto oMessage = this->messages_.get(index);
-    if (!oMessage)
+    auto optItem = this->messages_.find(hint, [&](const auto &it) {
+        return it->getMessagePtr() == prev;
+    });
+    if (!optItem)
     {
         return;
     }
-
-    auto message = *oMessage;
+    const auto &[index, oldItem] = *optItem;
 
     auto newItem = std::make_shared<MessageLayout>(replacement);
 
-    if (message->flags.has(MessageLayoutFlag::AlternateBackground))
+    if (oldItem->flags.has(MessageLayoutFlag::AlternateBackground))
     {
         newItem->flags.set(MessageLayoutFlag::AlternateBackground);
     }
@@ -1278,7 +1281,7 @@ void ChannelView::messageReplaced(size_t index, MessagePtr &replacement)
     this->scrollBar_->replaceHighlight(index,
                                        replacement->getScrollBarHighlight());
 
-    this->messages_.replaceItem(message, newItem);
+    this->messages_.replaceItem(index, newItem);
     this->queueLayout();
 }
 

src/widgets/helper/ChannelView.hpp

@@ -268,7 +268,8 @@ private:
                          std::optional<MessageFlags> overridingFlags);
     void messageAddedAtStart(std::vector<MessagePtr> &messages);
     void messageRemoveFromStart(MessagePtr &message);
-    void messageReplaced(size_t index, MessagePtr &replacement);
+    void messageReplaced(size_t hint, const MessagePtr &prev,
+                         const MessagePtr &replacement);
     void messagesUpdated();
 
     void performLayout(bool causedByScrollbar = false,

tests/src/LimitedQueue.cpp

@@ -114,20 +114,153 @@ TEST(LimitedQueue, PushFront)
 
 TEST(LimitedQueue, ReplaceItem)
 {
-    LimitedQueue<int> queue(5);
+    LimitedQueue<int> queue(10);
     queue.pushBack(1);
     queue.pushBack(2);
     queue.pushBack(3);
+    queue.pushBack(4);
+    queue.pushBack(5);
+    queue.pushBack(6);
 
     int idex = queue.replaceItem(2, 10);
     EXPECT_EQ(idex, 1);
-    idex = queue.replaceItem(5, 11);
+    idex = queue.replaceItem(7, 11);
     EXPECT_EQ(idex, -1);
 
-    bool res = queue.replaceItem(std::size_t(0), 9);
+    int prev = -1;
+    bool res = queue.replaceItem(std::size_t(0), 9, &prev);
     EXPECT_TRUE(res);
-    res = queue.replaceItem(std::size_t(5), 4);
+    EXPECT_EQ(prev, 1);
+    res = queue.replaceItem(std::size_t(6), 4);
     EXPECT_FALSE(res);
 
-    SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3}, "first snapshot");
+    // correct hint
+    EXPECT_EQ(queue.replaceItem(3, 4, 11), 3);
+    // incorrect hints
+    EXPECT_EQ(queue.replaceItem(5, 11, 12), 3);
+    EXPECT_EQ(queue.replaceItem(0, 12, 13), 3);
+    // oob hint
+    EXPECT_EQ(queue.replaceItem(42, 13, 14), 3);
+    // bad needle
+    EXPECT_EQ(queue.replaceItem(0, 15, 16), -1);
+
+    SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3, 14, 5, 6},
+                    "first snapshot");
+}
+
+TEST(LimitedQueue, Find)
+{
+    LimitedQueue<int> queue(10);
+    queue.pushBack(1);
+    queue.pushBack(2);
+    queue.pushBack(3);
+    queue.pushBack(4);
+    queue.pushBack(5);
+    queue.pushBack(6);
+
+    // without hint
+    EXPECT_FALSE(queue
+                     .find([](int i) {
+                         return i == 0;
+                     })
+                     .has_value());
+    EXPECT_EQ(queue
+                  .find([](int i) {
+                      return i == 1;
+                  })
+                  .value(),
+              1);
+    EXPECT_EQ(queue
+                  .find([](int i) {
+                      return i == 2;
+                  })
+                  .value(),
+              2);
+    EXPECT_EQ(queue
+                  .find([](int i) {
+                      return i == 6;
+                  })
+                  .value(),
+              6);
+    EXPECT_FALSE(queue
+                     .find([](int i) {
+                         return i == 7;
+                     })
+                     .has_value());
+    EXPECT_FALSE(queue
+                     .find([](int i) {
+                         return i > 6;
+                     })
+                     .has_value());
+    EXPECT_FALSE(queue
+                     .find([](int i) {
+                         return i <= 0;
+                     })
+                     .has_value());
+
+    using Pair = std::pair<size_t, int>;
+    // with hint
+    EXPECT_FALSE(queue
+                     .find(0,
+                           [](int i) {
+                               return i == 0;
+                           })
+                     .has_value());
+    // correct hint
+    EXPECT_EQ(queue
+                  .find(0,
+                        [](int i) {
+                            return i == 1;
+                        })
+                  .value(),
+              (Pair{0, 1}));
+    EXPECT_EQ(queue
+                  .find(1,
+                        [](int i) {
+                            return i == 2;
+                        })
+                  .value(),
+              (Pair{1, 2}));
+    // incorrect hint
+    EXPECT_EQ(queue
+                  .find(1,
+                        [](int i) {
+                            return i == 1;
+                        })
+                  .value(),
+              (Pair{0, 1}));
+    EXPECT_EQ(queue
+                  .find(5,
+                        [](int i) {
+                            return i == 6;
+                        })
+                  .value(),
+              (Pair{5, 6}));
+    // oob hint
+    EXPECT_EQ(queue
+                  .find(6,
+                        [](int i) {
+                            return i == 3;
+                        })
+                  .value(),
+              (Pair{2, 3}));
+    // non-existent items
+    EXPECT_FALSE(queue
+                     .find(42,
+                           [](int i) {
+                               return i == 7;
+                           })
+                     .has_value());
+    EXPECT_FALSE(queue
+                     .find(0,
+                           [](int i) {
+                               return i > 6;
+                           })
+                     .has_value());
+    EXPECT_FALSE(queue
+                     .find(0,
+                           [](int i) {
+                               return i <= 0;
+                           })
+                     .has_value());
 }