Fix crashes that can occur when selecting/copying text (#4153)

CHANGELOG.md

@@ -112,6 +112,7 @@
 - Bugfix: Display sent IRC messages like received ones (#4027)
 - Bugfix: Fixed non-global FrankerFaceZ emotes from being loaded as global emotes. (#3921)
 - Bugfix: Fixed trailing spaces from preventing Nicknames from working correctly. (#3946)
+- Bugfix: Fixed crashes that can occur while selecting/copying messages and they are removed. (#4153)
 - Bugfix: Fixed trailing spaces from preventing User Highlights from working correctly. (#4051)
 - Bugfix: Fixed channel-based popups from rewriting messages to file log (#4060)
 - Bugfix: Fixed invalid/dangling completion when cycling through previous messages or replying (#4072)

src/messages/Selection.hpp

@@ -1,25 +1,21 @@
 #pragma once
 
+#include <cstdint>
 #include <tuple>
 #include <utility>
 
 namespace chatterino {
 
 struct SelectionItem {
-    int messageIndex;
+    uint32_t messageIndex{0};
-    int charIndex;
+    uint32_t charIndex{0};
 
-    SelectionItem()
+    SelectionItem() = default;
+
+    SelectionItem(uint32_t _messageIndex, uint32_t _charIndex)
+        : messageIndex(_messageIndex)
+        , charIndex(_charIndex)
     {
-        this->messageIndex = 0;
-        this->charIndex = 0;
-    }
-
-    SelectionItem(int _messageIndex, int _charIndex)
-    {
-        this->messageIndex = _messageIndex;
-
-        this->charIndex = _charIndex;
     }
 
     bool operator<(const SelectionItem &b) const
@@ -75,14 +71,54 @@ struct Selection {
         return this->selectionMin.messageIndex ==
                this->selectionMax.messageIndex;
     }
+
+    // Shift all message selection indices `offset` back
+    void shiftMessageIndex(uint32_t offset)
+    {
+        if (offset > this->selectionMin.messageIndex)
+        {
+            this->selectionMin.messageIndex = 0;
+        }
+        else
+        {
+            this->selectionMin.messageIndex -= offset;
+        }
+
+        if (offset > this->selectionMax.messageIndex)
+        {
+            this->selectionMax.messageIndex = 0;
+        }
+        else
+        {
+            this->selectionMax.messageIndex -= offset;
+        }
+
+        if (offset > this->start.messageIndex)
+        {
+            this->start.messageIndex = 0;
+        }
+        else
+        {
+            this->start.messageIndex -= offset;
+        }
+
+        if (offset > this->end.messageIndex)
+        {
+            this->end.messageIndex = 0;
+        }
+        else
+        {
+            this->end.messageIndex -= offset;
+        }
+    }
 };
 
 struct DoubleClickSelection {
-    int originalStart = 0;
+    uint32_t originalStart{0};
-    int originalEnd = 0;
+    uint32_t originalEnd{0};
-    int origMessageIndex;
+    uint32_t origMessageIndex{0};
-    bool selectingLeft = false;
+    bool selectingLeft{false};
-    bool selectingRight = false;
+    bool selectingRight{false};
     SelectionItem origStartItem;
     SelectionItem origEndItem;
 };

src/messages/layouts/MessageLayout.cpp

@@ -442,7 +442,7 @@ int MessageLayout::getSelectionIndex(QPoint position)
     return this->container_->getSelectionIndex(position);
 }
 
-void MessageLayout::addSelectionText(QString &str, int from, int to,
+void MessageLayout::addSelectionText(QString &str, uint32_t from, uint32_t to,
                                      CopyMode copymode)
 {
     this->container_->addSelectionText(str, from, to, copymode);

src/messages/layouts/MessageLayout.hpp

@@ -59,7 +59,8 @@ public:
     int getLastCharacterIndex() const;
     int getFirstMessageCharacterIndex() const;
     int getSelectionIndex(QPoint position);
-    void addSelectionText(QString &str, int from = 0, int to = INT_MAX,
+    void addSelectionText(QString &str, uint32_t from = 0,
+                          uint32_t to = UINT32_MAX,
                           CopyMode copymode = CopyMode::Everything);
 
     // Misc

src/messages/layouts/MessageLayoutContainer.cpp

@@ -798,10 +798,10 @@ int MessageLayoutContainer::getFirstMessageCharacterIndex() const
     return index;
 }
 
-void MessageLayoutContainer::addSelectionText(QString &str, int from, int to,
+void MessageLayoutContainer::addSelectionText(QString &str, uint32_t from,
-                                              CopyMode copymode)
+                                              uint32_t to, CopyMode copymode)
 {
-    int index = 0;
+    uint32_t index = 0;
     bool first = true;
 
     for (auto &element : this->elements_)
@@ -819,8 +819,10 @@ void MessageLayoutContainer::addSelectionText(QString &str, int from, int to,
             if (element->getCreator().getFlags().hasAny(
                     {MessageElementFlag::Timestamp,
                      MessageElementFlag::Username, MessageElementFlag::Badges}))
+            {
                 continue;
             }
+        }
 
         auto indexCount = element->getSelectionIndexCount();
 

src/messages/layouts/MessageLayoutContainer.hpp

@@ -81,7 +81,8 @@ struct MessageLayoutContainer {
     int getSelectionIndex(QPoint point);
     int getLastCharacterIndex() const;
     int getFirstMessageCharacterIndex() const;
-    void addSelectionText(QString &str, int from, int to, CopyMode copymode);
+    void addSelectionText(QString &str, uint32_t from, uint32_t to,
+                          CopyMode copymode);
 
     bool isCollapsed();
 

src/messages/layouts/MessageLayoutElement.cpp

@@ -108,8 +108,8 @@ ImageLayoutElement::ImageLayoutElement(MessageElement &creator, ImagePtr image,
     this->trailingSpace = creator.hasTrailingSpace();
 }
 
-void ImageLayoutElement::addCopyTextToString(QString &str, int from,
+void ImageLayoutElement::addCopyTextToString(QString &str, uint32_t from,
-                                             int to) const
+                                             uint32_t to) const
 {
     const auto *emoteElement =
         dynamic_cast<EmoteElement *>(&this->getCreator());
@@ -272,8 +272,8 @@ void TextLayoutElement::listenToLinkChanges()
         });
 }
 
-void TextLayoutElement::addCopyTextToString(QString &str, int from,
+void TextLayoutElement::addCopyTextToString(QString &str, uint32_t from,
-                                            int to) const
+                                            uint32_t to) const
 {
     str += this->getText().mid(from, to - from);
 
@@ -387,8 +387,8 @@ TextIconLayoutElement::TextIconLayoutElement(MessageElement &creator,
 {
 }
 
-void TextIconLayoutElement::addCopyTextToString(QString &str, int from,
+void TextIconLayoutElement::addCopyTextToString(QString &str, uint32_t from,
-                                                int to) const
+                                                uint32_t to) const
 {
 }
 
@@ -513,14 +513,12 @@ int ReplyCurveLayoutElement::getXFromIndex(int index)
     {
         return this->getRect().left();
     }
-    else
+
-    {
     return this->getRect().right();
 }
-}
 
-void ReplyCurveLayoutElement::addCopyTextToString(QString &str, int from,
+void ReplyCurveLayoutElement::addCopyTextToString(QString &str, uint32_t from,
-                                                  int to) const
+                                                  uint32_t to) const
 {
 }
 

src/messages/layouts/MessageLayoutElement.hpp

@@ -1,19 +1,20 @@
 #pragma once
 
-#include <QPen>
-#include <QPoint>
-#include <QRect>
-#include <QString>
-#include <boost/noncopyable.hpp>
-#include <climits>
-
 #include "common/FlagsEnum.hpp"
 #include "messages/Link.hpp"
 #include "messages/MessageColor.hpp"
 #include "messages/MessageElement.hpp"
 
+#include <QPen>
+#include <QPoint>
+#include <QRect>
+#include <QString>
+#include <boost/noncopyable.hpp>
 #include <pajlada/signals/signalholder.hpp>
 
+#include <climits>
+#include <cstdint>
+
 class QPainter;
 
 namespace chatterino {
@@ -41,8 +42,8 @@ public:
     MessageLayoutElement *setLink(const Link &link_);
     MessageLayoutElement *setText(const QString &text_);
 
-    virtual void addCopyTextToString(QString &str, int from = 0,
+    virtual void addCopyTextToString(QString &str, uint32_t from = 0,
-                                     int to = INT_MAX) const = 0;
+                                     uint32_t to = UINT32_MAX) const = 0;
     virtual int getSelectionIndexCount() const = 0;
     virtual void paint(QPainter &painter) = 0;
     virtual void paintAnimated(QPainter &painter, int yOffset) = 0;
@@ -72,8 +73,8 @@ public:
                        const QSize &size);
 
 protected:
-    void addCopyTextToString(QString &str, int from = 0,
+    void addCopyTextToString(QString &str, uint32_t from = 0,
-                             int to = INT_MAX) const override;
+                             uint32_t to = UINT32_MAX) const override;
     int getSelectionIndexCount() const override;
     void paint(QPainter &painter) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
@@ -124,8 +125,8 @@ public:
     void listenToLinkChanges();
 
 protected:
-    void addCopyTextToString(QString &str, int from = 0,
+    void addCopyTextToString(QString &str, uint32_t from = 0,
-                             int to = INT_MAX) const override;
+                             uint32_t to = UINT32_MAX) const override;
     int getSelectionIndexCount() const override;
     void paint(QPainter &painter) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
@@ -148,8 +149,8 @@ public:
                           const QString &line2, float scale, const QSize &size);
 
 protected:
-    void addCopyTextToString(QString &str, int from = 0,
+    void addCopyTextToString(QString &str, uint32_t from = 0,
-                             int to = INT_MAX) const override;
+                             uint32_t to = UINT32_MAX) const override;
     int getSelectionIndexCount() const override;
     void paint(QPainter &painter) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
@@ -173,8 +174,8 @@ protected:
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
     int getXFromIndex(int index) override;
-    void addCopyTextToString(QString &str, int from = 0,
+    void addCopyTextToString(QString &str, uint32_t from = 0,
-                             int to = INT_MAX) const override;
+                             uint32_t to = UINT32_MAX) const override;
     int getSelectionIndexCount() const override;
 
 private:

src/widgets/helper/ChannelView.cpp

@@ -353,10 +353,7 @@ void ChannelView::updatePauses()
 void ChannelView::unpaused()
 {
     /// Move selection
-    this->selection_.selectionMin.messageIndex -= this->pauseSelectionOffset_;
+    this->selection_.shiftMessageIndex(this->pauseSelectionOffset_);
-    this->selection_.selectionMax.messageIndex -= this->pauseSelectionOffset_;
-    this->selection_.start.messageIndex -= this->pauseSelectionOffset_;
-    this->selection_.end.messageIndex -= this->pauseSelectionOffset_;
 
     this->pauseSelectionOffset_ = 0;
 }
@@ -456,7 +453,7 @@ void ChannelView::layoutVisibleMessages(
 
         for (auto i = start; i < messages.size() && y <= this->height(); i++)
         {
-            auto message = messages[i];
+            const auto &message = messages[i];
 
             redrawRequired |=
                 message->layout(layoutWidth, this->scale(), flags);
@@ -550,22 +547,31 @@ QString ChannelView::getSelectedText()
     LimitedQueueSnapshot<MessageLayoutPtr> &messagesSnapshot =
         this->getMessagesSnapshot();
 
-    Selection _selection = this->selection_;
+    Selection selection = this->selection_;
 
-    if (_selection.isEmpty())
+    if (selection.isEmpty())
     {
         return result;
     }
 
-    for (int msg = _selection.selectionMin.messageIndex;
+    const auto numMessages = messagesSnapshot.size();
-         msg <= _selection.selectionMax.messageIndex; msg++)
+    const auto indexStart = selection.selectionMin.messageIndex;
+    const auto indexEnd = selection.selectionMax.messageIndex;
+
+    if (indexEnd >= numMessages || indexStart >= numMessages)
+    {
+        // One of our messages is out of bounds
+        return result;
+    }
+
+    for (auto msg = indexStart; msg <= indexEnd; msg++)
     {
         MessageLayoutPtr layout = messagesSnapshot[msg];
-        int from = msg == _selection.selectionMin.messageIndex
+        auto from = msg == selection.selectionMin.messageIndex
-                       ? _selection.selectionMin.charIndex
+                        ? selection.selectionMin.charIndex
                         : 0;
-        int to = msg == _selection.selectionMax.messageIndex
+        auto to = msg == selection.selectionMax.messageIndex
-                     ? _selection.selectionMax.charIndex
+                      ? selection.selectionMax.charIndex
                       : layout->getLastCharacterIndex() + 1;
 
         layout->addSelectionText(result, from, to);
@@ -961,10 +967,7 @@ void ChannelView::messageRemoveFromStart(MessagePtr &message)
     }
     else
     {
-        this->selection_.selectionMin.messageIndex--;
+        this->selection_.shiftMessageIndex(1);
-        this->selection_.selectionMax.messageIndex--;
-        this->selection_.start.messageIndex--;
-        this->selection_.end.messageIndex--;
     }
 
     this->queueLayout();

src/widgets/helper/ChannelView.hpp

@@ -184,7 +184,7 @@ private:
     void messageReplaced(size_t index, MessagePtr &replacement);
     void messagesUpdated();
 
-    void performLayout(bool causedByScollbar = false);
+    void performLayout(bool causedByScrollbar = false);
     void layoutVisibleMessages(
         LimitedQueueSnapshot<MessageLayoutPtr> &messages);
     void updateScrollbar(LimitedQueueSnapshot<MessageLayoutPtr> &messages,
@@ -255,7 +255,8 @@ private:
         pauses_;
     boost::optional<SteadyClock::time_point> pauseEnd_;
     int pauseScrollOffset_ = 0;
-    int pauseSelectionOffset_ = 0;
+    // Keeps track how many message indices we need to offset the selection when we resume scrolling
+    uint32_t pauseSelectionOffset_ = 0;
 
     boost::optional<MessageElementFlags> overrideFlags_;
     MessageLayoutPtr lastReadMessage_;