Fix double click to select full words (#5243)

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

CHANGELOG.md

@@ -83,6 +83,7 @@
 - Bugfix: Fixed badge highlight changes not immediately being reflected. (#5110)
 - Bugfix: Fixed emotes being reloaded when pressing "Cancel" in the settings dialog, causing a slowdown. (#5240)
 - Bugfix: Fixed some emotes not appearing when using _Ignores_. (#4965, #5126)
+- Bugfix: Fixed double-click selection not selecting words that were split onto multiple lines correctly. (#5243)
 - Bugfix: Fixed lookahead/-behind not working in _Ignores_. (#4965, #5126)
 - Bugfix: Fixed Image Uploader accidentally deleting images with some hosts when link resolver was enabled. (#4971)
 - Bugfix: Fixed rare crash with Image Uploader when closing a split right after starting an upload. (#4971)

src/messages/MessageElement.cpp

@@ -454,7 +454,7 @@ TextElement::TextElement(const QString &text, MessageElementFlags flags,
 void TextElement::addToContainer(MessageLayoutContainer &container,
                                  MessageElementFlags flags)
 {
-    auto *app = getApp();
+    auto *app = getIApp();
 
     if (flags.hasAny(this->getFlags()))
     {
@@ -463,6 +463,8 @@ void TextElement::addToContainer(MessageLayoutContainer &container,
 
         for (const auto &word : this->words_)
         {
+            auto wordId = container.nextWordId();
+
             auto getTextLayoutElement = [&](QString text, int width,
                                             bool hasTrailingSpace) {
                 auto color = this->color_.getColor(*app->getThemes());
@@ -473,6 +475,7 @@ void TextElement::addToContainer(MessageLayoutContainer &container,
                     this->style_, container.getScale());
                 e->setTrailingSpace(hasTrailingSpace);
                 e->setText(text);
+                e->setWordId(wordId);
 
                 return e;
             };

src/messages/layouts/MessageLayout.cpp

@@ -443,12 +443,31 @@ void MessageLayout::deleteCache()
 // returns nullptr if none was found
 
 // fourtf: this should return a MessageLayoutItem
-const MessageLayoutElement *MessageLayout::getElementAt(QPoint point)
+const MessageLayoutElement *MessageLayout::getElementAt(QPoint point) const
 {
     // go through all words and return the first one that contains the point.
     return this->container_.getElementAt(point);
 }
 
+std::pair<int, int> MessageLayout::getWordBounds(
+    const MessageLayoutElement *hoveredElement, QPoint relativePos) const
+{
+    // An element with wordId != -1 can be multiline, so we need to check all
+    // elements in the container
+    if (hoveredElement->getWordId() != -1)
+    {
+        return this->container_.getWordBounds(hoveredElement);
+    }
+
+    const auto wordStart = this->getSelectionIndex(relativePos) -
+                           hoveredElement->getMouseOverIndex(relativePos);
+    const auto selectionLength = hoveredElement->getSelectionIndexCount();
+    const auto length = hoveredElement->hasTrailingSpace() ? selectionLength - 1
+                                                           : selectionLength;
+
+    return {wordStart, wordStart + length};
+}
+
 size_t MessageLayout::getLastCharacterIndex() const
 {
     return this->container_.getLastCharacterIndex();

src/messages/layouts/MessageLayout.hpp

@@ -70,7 +70,21 @@ public:
      *
      * If no element is found at the given point, this returns a null pointer
      */
-    const MessageLayoutElement *getElementAt(QPoint point);
+    const MessageLayoutElement *getElementAt(QPoint point) const;
+
+    /**
+     * @brief Returns the word bounds of the given element
+     *
+     * The first value is the index of the first character in the word,
+     * the second value is the index of the character after the last character in the word.
+     *
+     * Given the word "abc" by itself, we would return (0, 3)
+     *
+     *  V  V
+     * "abc "
+     */
+    std::pair<int, int> getWordBounds(
+        const MessageLayoutElement *hoveredElement, QPoint relativePos) const;
 
     /**
      * Get the index of the last character in this message's container

src/messages/layouts/MessageLayoutContainer.cpp

@@ -51,6 +51,7 @@ void MessageLayoutContainer::beginLayout(int width, float scale,
     this->textLineHeight_ = mediumFontMetrics.height();
     this->spaceWidth_ = mediumFontMetrics.horizontalAdvance(' ');
     this->dotdotdotWidth_ = mediumFontMetrics.horizontalAdvance("...");
+    this->currentWordId_ = 0;
     this->canAddMessages_ = true;
     this->isCollapsed_ = false;
     this->wasPrevReversed_ = false;
@@ -456,6 +457,50 @@ size_t MessageLayoutContainer::getFirstMessageCharacterIndex() const
     return index;
 }
 
+std::pair<int, int> MessageLayoutContainer::getWordBounds(
+    const MessageLayoutElement *hoveredElement) const
+{
+    if (this->elements_.empty())
+    {
+        return {0, 0};
+    }
+
+    size_t index = 0;
+    size_t wordStart = 0;
+
+    for (; index < this->elements_.size(); index++)
+    {
+        const auto &element = this->elements_[index];
+        if (element->getWordId() == hoveredElement->getWordId())
+        {
+            break;
+        }
+
+        wordStart += element->getSelectionIndexCount();
+    }
+
+    size_t wordEnd = wordStart;
+
+    for (; index < this->elements_.size(); index++)
+    {
+        const auto &element = this->elements_[index];
+        if (element->getWordId() != hoveredElement->getWordId())
+        {
+            break;
+        }
+
+        wordEnd += element->getSelectionIndexCount();
+    }
+
+    const auto *lastElementInSelection = this->elements_[index - 1].get();
+    if (lastElementInSelection->hasTrailingSpace())
+    {
+        wordEnd--;
+    }
+
+    return {wordStart, wordEnd};
+}
+
 size_t MessageLayoutContainer::getLastCharacterIndex() const
 {
     if (this->lines_.empty())
@@ -505,6 +550,11 @@ int MessageLayoutContainer::remainingWidth() const
            this->currentX_;
 }
 
+int MessageLayoutContainer::nextWordId()
+{
+    return this->currentWordId_++;
+}
+
 void MessageLayoutContainer::addElement(MessageLayoutElement *element,
                                         const bool forceAdd,
                                         const int prevIndex)

src/messages/layouts/MessageLayoutContainer.hpp

@@ -111,6 +111,20 @@ struct MessageLayoutContainer {
      */
     size_t getFirstMessageCharacterIndex() const;
 
+    /**
+     * @brief Returns the word bounds of the given element
+     *
+     * The first value is the index of the first character in the word,
+     * the second value is the index of the character after the last character in the word.
+     *
+     * Given the word "abc" by itself, we would return (0, 3)
+     *
+     *  V  V
+     * "abc "
+     */
+    std::pair<int, int> getWordBounds(
+        const MessageLayoutElement *hoveredElement) const;
+
     /**
      * Get the index of the last character in this message
      * This is the sum of all the characters in `elements_`
@@ -154,6 +168,11 @@ struct MessageLayoutContainer {
      */
     int remainingWidth() const;
 
+    /**
+     * Returns the id of the next word that can be added to this container
+     */
+    int nextWordId();
+
 private:
     struct Line {
         /**
@@ -272,6 +291,7 @@ private:
     int spaceWidth_ = 4;
     int textLineHeight_ = 0;
     int dotdotdotWidth_ = 0;
+    int currentWordId_ = 0;
     bool canAddMessages_ = true;
     bool isCollapsed_ = false;
     bool wasPrevReversed_ = false;

src/messages/layouts/MessageLayoutElement.cpp

@@ -108,6 +108,16 @@ FlagsEnum<MessageElementFlag> MessageLayoutElement::getFlags() const
     return this->creator_.getFlags();
 }
 
+int MessageLayoutElement::getWordId() const
+{
+    return this->wordId_;
+}
+
+void MessageLayoutElement::setWordId(int wordId)
+{
+    this->wordId_ = wordId;
+}
+
 //
 // IMAGE
 //

src/messages/layouts/MessageLayoutElement.hpp

@@ -71,6 +71,9 @@ public:
     const QString &getText() const;
     FlagsEnum<MessageElementFlag> getFlags() const;
 
+    int getWordId() const;
+    void setWordId(int wordId);
+
 protected:
     bool trailingSpace = true;
 
@@ -83,6 +86,13 @@ private:
      * The line of the container this element is laid out at
      */
     size_t line_{};
+
+    /// @brief ID of a word inside its container
+    ///
+    /// One word has exactly one ID that is used to identify elements created
+    /// from the same word (due to wrapping).
+    /// IDs are unique in a MessageLayoutContainer.
+    int wordId_ = -1;
 };
 
 // IMAGE

src/widgets/helper/ChannelView.cpp

@@ -290,23 +290,6 @@ qreal highlightEasingFunction(qreal progress)
     return 1.0 + pow((20.0 / 9.0) * (0.5 * progress - 0.5), 3.0);
 }
 
-/// @return the start and end of the word bounds
-std::pair<int, int> getWordBounds(MessageLayout *layout,
-                                  const MessageLayoutElement *element,
-                                  const QPoint &relativePos)
-{
-    assert(layout != nullptr);
-    assert(element != nullptr);
-
-    const auto wordStart = layout->getSelectionIndex(relativePos) -
-                           element->getMouseOverIndex(relativePos);
-    const auto selectionLength = element->getSelectionIndexCount();
-    const auto length =
-        element->hasTrailingSpace() ? selectionLength - 1 : selectionLength;
-
-    return {wordStart, wordStart + length};
-}
-
 }  // namespace
 
 namespace chatterino {
@@ -1827,7 +1810,7 @@ void ChannelView::mouseMoveEvent(QMouseEvent *event)
     if (this->isDoubleClick_ && hoverLayoutElement)
     {
         auto [wordStart, wordEnd] =
-            getWordBounds(layout.get(), hoverLayoutElement, relativePos);
+            layout->getWordBounds(hoverLayoutElement, relativePos);
         auto hoveredWord = Selection{SelectionItem(messageIndex, wordStart),
                                      SelectionItem(messageIndex, wordEnd)};
         // combined selection spanning from initially selected word to hoveredWord
@@ -2657,7 +2640,8 @@ void ChannelView::mouseDoubleClickEvent(QMouseEvent *event)
     }
 
     auto [wordStart, wordEnd] =
-        getWordBounds(layout.get(), hoverLayoutElement, relativePos);
+        layout->getWordBounds(hoverLayoutElement, relativePos);
+
     this->doubleClickSelection_ = {SelectionItem(messageIndex, wordStart),
                                    SelectionItem(messageIndex, wordEnd)};
     this->setSelection(this->doubleClickSelection_);

tests/CMakeLists.txt

@@ -39,6 +39,7 @@ set(test_SOURCES
     ${CMAKE_CURRENT_LIST_DIR}/src/NotebookTab.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/SplitInput.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/LinkInfo.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/src/MessageLayout.cpp
     # Add your new file above this line!
     )
 

tests/src/MessageLayout.cpp

@@ -0,0 +1,90 @@
+#include "messages/layouts/MessageLayout.hpp"
+
+#include "Application.hpp"
+#include "controllers/accounts/AccountController.hpp"
+#include "messages/MessageBuilder.hpp"
+#include "messages/MessageElement.hpp"
+#include "mocks/EmptyApplication.hpp"
+#include "singletons/Emotes.hpp"
+#include "singletons/Fonts.hpp"
+#include "singletons/Settings.hpp"
+#include "singletons/Theme.hpp"
+#include "singletons/WindowManager.hpp"
+
+#include <gtest/gtest.h>
+#include <QDebug>
+#include <QString>
+
+#include <memory>
+
+using namespace chatterino;
+
+namespace {
+
+class MockApplication : mock::EmptyApplication
+{
+public:
+    MockApplication()
+        : settings(this->settingsDir.filePath("settings.json"))
+        , fonts(this->settings)
+        , windowManager(this->paths_)
+    {
+    }
+    Theme *getThemes() override
+    {
+        return &this->theme;
+    }
+
+    Fonts *getFonts() override
+    {
+        return &this->fonts;
+    }
+
+    WindowManager *getWindows() override
+    {
+        return &this->windowManager;
+    }
+
+    Settings settings;
+    Theme theme;
+    Fonts fonts;
+    WindowManager windowManager;
+};
+
+constexpr int WIDTH = 300;
+
+class MessageLayoutTest
+{
+public:
+    // "aaaaaaaa bbbbbbbb cccccccc"
+    MessageLayoutTest(const QString &text)
+    {
+        MessageBuilder builder;
+        builder.append(
+            std::make_unique<TextElement>(text, MessageElementFlag::Text));
+        this->layout = std::make_unique<MessageLayout>(builder.release());
+        this->layout->layout(WIDTH, 1, MessageElementFlag::Text, false);
+    }
+
+    MockApplication mockApplication;
+    std::unique_ptr<MessageLayout> layout;
+};
+
+}  // namespace
+
+TEST(TextElement, BasicCase)
+{
+    auto test = MessageLayoutTest("abc");
+
+    // Simulate we are clicking on the first word
+    auto point = QPoint(WIDTH / 20, test.layout->getHeight() / 2);
+
+    const auto *hoveredElement = test.layout->getElementAt(point);
+    ASSERT_NE(hoveredElement, nullptr);
+
+    const auto [wordStart, wordEnd] =
+        test.layout->getWordBounds(hoveredElement, point);
+
+    EXPECT_EQ(wordStart, 0);
+    EXPECT_EQ(wordEnd, 3);
+}