fix: sort elements after RTL reordering (#5525)

Chatterino · Jul 28, 2024 · 5ee5abf · 5ee5abf
1 parent 5fc4309
commit 5ee5abf
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,7 @@
 - Bugfix: Fixed `/clearmessages` not working with more than one window. (#5489)
 - Bugfix: Fixed splits staying paused after unfocusing Chatterino in certain configurations. (#5504)
 - Bugfix: Links with invalid characters in the domain are no longer detected. (#5509)
+- Bugfix: Fixed janky selection for messages with RTL segments (selection is still wrong, but consistently wrong). (#5525)
 - 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)

diff --git a/src/messages/layouts/MessageLayoutContainer.cpp b/src/messages/layouts/MessageLayoutContainer.cpp
@@ -56,6 +56,8 @@ void MessageLayoutContainer::beginLayout(int width, float scale,
     this->currentWordId_ = 0;
     this->canAddMessages_ = true;
     this->isCollapsed_ = false;
+    this->lineContainsRTL_ = false;
+    this->anyReorderingDone_ = false;
 }
 
 void MessageLayoutContainer::endLayout()
@@ -105,6 +107,17 @@ void MessageLayoutContainer::endLayout()
     {
         this->elements_.back()->setTrailingSpace(false);
     }
+
+    if (this->anyReorderingDone_)
+    {
+        std::ranges::sort(this->elements_, [](const auto &a, const auto &b) {
+            if (a->getLine() == b->getLine())
+            {
+                return a->getRect().x() < b->getRect().x();
+            }
+            return a->getLine() < b->getLine();
+        });
+    }
 }
 
 void MessageLayoutContainer::addElement(MessageLayoutElement *element)
@@ -137,6 +150,7 @@ void MessageLayoutContainer::breakLine()
             }
         }
         this->lineContainsRTL_ = false;
+        this->anyReorderingDone_ = true;
     }
 
     int xOffset = 0;

diff --git a/src/messages/layouts/MessageLayoutContainer.hpp b/src/messages/layouts/MessageLayoutContainer.hpp
@@ -357,6 +357,9 @@ struct MessageLayoutContainer {
     /// linebreak after which it's reset to `false`.
     bool lineContainsRTL_ = false;
 
+    /// True if there was any RTL/LTR reordering done in this container
+    bool anyReorderingDone_ = false;
+
     /// @brief The direction of the text in this container.
     ///
     /// This starts off as neutral until an element is encountered that is

diff --git a/tests/src/MessageLayoutContainer.cpp b/tests/src/MessageLayoutContainer.cpp
@@ -118,34 +118,31 @@ TEST_P(MessageLayoutContainerTest, RtlReordering)
                                                MessageElementFlag::TwitchEmote,
                                            });
     }
-    container.breakLine();
+    container.endLayout();
     ASSERT_EQ(container.line_, 1) << "unexpected linebreak";
 
-    // message layout elements ordered by x position
-    std::vector<MessageLayoutElement *> ordered;
-    ordered.reserve(container.elements_.size());
+    int x = -1;
     for (const auto &el : container.elements_)
     {
-        ordered.push_back(el.get());
+        ASSERT_LT(x, el->getRect().x());
+        x = el->getRect().x();
     }
 
-    std::ranges::sort(ordered, [](auto *a, auto *b) {
-        return a->getRect().x() < b->getRect().x();
-    });
-
     QString got;
-    for (const auto &el : ordered)
+    for (const auto &el : container.elements_)
     {
         if (!got.isNull())
         {
             got.append(' ');
         }
 
-        if (dynamic_cast<ImageLayoutElement *>(el))
+        if (dynamic_cast<ImageLayoutElement *>(el.get()))
         {
             el->addCopyTextToString(got);
-            ASSERT_TRUE(got.endsWith(' '));
-            got.chop(1);
+            if (el->hasTrailingSpace())
+            {
+                got.chop(1);
+            }
         }
         else
         {