Repository: microsoft/terminal
File: src/renderer/dx/CustomTextLayout.cpp (C++)

Code before:
        // Draw each run separately.
        for (UINT32 runIndex = 0; runIndex < _runs.size(); ++runIndex)
        {
            // Get the run
            Run& run = _runs.at(runIndex);

            // Prepare the glyph run and description objects by converting our
            // internal storage representation into something that matches DWrite's structures.
            DWRITE_GLYPH_RUN glyphRun = { 0 };
            glyphRun.bidiLevel = run.bidiLevel;
            glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale;
            glyphRun.fontFace = run.fontFace.Get();
            glyphRun.glyphAdvances = _glyphAdvances.data() + run.glyphStart;
            glyphRun.glyphCount = run.glyphCount;
            glyphRun.glyphIndices = _glyphIndices.data() + run.glyphStart;
            glyphRun.glyphOffsets = _glyphOffsets.data() + run.glyphStart;
            glyphRun.isSideways = false;

            DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription = { 0 };
            glyphRunDescription.clusterMap = _glyphClusters.data();
            glyphRunDescription.localeName = _localeName.data();
            glyphRunDescription.string = _text.data();
            glyphRunDescription.stringLength = run.textLength;
            glyphRunDescription.textPosition = run.textStart;

            // Calculate the position *after* drawing, this will be used if we're in RTL.
            // Original comment:
            // Shift origin to the right for the next run based on the amount of space consumed.
            auto postOriginX = std::accumulate(_glyphAdvances.begin() + run.glyphStart,
                                               _glyphAdvances.begin() + run.glyphStart + run.glyphCount,
                                               mutableOrigin.x);

            if (WI_IsFlagSet(glyphRun.bidiLevel, 1))
                mutableOrigin.x = postOriginX;

            // Try to draw it
            RETURN_IF_FAILED(renderer->DrawGlyphRun(clientDrawingContext,
                                                    mutableOrigin.x,
                                                    mutableOrigin.y,
                                                    DWRITE_MEASURING_MODE_NATURAL,
                                                    &glyphRun,
                                                    &glyphRunDescription,
                                                    nullptr));

            mutableOrigin.x = postOriginX;

        }
    }
    CATCH_RETURN();
    return S_OK;
}

Review comment (suggestion):
The comment explains what it does, but not why. Please include here and/or below that the reason WHY we precalculate the final position is because DirectX expects the origin to the RIGHT of the text for RTL and LEFT for LTR.

Code after:
        // Draw each run separately.
        for (UINT32 runIndex = 0; runIndex < _runs.size(); ++runIndex)
        {
            // Get the run
            Run& run = _runs.at(runIndex);

            // Prepare the glyph run and description objects by converting our
            // internal storage representation into something that matches DWrite's structures.
            DWRITE_GLYPH_RUN glyphRun = { 0 };
            glyphRun.bidiLevel = run.bidiLevel;
            glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale;
            glyphRun.fontFace = run.fontFace.Get();
            glyphRun.glyphAdvances = _glyphAdvances.data() + run.glyphStart;
            glyphRun.glyphCount = run.glyphCount;
            glyphRun.glyphIndices = _glyphIndices.data() + run.glyphStart;
            glyphRun.glyphOffsets = _glyphOffsets.data() + run.glyphStart;
            glyphRun.isSideways = false;

            DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription = { 0 };
            glyphRunDescription.clusterMap = _glyphClusters.data();
            glyphRunDescription.localeName = _localeName.data();
            glyphRunDescription.string = _text.data();
            glyphRunDescription.stringLength = run.textLength;
            glyphRunDescription.textPosition = run.textStart;

            // Calculate the origin for the next run based on the amount of space
            // that would be consumed. We are doing this calculation now, not after,
            // because if the text is RTL then we need to advance immediately, before the
            // write call since DirectX expects the origin to the RIGHT of the text for RTL.
            const auto postOriginX = std::accumulate(_glyphAdvances.begin() + run.glyphStart,
                                                     _glyphAdvances.begin() + run.glyphStart + run.glyphCount,
                                                     mutableOrigin.x);

            // Check for RTL, if it is, apply space adjustment.
            if (WI_IsFlagSet(glyphRun.bidiLevel, 1))
            {
                mutableOrigin.x = postOriginX;
            }

            // Try to draw it
            RETURN_IF_FAILED(renderer->DrawGlyphRun(clientDrawingContext,
                                                    mutableOrigin.x,
                                                    mutableOrigin.y,
                                                    DWRITE_MEASURING_MODE_NATURAL,
                                                    &glyphRun,
                                                    &glyphRunDescription,
                                                    nullptr));

            // Either way, we should be at this point by the end of writing this sequence,
            // whether it was LTR or RTL.
            mutableOrigin.x = postOriginX;