chore: cleanup, document, and test some RTL code

CHANGELOG.md

@@ -55,6 +55,7 @@
 - Dev: Deprecate Qt 5.12. (#5396)
 - Dev: The running Qt version is now shown in the about page if it differs from the compiled version. (#5501)
 - Dev: `FlagsEnum` is now `constexpr`. (#5510)
+- Dev: Documented and added tests to RTL handling. (#5473)
 
 ## 2.5.1
 

src/messages/Image.cpp

@@ -53,24 +53,27 @@ Frames::Frames(QList<Frame> &&frames)
             getApp()->getEmotes()->getGIFTimer().signal.connect([this] {
                 this->advance();
             });
-    }
 
-    auto totalLength = std::accumulate(this->items_.begin(), this->items_.end(),
-                                       0UL, [](auto init, auto &&frame) {
-                                           return init + frame.duration;
-                                       });
+        auto totalLength =
+            std::accumulate(this->items_.begin(), this->items_.end(), 0UL,
+                            [](auto init, auto &&frame) {
+                                return init + frame.duration;
+                            });
 
-    if (totalLength == 0)
-    {
-        this->durationOffset_ = 0;
-    }
-    else
-    {
-        this->durationOffset_ = std::min<int>(
-            int(getApp()->getEmotes()->getGIFTimer().position() % totalLength),
-            60000);
+        if (totalLength == 0)
+        {
+            this->durationOffset_ = 0;
+        }
+        else
+        {
+            this->durationOffset_ = std::min<int>(
+                int(getApp()->getEmotes()->getGIFTimer().position() %
+                    totalLength),
+                60000);
+        }
+        this->processOffset();
     }
-    this->processOffset();
+
     DebugCount::increase("image bytes", this->memoryUsage());
     DebugCount::increase("image bytes (ever loaded)", this->memoryUsage());
 }

src/messages/MessageElement.cpp

@@ -593,8 +593,6 @@ void SingleLineTextElement::addToContainer(MessageLayoutContainer &container,
         // once we encounter an emote or reach the end of the message text. */
         QString currentText;
 
-        container.first = FirstWord::Neutral;
-
         bool firstIteration = true;
         for (Word &word : this->words_)
         {

src/messages/layouts/MessageLayout.hpp

@@ -119,9 +119,9 @@ class MessageLayout
     QPixmap *ensureBuffer(QPainter &painter, int width);
 
     // variables
-    MessagePtr message_;
+    const MessagePtr message_;
     MessageLayoutContainer container_;
-    std::unique_ptr<QPixmap> buffer_{};
+    std::unique_ptr<QPixmap> buffer_;
     bool bufferValid_ = false;
 
     int height_ = 0;

src/messages/layouts/MessageLayoutContainer.cpp

@@ -1,4 +1,4 @@
-#include "MessageLayoutContainer.hpp"
+#include "messages/layouts/MessageLayoutContainer.hpp"
 
 #include "Application.hpp"
 #include "messages/layouts/MessageLayoutContext.hpp"
@@ -14,6 +14,7 @@
 #include <QDebug>
 #include <QMargins>
 #include <QPainter>
+#include <QVarLengthArray>
 
 #include <optional>
 
@@ -55,7 +56,6 @@ void MessageLayoutContainer::beginLayout(int width, float scale,
     this->currentWordId_ = 0;
     this->canAddMessages_ = true;
     this->isCollapsed_ = false;
-    this->wasPrevReversed_ = false;
 }
 
 void MessageLayoutContainer::endLayout()
@@ -71,7 +71,7 @@ void MessageLayoutContainer::endLayout()
             QSize(this->dotdotdotWidth_, this->textLineHeight_),
             QColor("#00D80A"), FontStyle::ChatMediumBold, this->scale_);
 
-        if (this->first == FirstWord::RTL)
+        if (this->isRTL())
         {
             // Shift all elements in the next line to the left
             for (auto i = this->lines_.back().startIndex;
@@ -125,9 +125,9 @@ void MessageLayoutContainer::addElementNoLineBreak(
 
 void MessageLayoutContainer::breakLine()
 {
-    if (this->containsRTL)
+    if (this->lineContainsRTL_ || this->isRTL())
     {
-        for (int i = 0; i < this->elements_.size(); i++)
+        for (size_t i = 0; i < this->elements_.size(); i++)
         {
             if (this->elements_[i]->getFlags().has(
                     MessageElementFlag::Username))
@@ -136,6 +136,7 @@ void MessageLayoutContainer::breakLine()
                 break;
             }
         }
+        this->lineContainsRTL_ = false;
     }
 
     int xOffset = 0;
@@ -404,7 +405,7 @@ size_t MessageLayoutContainer::getSelectionIndex(QPoint point) const
 
     size_t index = 0;
 
-    for (auto i = 0; i < lineEnd; i++)
+    for (size_t i = 0; i < lineEnd; i++)
     {
         auto &&element = this->elements_[i];
 
@@ -565,30 +566,37 @@ int MessageLayoutContainer::nextWordId()
 
 void MessageLayoutContainer::addElement(MessageLayoutElement *element,
                                         const bool forceAdd,
-                                        const int prevIndex)
+                                        const qsizetype prevIndex)
 {
     if (!this->canAddElements() && !forceAdd)
     {
+        assert(prevIndex == -2 &&
+               "element is still referenced in this->elements_");
         delete element;
         return;
     }
 
-    bool isRTLMode = this->first == FirstWord::RTL && prevIndex != -2;
     bool isAddingMode = prevIndex == -2;
-
-    // This lambda contains the logic for when to step one 'space width' back for compact x emotes
-    auto shouldRemoveSpaceBetweenEmotes = [this, prevIndex]() -> bool {
+    bool isRTLAdjusting = this->isRTL() && !isAddingMode;
+
+    /// Returns `true` if a previously added `spaceWidth_` should be removed
+    /// before the to be added emote. The space was inserted by the
+    /// previous element but isn't desired as "removeSpacesBetweenEmotes" is
+    /// enabled and both elements are emotes.
+    auto shouldRemoveSpaceBetweenEmotes = [this, prevIndex,
+                                           isAddingMode]() -> bool {
         if (prevIndex == -1 || this->elements_.empty())
         {
             // No previous element found
             return false;
         }
 
-        const auto &lastElement = prevIndex == -2 ? this->elements_.back()
-                                                  : this->elements_[prevIndex];
+        const auto &lastElement =
+            isAddingMode ? this->elements_.back() : this->elements_[prevIndex];
 
         if (!lastElement)
         {
+            assert(false && "Empty element in container found");
             return false;
         }
 
@@ -608,23 +616,24 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
         return lastElement->getFlags().has(MessageElementFlag::EmoteImages);
     };
 
-    if (element->getText().isRightToLeft())
+    bool isRTLElement = element->getText().isRightToLeft();
+    if (isRTLElement)
     {
-        this->containsRTL = true;
+        this->lineContainsRTL_ = true;
     }
 
     // check the first non-neutral word to see if we should render RTL or LTR
-    if (isAddingMode && this->first == FirstWord::Neutral &&
+    if (isAddingMode && this->isNeutral() &&
         element->getFlags().has(MessageElementFlag::Text) &&
         !element->getFlags().has(MessageElementFlag::RepliedMessage))
     {
-        if (element->getText().isRightToLeft())
+        if (isRTLElement)
         {
-            this->first = FirstWord::RTL;
+            this->textDirection_ = TextDirection::RTL;
         }
-        else if (!isNeutral(element->getText()))
+        else if (!chatterino::isNeutral(element->getText()))
         {
-            this->first = FirstWord::LTR;
+            this->textDirection_ = TextDirection::LTR;
         }
     }
 
@@ -665,7 +674,7 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
         shouldRemoveSpaceBetweenEmotes())
     {
         // Move cursor one 'space width' to the left (right in case of RTL) to combine hug the previous emote
-        if (isRTLMode)
+        if (isRTLAdjusting)
         {
             this->currentX_ += this->spaceWidth_;
         }
@@ -675,7 +684,7 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
         }
     }
 
-    if (isRTLMode)
+    if (isRTLAdjusting)
     {
         // shift by width since we are calculating according to top right in RTL mode
         // but setPosition wants top left
@@ -697,7 +706,7 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
     }
 
     // set current x
-    if (isRTLMode)
+    if (isRTLAdjusting)
     {
         this->currentX_ -= element->getRect().width();
     }
@@ -708,7 +717,7 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
 
     if (element->hasTrailingSpace())
     {
-        if (isRTLMode)
+        if (isRTLAdjusting)
         {
             this->currentX_ -= this->spaceWidth_;
         }
@@ -719,80 +728,69 @@ void MessageLayoutContainer::addElement(MessageLayoutElement *element,
     }
 }
 
-void MessageLayoutContainer::reorderRTL(int firstTextIndex)
+void MessageLayoutContainer::reorderRTL(size_t firstTextIndex)
 {
     if (this->elements_.empty())
     {
         return;
     }
 
-    int startIndex = static_cast<int>(this->lineStart_);
-    int endIndex = static_cast<int>(this->elements_.size()) - 1;
+    size_t startIndex = this->lineStart_;
+    size_t endIndex = this->elements_.size() - 1;
 
     if (firstTextIndex >= endIndex || startIndex >= this->elements_.size())
     {
         return;
     }
     startIndex = std::max(startIndex, firstTextIndex);
 
-    std::vector<int> correctSequence;
-    std::stack<int> swappedSequence;
-
-    // we reverse a sequence of words if it's opposite to the text direction
-    // the second condition below covers the possible three cases:
-    // 1 - if we are in RTL mode (first non-neutral word is RTL)
-    // we render RTL, reversing LTR sequences,
-    // 2 - if we are in LTR mode (first non-neutral word is LTR or all words are neutral)
-    // we render LTR, reversing RTL sequences
-    // 3 - neutral words follow previous words, we reverse a neutral word when the previous word was reversed
-
-    // the first condition checks if a neutral word is treated as a RTL word
-    // this is used later to add U+202B (RTL embedding) character signal to
-    // fix punctuation marks and mixing embedding LTR in an RTL word
-    // this can happen in two cases:
-    // 1 - in RTL mode, the previous word should be RTL (i.e. not reversed)
-    // 2 - in LTR mode, the previous word should be RTL (i.e. reversed)
-    for (int i = startIndex; i <= endIndex; i++)
+    QVarLengthArray<size_t, 32> correctSequence;
+    // temporary buffer to store elements in opposite order
+    QVarLengthArray<size_t, 32> swappedSequence;
+
+    bool isReversing = false;
+    for (size_t i = startIndex; i <= endIndex; i++)
     {
         auto &element = this->elements_[i];
 
-        const auto neutral = isNeutral(element->getText());
+        const auto neutral = chatterino::isNeutral(element->getText());
         const auto neutralOrUsername =
             neutral || element->getFlags().has(MessageElementFlag::Mention);
 
+        // check if neutral words are treated as RTL to add U+202B (RTL
+        // embedding) which fixes punctuation marks
         if (neutral &&
-            ((this->first == FirstWord::RTL && !this->wasPrevReversed_) ||
-             (this->first == FirstWord::LTR && this->wasPrevReversed_)))
+            ((this->isRTL() && !isReversing) || (this->isLTR() && isReversing)))
         {
             element->reversedNeutral = true;
         }
-        if (((element->getText().isRightToLeft() !=
-              (this->first == FirstWord::RTL)) &&
+
+        if ((element->getText().isRightToLeft() != this->isRTL() &&
              !neutralOrUsername) ||
-            (neutralOrUsername && this->wasPrevReversed_))
+            (neutralOrUsername && isReversing))
         {
-            swappedSequence.push(i);
-            this->wasPrevReversed_ = true;
+            swappedSequence.append(i);
+            isReversing = true;
         }
         else
         {
             while (!swappedSequence.empty())
             {
-                correctSequence.push_back(swappedSequence.top());
-                swappedSequence.pop();
+                correctSequence.push_back(swappedSequence.last());
+                swappedSequence.pop_back();
             }
             correctSequence.push_back(i);
-            this->wasPrevReversed_ = false;
+            isReversing = false;
         }
     }
     while (!swappedSequence.empty())
     {
-        correctSequence.push_back(swappedSequence.top());
-        swappedSequence.pop();
+        correctSequence.push_back(swappedSequence.last());
+        swappedSequence.pop_back();
     }
 
     // render right to left if we are in RTL mode, otherwise LTR
-    if (this->first == FirstWord::RTL)
+    if (this->isRTL())
     {
         this->currentX_ = this->elements_[endIndex]->getRect().right();
     }
@@ -806,10 +804,11 @@ void MessageLayoutContainer::reorderRTL(int firstTextIndex)
         this->addElement(this->elements_[correctSequence[0]].get(), false, -1);
     }
 
-    for (int i = 1; i < correctSequence.size() && this->canAddElements(); i++)
+    for (qsizetype i = 1; i < correctSequence.size() && this->canAddElements();
+         i++)
     {
         this->addElement(this->elements_[correctSequence[i]].get(), false,
-                         correctSequence[i - 1]);
+                         static_cast<qsizetype>(correctSequence[i - 1]));
     }
 }
 
@@ -992,4 +991,19 @@ bool MessageLayoutContainer::canCollapse() const
            this->flags_.has(MessageFlag::Collapsed);
 }
 
+bool MessageLayoutContainer::isRTL() const noexcept
+{
+    return this->textDirection_ == TextDirection::RTL;
+}
+
+bool MessageLayoutContainer::isLTR() const noexcept
+{
+    return this->textDirection_ == TextDirection::LTR;
+}
+
+bool MessageLayoutContainer::isNeutral() const noexcept
+{
+    return this->textDirection_ == TextDirection::Neutral;
+}
+
 }  // namespace chatterino