Fix selection rendering (#4830)

The rendering of selections was not aligned to the actual selection that took place for newlines at the end of messages, if they were the only part that was selected of that message.

In addition to that fix, we've already refactored the MessageLayoutContainer to try to make it a little bit more sane to work with in the future.

Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com>

CHANGELOG.md

@@ -13,7 +13,7 @@
 - Bugfix: Fixed selection of tabs after closing a tab when using "Live Tabs Only". (#4770)
 - Bugfix: Fixed input in reply thread popup losing focus when dragging. (#4815)
 - Bugfix: Fixed the Quick Switcher (CTRL+K) from sometimes showing up on the wrong window. (#4819)
-- Bugfix: Fixed too much text being copied when copying chat messages. (#4812)
+- Bugfix: Fixed too much text being copied when copying chat messages. (#4812, #4830)
 - Dev: Fixed UTF16 encoding of `modes` file for the installer. (#4791)
 - Dev: Temporarily disable High DPI scaling on Qt6 builds on Windows. (#4767)
 - Dev: Tests now run on Ubuntu 22.04 instead of 20.04 to loosen C++ restrictions in tests. (#4774)

src/messages/Selection.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <cstddef>
 #include <cstdint>
 #include <tuple>
 #include <utility>
@@ -7,12 +8,12 @@
 namespace chatterino {
 
 struct SelectionItem {
-    uint32_t messageIndex{0};
+    size_t messageIndex{0};
-    uint32_t charIndex{0};
+    size_t charIndex{0};
 
     SelectionItem() = default;
 
-    SelectionItem(uint32_t _messageIndex, uint32_t _charIndex)
+    SelectionItem(size_t _messageIndex, size_t _charIndex)
         : messageIndex(_messageIndex)
         , charIndex(_charIndex)
     {
@@ -73,7 +74,7 @@ struct Selection {
     }
 
     // Shift all message selection indices `offset` back
-    void shiftMessageIndex(uint32_t offset)
+    void shiftMessageIndex(size_t offset)
     {
         if (offset > this->selectionMin.messageIndex)
         {

src/messages/layouts/MessageLayout.cpp

@@ -143,7 +143,7 @@ void MessageLayout::actuallyLayout(int width, MessageElementFlags flags)
     bool hideSimilar = getSettings()->hideSimilar;
     bool hideReplies = !flags.has(MessageElementFlag::RepliedMessage);
 
-    this->container_.begin(width, this->scale_, messageFlags);
+    this->container_.beginLayout(width, this->scale_, messageFlags);
 
     for (const auto &element : this->message_->elements)
     {
@@ -184,7 +184,7 @@ void MessageLayout::actuallyLayout(int width, MessageElementFlags flags)
         this->deleteBuffer();
     }
 
-    this->container_.end();
+    this->container_.endLayout();
     this->height_ = this->container_.getHeight();
 
     // collapsed state
@@ -424,17 +424,17 @@ const MessageLayoutElement *MessageLayout::getElementAt(QPoint point)
     return this->container_.getElementAt(point);
 }
 
-int MessageLayout::getLastCharacterIndex() const
+size_t MessageLayout::getLastCharacterIndex() const
 {
     return this->container_.getLastCharacterIndex();
 }
 
-int MessageLayout::getFirstMessageCharacterIndex() const
+size_t MessageLayout::getFirstMessageCharacterIndex() const
 {
     return this->container_.getFirstMessageCharacterIndex();
 }
 
-int MessageLayout::getSelectionIndex(QPoint position)
+size_t MessageLayout::getSelectionIndex(QPoint position) const
 {
     return this->container_.getSelectionIndex(position);
 }

src/messages/layouts/MessageLayout.hpp

@@ -62,9 +62,23 @@ public:
 
     // Elements
     const MessageLayoutElement *getElementAt(QPoint point);
-    int getLastCharacterIndex() const;
+
-    int getFirstMessageCharacterIndex() const;
+    /**
-    int getSelectionIndex(QPoint position);
+     * Get the index of the last character in this message's container
+     * This is the sum of all the characters in `elements_`
+     */
+    size_t getLastCharacterIndex() const;
+
+    /**
+     * Get the index of the first visible character in this message's container
+     * This is not always 0 in case there elements that are skipped
+     */
+    size_t getFirstMessageCharacterIndex() const;
+
+    /**
+     * Get the character index at the given position, in the context of selections
+     */
+    size_t getSelectionIndex(QPoint position) const;
     void addSelectionText(QString &str, uint32_t from = 0,
                           uint32_t to = UINT32_MAX,
                           CopyMode copymode = CopyMode::Everything);

src/messages/layouts/MessageLayoutContainer.hpp

@@ -7,6 +7,7 @@
 #include <QRect>
 
 #include <memory>
+#include <optional>
 #include <vector>
 
 class QPainter;
@@ -20,87 +21,173 @@ class MessageLayoutElement;
 struct Selection;
 struct MessagePaintContext;
 
-struct Margin {
-    int top;
-    int right;
-    int bottom;
-    int left;
-
-    Margin()
-        : Margin(0)
-    {
-    }
-
-    Margin(int value)
-        : Margin(value, value, value, value)
-    {
-    }
-
-    Margin(int _top, int _right, int _bottom, int _left)
-        : top(_top)
-        , right(_right)
-        , bottom(_bottom)
-        , left(_left)
-    {
-    }
-};
-
 struct MessageLayoutContainer {
     MessageLayoutContainer() = default;
 
     FirstWord first = FirstWord::Neutral;
-    bool containsRTL = false;
 
-    int getHeight() const;
+    /**
+     * Begin the layout process of this message
+     *
+     * This will reset all line calculations, and will be considered incomplete
+     * until the accompanying end function has been called
+     */
+    void beginLayout(int width_, float scale_, MessageFlags flags_);
+
+    /**
+     * Finish the layout process of this message
+     */
+    void endLayout();
+
+    /**
+     * Add the given `element` to this message.
+     *
+     * This will also prepend a line break if the element
+     * does not fit in the current line
+     */
+    void addElement(MessageLayoutElement *element);
+
+    /**
+     * Add the given `element` to this message
+     */
+    void addElementNoLineBreak(MessageLayoutElement *element);
+
+    /**
+     * Break the current line
+     */
+    void breakLine();
+
+    /**
+     * Paint the elements in this message
+     */
+    void paintElements(QPainter &painter, const MessagePaintContext &ctx) const;
+
+    /**
+     * Paint the animated elements in this message
+     */
+    void paintAnimatedElements(QPainter &painter, int yOffset) const;
+
+    /**
+     * Paint the selection for this container
+     * This container contains one or more message elements
+     *
+     * @param painter The painter we draw everything to
+     * @param messageIndex This container's message index in the context of
+     *                     the layout we're being painted in
+     * @param selection The selection we need to paint
+     * @param yOffset The extra offset added to Y for everything that's painted
+     */
+    void paintSelection(QPainter &painter, size_t messageIndex,
+                        const Selection &selection, int yOffset) const;
+
+    /**
+     * Add text from this message into the `str` parameter
+     *
+     * @param[out] str The string where we append our selected text to
+     * @param from The character index from which we collecting our selected text
+     * @param to The character index where we stop collecting our selected text
+     * @param copymode Decides what from the message gets added to the selected text
+     */
+    void addSelectionText(QString &str, uint32_t from, uint32_t to,
+                          CopyMode copymode) const;
+
+    /**
+     * Returns a raw pointer to the element at the given point
+     *
+     * If no element is found at the given point, this returns a null pointer
+     */
+    MessageLayoutElement *getElementAt(QPoint point) const;
+
+    /**
+     * Get the character index at the given point, in the context of selections
+     */
+    size_t getSelectionIndex(QPoint point) const;
+
+    /**
+     * Get the index of the first visible character in this message
+     *
+     * This can be non-zero if there are elements in this message that are skipped
+     */
+    size_t getFirstMessageCharacterIndex() const;
+
+    /**
+     * Get the index of the last character in this message
+     * This is the sum of all the characters in `elements_`
+     */
+    size_t getLastCharacterIndex() const;
+
+    /**
+     * Returns the width of this message
+     */
     int getWidth() const;
+
+    /**
+     * Returns the height of this message
+     */
+    int getHeight() const;
+
+    /**
+     * Returns the scale of this message
+     */
     float getScale() const;
 
-    // methods
+    /**
-    void begin(int width_, float scale_, MessageFlags flags_);
+     * Returns true if this message is collapsed
-    void end();
+     */
-
-    void clear();
-    bool canAddElements() const;
-    void addElement(MessageLayoutElement *element);
-    void addElementNoLineBreak(MessageLayoutElement *element);
-    void breakLine();
-    bool atStartOfLine();
-    bool fitsInLine(int width_);
-    int remainingWidth() const;
-    // this method is called when a message has an RTL word
-    // we need to reorder the words to be shown properly
-    // however we don't we to reorder non-text elements like badges, timestamps, username
-    // firstTextIndex is the index of the first text element that we need to start the reordering from
-    void reorderRTL(int firstTextIndex);
-    MessageLayoutElement *getElementAt(QPoint point);
-
-    // painting
-    void paintElements(QPainter &painter, const MessagePaintContext &ctx);
-    void paintAnimatedElements(QPainter &painter, int yOffset);
-    void paintSelection(QPainter &painter, size_t messageIndex,
-                        const Selection &selection, int yOffset);
-
-    // selection
-    int getSelectionIndex(QPoint point);
-    int getLastCharacterIndex() const;
-    int getFirstMessageCharacterIndex() const;
-    void addSelectionText(QString &str, uint32_t from, uint32_t to,
-                          CopyMode copymode);
-
     bool isCollapsed() const;
 
+    /**
+     * Return true if we are at the start of a new line
+     */
+    bool atStartOfLine() const;
+
+    /**
+     * Check whether an additional `width` would fit in the current line
+     *
+     * Returns true if it does fit, false if not
+     */
+    bool fitsInLine(int width) const;
+
+    /**
+     * Returns the remaining width of this line until we will need to start a new line
+     */
+    int remainingWidth() const;
+
 private:
     struct Line {
-        int startIndex{};
+        /**
-        int endIndex{};
+         * The index of the first message element on this line
-        int startCharIndex{};
+         * Points into `elements_`
-        int endCharIndex{};
+         */
+        size_t startIndex{};
+
+        /**
+         * The index of the last message element on this line
+         * Points into `elements_`
+         */
+        size_t endIndex{};
+
+        /**
+         * In the context of selections, the index of the first character on this line
+         * The first line's startCharIndex will always be 0
+         */
+        size_t startCharIndex{};
+
+        /**
+         * In the context of selections, the index of the last character on this line
+         * The last line's startCharIndex will always be the sum of all characters in this message
+         */
+        size_t endCharIndex{};
+
+        /**
+         * The rectangle that covers all elements on this line
+         * This rectangle will always take up 100% of the view's width
+         */
         QRect rect;
     };
 
-    // helpers
     /*
-    _addElement is called at two stages. first stage is the normal one where we want to add message layout elements to the container.
+    addElement is called at two stages. first stage is the normal one where we want to add message layout elements to the container.
     If we detect an RTL word in the message, reorderRTL will be called, which is the second stage, where we call _addElement
     again for each layout element, but in the correct order this time, without adding the elemnt to the this->element_ vector.
     Due to compact emote logic, we need the previous element to check if we should change the spacing or not.
@@ -109,21 +196,76 @@ private:
     In stage one we don't need that and we pass -2 to indicate stage one (i.e. adding mode)
     In stage two, we pass -1 for the first element, and the index of the oredered privous element for the rest.
     */
-    void _addElement(MessageLayoutElement *element, bool forceAdd = false,
+    void addElement(MessageLayoutElement *element, bool forceAdd,
-                     int prevIndex = -2);
+                    int prevIndex);
-    bool canCollapse();
 
-    const Margin margin = {4, 8, 4, 8};
+    // this method is called when a message has an RTL word
+    // we need to reorder the words to be shown properly
+    // however we don't we to reorder non-text elements like badges, timestamps, username
+    // firstTextIndex is the index of the first text element that we need to start the reordering from
+    void reorderRTL(int firstTextIndex);
+
+    /**
+     * Paint a selection rectangle over the given line
+     *
+     * @param painter The painter we draw everything to
+     * @param line The line whose rect we use as the base top & bottom of the rect to paint
+     * @param left The left coordinates of the rect to paint
+     * @param right The right coordinates of the rect to paint
+     * @param yOffset Extra offset for line's top & bottom
+     * @param color Color of the selection
+     **/
+    void paintSelectionRect(QPainter &painter, const Line &line, int left,
+                            int right, int yOffset, const QColor &color) const;
+
+    /**
+     * Paint the selection start
+     *
+     * Returns a line index if this message should also paint the selection end
+     */
+    std::optional<size_t> paintSelectionStart(QPainter &painter,
+                                              size_t messageIndex,
+                                              const Selection &selection,
+                                              int yOffset) const;
+
+    /**
+     * Paint the selection end
+     *
+     * @param lineIndex The index of the line to start painting at
+     */
+    void paintSelectionEnd(QPainter &painter, size_t lineIndex,
+                           const Selection &selection, int yOffset) const;
+
+    /**
+     * canAddElements returns true if it's possible to add more elements to this message
+     */
+    bool canAddElements() const;
+
+    /**
+     * Return true if this message can collapse
+     *
+     * TODO: comment this better :-)
+     */
+    bool canCollapse() const;
 
     // variables
-    float scale_ = 1.f;
+    float scale_ = 1.F;
     int width_ = 0;
     MessageFlags flags_{};
-    int line_ = 0;
+    /**
+     * line_ is the current line index we are adding
+     * This is not the number of lines this message contains, since this will stop
+     * incrementing if the message is collapsed
+     */
+    size_t line_{};
     int height_ = 0;
     int currentX_ = 0;
     int currentY_ = 0;
-    int charIndex_ = 0;
+    /**
+     * charIndex_ is the selection-contexted index of where we currently are in our message
+     * At the end, this will always be equal to the sum of `elements_` getSelectionIndexCount()
+     */
+    size_t charIndex_ = 0;
     size_t lineStart_ = 0;
     int lineHeight_ = 0;
     int spaceWidth_ = 4;
@@ -133,7 +275,19 @@ private:
     bool isCollapsed_ = false;
     bool wasPrevReversed_ = false;
 
+    /**
+     * containsRTL indicates whether or not any of the text in this message
+     * contains any right-to-left characters (e.g. arabic)
+     */
+    bool containsRTL = false;
+
     std::vector<std::unique_ptr<MessageLayoutElement>> elements_;
+
+    /**
+     * A list of lines covering this message
+     * A message that spans 3 lines in a view will have 3 elements in lines_
+     * These lines hold no relation to the elements that are in this
+     */
     std::vector<Line> lines_;
 };
 

src/messages/layouts/MessageLayoutElement.cpp

@@ -60,12 +60,12 @@ bool MessageLayoutElement::hasTrailingSpace() const
     return this->trailingSpace;
 }
 
-int MessageLayoutElement::getLine() const
+size_t MessageLayoutElement::getLine() const
 {
     return this->line_;
 }
 
-void MessageLayoutElement::setLine(int line)
+void MessageLayoutElement::setLine(size_t line)
 {
     this->line_ = line;
 }
@@ -132,7 +132,7 @@ void ImageLayoutElement::addCopyTextToString(QString &str, uint32_t from,
     }
 }
 
-int ImageLayoutElement::getSelectionIndexCount() const
+size_t ImageLayoutElement::getSelectionIndexCount() const
 {
     return this->trailingSpace ? 2 : 1;
 }
@@ -176,7 +176,7 @@ int ImageLayoutElement::getMouseOverIndex(const QPoint &abs) const
     return 0;
 }
 
-int ImageLayoutElement::getXFromIndex(int index)
+int ImageLayoutElement::getXFromIndex(size_t index)
 {
     if (index <= 0)
     {
@@ -224,7 +224,7 @@ void LayeredImageLayoutElement::addCopyTextToString(QString &str, uint32_t from,
     }
 }
 
-int LayeredImageLayoutElement::getSelectionIndexCount() const
+size_t LayeredImageLayoutElement::getSelectionIndexCount() const
 {
     return this->trailingSpace ? 2 : 1;
 }
@@ -304,7 +304,7 @@ int LayeredImageLayoutElement::getMouseOverIndex(const QPoint &abs) const
     return 0;
 }
 
-int LayeredImageLayoutElement::getXFromIndex(int index)
+int LayeredImageLayoutElement::getXFromIndex(size_t index)
 {
     if (index <= 0)
     {
@@ -422,7 +422,7 @@ void TextLayoutElement::addCopyTextToString(QString &str, uint32_t from,
     }
 }
 
-int TextLayoutElement::getSelectionIndexCount() const
+size_t TextLayoutElement::getSelectionIndexCount() const
 {
     return this->getText().length() + (this->trailingSpace ? 1 : 0);
 }
@@ -489,7 +489,7 @@ int TextLayoutElement::getMouseOverIndex(const QPoint &abs) const
     return this->getSelectionIndexCount() - (this->hasTrailingSpace() ? 1 : 0);
 }
 
-int TextLayoutElement::getXFromIndex(int index)
+int TextLayoutElement::getXFromIndex(size_t index)
 {
     auto app = getApp();
 
@@ -532,7 +532,7 @@ void TextIconLayoutElement::addCopyTextToString(QString &str, uint32_t from,
 {
 }
 
-int TextIconLayoutElement::getSelectionIndexCount() const
+size_t TextIconLayoutElement::getSelectionIndexCount() const
 {
     return this->trailingSpace ? 2 : 1;
 }
@@ -576,7 +576,7 @@ int TextIconLayoutElement::getMouseOverIndex(const QPoint &abs) const
     return 0;
 }
 
-int TextIconLayoutElement::getXFromIndex(int index)
+int TextIconLayoutElement::getXFromIndex(size_t index)
 {
     if (index <= 0)
     {
@@ -649,7 +649,7 @@ int ReplyCurveLayoutElement::getMouseOverIndex(const QPoint &abs) const
     return 0;
 }
 
-int ReplyCurveLayoutElement::getXFromIndex(int index)
+int ReplyCurveLayoutElement::getXFromIndex(size_t index)
 {
     if (index <= 0)
     {
@@ -664,7 +664,7 @@ void ReplyCurveLayoutElement::addCopyTextToString(QString &str, uint32_t from,
 {
 }
 
-int ReplyCurveLayoutElement::getSelectionIndexCount() const
+size_t ReplyCurveLayoutElement::getSelectionIndexCount() const
 {
     return 1;
 }

src/messages/layouts/MessageLayoutElement.hpp

@@ -40,8 +40,8 @@ public:
     MessageElement &getCreator() const;
     void setPosition(QPoint point);
     bool hasTrailingSpace() const;
-    int getLine() const;
+    size_t getLine() const;
-    void setLine(int line);
+    void setLine(size_t line);
 
     MessageLayoutElement *setTrailingSpace(bool value);
     MessageLayoutElement *setLink(const Link &link_);
@@ -49,12 +49,12 @@ public:
 
     virtual void addCopyTextToString(QString &str, uint32_t from = 0,
                                      uint32_t to = UINT32_MAX) const = 0;
-    virtual int getSelectionIndexCount() const = 0;
+    virtual size_t getSelectionIndexCount() const = 0;
     virtual void paint(QPainter &painter,
                        const MessageColors &messageColors) = 0;
     virtual void paintAnimated(QPainter &painter, int yOffset) = 0;
     virtual int getMouseOverIndex(const QPoint &abs) const = 0;
-    virtual int getXFromIndex(int index) = 0;
+    virtual int getXFromIndex(size_t index) = 0;
 
     const Link &getLink() const;
     const QString &getText() const;
@@ -68,7 +68,10 @@ private:
     QRect rect_;
     Link link_;
     MessageElement &creator_;
-    int line_{};
+    /**
+     * The line of the container this element is laid out at
+     */
+    size_t line_{};
 };
 
 // IMAGE
@@ -81,11 +84,11 @@ public:
 protected:
     void addCopyTextToString(QString &str, uint32_t from = 0,
                              uint32_t to = UINT32_MAX) const override;
-    int getSelectionIndexCount() const override;
+    size_t getSelectionIndexCount() const override;
     void paint(QPainter &painter, const MessageColors &messageColors) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
-    int getXFromIndex(int index) override;
+    int getXFromIndex(size_t index) override;
 
     ImagePtr image_;
 };
@@ -100,11 +103,11 @@ public:
 protected:
     void addCopyTextToString(QString &str, uint32_t from = 0,
                              uint32_t to = UINT32_MAX) const override;
-    int getSelectionIndexCount() const override;
+    size_t getSelectionIndexCount() const override;
     void paint(QPainter &painter, const MessageColors &messageColors) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
-    int getXFromIndex(int index) override;
+    int getXFromIndex(size_t index) override;
 
     std::vector<ImagePtr> images_;
     std::vector<QSize> sizes_;
@@ -153,11 +156,11 @@ public:
 protected:
     void addCopyTextToString(QString &str, uint32_t from = 0,
                              uint32_t to = UINT32_MAX) const override;
-    int getSelectionIndexCount() const override;
+    size_t getSelectionIndexCount() const override;
     void paint(QPainter &painter, const MessageColors &messageColors) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
-    int getXFromIndex(int index) override;
+    int getXFromIndex(size_t index) override;
 
     QColor color_;
     FontStyle style_;
@@ -177,11 +180,11 @@ public:
 protected:
     void addCopyTextToString(QString &str, uint32_t from = 0,
                              uint32_t to = UINT32_MAX) const override;
-    int getSelectionIndexCount() const override;
+    size_t getSelectionIndexCount() const override;
     void paint(QPainter &painter, const MessageColors &messageColors) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
-    int getXFromIndex(int index) override;
+    int getXFromIndex(size_t index) override;
 
 private:
     float scale;
@@ -199,10 +202,10 @@ protected:
     void paint(QPainter &painter, const MessageColors &messageColors) override;
     void paintAnimated(QPainter &painter, int yOffset) override;
     int getMouseOverIndex(const QPoint &abs) const override;
-    int getXFromIndex(int index) override;
+    int getXFromIndex(size_t index) override;
     void addCopyTextToString(QString &str, uint32_t from = 0,
                              uint32_t to = UINT32_MAX) const override;
-    int getSelectionIndexCount() const override;
+    size_t getSelectionIndexCount() const override;
 
 private:
     const QPen pen_;

src/widgets/helper/ChannelView.cpp

@@ -1519,7 +1519,7 @@ void ChannelView::mouseMoveEvent(QMouseEvent *event)
     if (this->isLeftMouseDown_)
     {
         // this->pause(PauseReason::Selecting, 300);
-        int index = layout->getSelectionIndex(relativePos);
+        auto index = layout->getSelectionIndex(relativePos);
 
         this->setSelection(this->selection_.start,
                            SelectionItem(messageIndex, index));