From 011facc13a5d35ab22418c1a3c34e67c4a15841d Mon Sep 17 00:00:00 2001 From: pajlada Date: Wed, 16 Nov 2022 00:32:15 +0100 Subject: [PATCH] Fix crashes that can occur when selecting/copying text (#4153) --- CHANGELOG.md | 1 + src/messages/Selection.hpp | 70 ++++++++++++++----- src/messages/layouts/MessageLayout.cpp | 2 +- src/messages/layouts/MessageLayout.hpp | 3 +- .../layouts/MessageLayoutContainer.cpp | 8 ++- .../layouts/MessageLayoutContainer.hpp | 3 +- src/messages/layouts/MessageLayoutElement.cpp | 22 +++--- src/messages/layouts/MessageLayoutElement.hpp | 35 +++++----- src/widgets/helper/ChannelView.cpp | 41 ++++++----- src/widgets/helper/ChannelView.hpp | 5 +- 10 files changed, 117 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1353964..79df7e23 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/messages/Selection.hpp b/src/messages/Selection.hpp index 175a4657..f16e6ce2 100644 --- a/src/messages/Selection.hpp +++ b/src/messages/Selection.hpp @@ -1,25 +1,21 @@ #pragma once +#include #include #include namespace chatterino { struct SelectionItem { - int messageIndex; - int charIndex; + uint32_t messageIndex{0}; + 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; - int originalEnd = 0; - int origMessageIndex; - bool selectingLeft = false; - bool selectingRight = false; + uint32_t originalStart{0}; + uint32_t originalEnd{0}; + uint32_t origMessageIndex{0}; + bool selectingLeft{false}; + bool selectingRight{false}; SelectionItem origStartItem; SelectionItem origEndItem; }; diff --git a/src/messages/layouts/MessageLayout.cpp b/src/messages/layouts/MessageLayout.cpp index f2403959..5fd24dd4 100644 --- a/src/messages/layouts/MessageLayout.cpp +++ b/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); diff --git a/src/messages/layouts/MessageLayout.hpp b/src/messages/layouts/MessageLayout.hpp index a8fcf229..7d20cbb6 100644 --- a/src/messages/layouts/MessageLayout.hpp +++ b/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 diff --git a/src/messages/layouts/MessageLayoutContainer.cpp b/src/messages/layouts/MessageLayoutContainer.cpp index 4de1d8fe..8fb503ee 100644 --- a/src/messages/layouts/MessageLayoutContainer.cpp +++ b/src/messages/layouts/MessageLayoutContainer.cpp @@ -798,10 +798,10 @@ int MessageLayoutContainer::getFirstMessageCharacterIndex() const return index; } -void MessageLayoutContainer::addSelectionText(QString &str, int from, int to, - CopyMode copymode) +void MessageLayoutContainer::addSelectionText(QString &str, uint32_t from, + uint32_t to, CopyMode copymode) { - int index = 0; + uint32_t index = 0; bool first = true; for (auto &element : this->elements_) @@ -819,7 +819,9 @@ 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(); diff --git a/src/messages/layouts/MessageLayoutContainer.hpp b/src/messages/layouts/MessageLayoutContainer.hpp index 153e0979..1f2530ba 100644 --- a/src/messages/layouts/MessageLayoutContainer.hpp +++ b/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(); diff --git a/src/messages/layouts/MessageLayoutElement.cpp b/src/messages/layouts/MessageLayoutElement.cpp index 92a2db02..ab75c16d 100644 --- a/src/messages/layouts/MessageLayoutElement.cpp +++ b/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, - int to) const +void ImageLayoutElement::addCopyTextToString(QString &str, uint32_t from, + uint32_t to) const { const auto *emoteElement = dynamic_cast(&this->getCreator()); @@ -272,8 +272,8 @@ void TextLayoutElement::listenToLinkChanges() }); } -void TextLayoutElement::addCopyTextToString(QString &str, int from, - int to) const +void TextLayoutElement::addCopyTextToString(QString &str, uint32_t from, + 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, - int to) const +void TextIconLayoutElement::addCopyTextToString(QString &str, uint32_t from, + uint32_t to) const { } @@ -513,14 +513,12 @@ int ReplyCurveLayoutElement::getXFromIndex(int index) { return this->getRect().left(); } - else - { - return this->getRect().right(); - } + + return this->getRect().right(); } -void ReplyCurveLayoutElement::addCopyTextToString(QString &str, int from, - int to) const +void ReplyCurveLayoutElement::addCopyTextToString(QString &str, uint32_t from, + uint32_t to) const { } diff --git a/src/messages/layouts/MessageLayoutElement.hpp b/src/messages/layouts/MessageLayoutElement.hpp index 5dfec7f0..4dd69233 100644 --- a/src/messages/layouts/MessageLayoutElement.hpp +++ b/src/messages/layouts/MessageLayoutElement.hpp @@ -1,19 +1,20 @@ #pragma once -#include -#include -#include -#include -#include -#include - #include "common/FlagsEnum.hpp" #include "messages/Link.hpp" #include "messages/MessageColor.hpp" #include "messages/MessageElement.hpp" +#include +#include +#include +#include +#include #include +#include +#include + 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, - int to = INT_MAX) const = 0; + virtual void addCopyTextToString(QString &str, uint32_t from = 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, - int to = INT_MAX) const override; + void addCopyTextToString(QString &str, uint32_t from = 0, + 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, - int to = INT_MAX) const override; + void addCopyTextToString(QString &str, uint32_t from = 0, + 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, - int to = INT_MAX) const override; + void addCopyTextToString(QString &str, uint32_t from = 0, + 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, - int to = INT_MAX) const override; + void addCopyTextToString(QString &str, uint32_t from = 0, + uint32_t to = UINT32_MAX) const override; int getSelectionIndexCount() const override; private: diff --git a/src/widgets/helper/ChannelView.cpp b/src/widgets/helper/ChannelView.cpp index c42fe046..b88a93dc 100644 --- a/src/widgets/helper/ChannelView.cpp +++ b/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_.selectionMax.messageIndex -= this->pauseSelectionOffset_; - this->selection_.start.messageIndex -= this->pauseSelectionOffset_; - this->selection_.end.messageIndex -= this->pauseSelectionOffset_; + this->selection_.shiftMessageIndex(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,23 +547,32 @@ QString ChannelView::getSelectedText() LimitedQueueSnapshot &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; - msg <= _selection.selectionMax.messageIndex; msg++) + const auto numMessages = messagesSnapshot.size(); + 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 - ? _selection.selectionMin.charIndex - : 0; - int to = msg == _selection.selectionMax.messageIndex - ? _selection.selectionMax.charIndex - : layout->getLastCharacterIndex() + 1; + auto from = msg == selection.selectionMin.messageIndex + ? selection.selectionMin.charIndex + : 0; + auto to = msg == selection.selectionMax.messageIndex + ? 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_.selectionMax.messageIndex--; - this->selection_.start.messageIndex--; - this->selection_.end.messageIndex--; + this->selection_.shiftMessageIndex(1); } this->queueLayout(); diff --git a/src/widgets/helper/ChannelView.hpp b/src/widgets/helper/ChannelView.hpp index 685b783d..3562169f 100644 --- a/src/widgets/helper/ChannelView.hpp +++ b/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 &messages); void updateScrollbar(LimitedQueueSnapshot &messages, @@ -255,7 +255,8 @@ private: pauses_; boost::optional 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 overrideFlags_; MessageLayoutPtr lastReadMessage_;