Batch RTL runs to ensure proper draw order (#7190)

Consecutive RTL GlyphRuns are drawn from the last to the first. References #538, #7149, all those issues asking for RTL closed as dupes. As @miniksa suggested in a comment on #7149 -- handle the thingy on the render side. If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in order abcdGFEh. This has ransom-noting, because I didn't touch the font scaling at all. This should fix the majority of RTL issues, except it *doesn't* fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase.
microsoft · Aug 7, 2020 · 60b44c8 · 60b44c8
1 parent 1c6aa4d
commit 60b44c8
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 75 deletions.
diff --git a/.github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt b/.github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt
@@ -0,0 +1,10 @@
+abcd
+dst
+EFG
+EFGh
+EMPTYBOX
+GFEh
+nrcs
+Remoting
+Scs
+Shobjidl
diff --git a/.github/actions/spell-check/expect/expect.txt b/.github/actions/spell-check/expect/expect.txt
@@ -99,7 +99,6 @@ AStomps
 ASYNCWINDOWPOS
 atch
 ATest
-atg
 attr
 ATTRCOLOR
 aumid
@@ -160,7 +159,6 @@ bitsavers
 bitset
 BKCOLOR
 BKGND
-BKMK
 Bksp
 blog
 Blt
@@ -295,7 +293,6 @@ codepage
 codepoint
 codeproject
 COINIT
-colo
 colorizing
 colororacle
 colorref
@@ -352,7 +349,6 @@ conpty
 conptylib
 consecteturadipiscingelit
 conserv
-consoleaccessibility
 consoleapi
 CONSOLECONTROL
 CONSOLEENDTASK
@@ -640,7 +636,6 @@ DROPDOWNLIST
 DROPFILES
 drv
 dsm
-Dst
 DSwap
 DTest
 dtor
@@ -683,7 +678,6 @@ Elems
 elif
 elseif
 emacs
-emptybox
 enabledelayedexpansion
 endian
 endif
@@ -752,7 +746,6 @@ fdw
 fea
 fesb
 FFDE
-FFF
 FFrom
 FGCOLOR
 fgetc
@@ -769,7 +762,6 @@ FILETYPE
 FILEW
 FILLATTR
 FILLCONSOLEOUTPUT
-Filledbox
 FILTERONPASTE
 finalizer
 FINDCASE
@@ -821,7 +813,6 @@ fsproj
 fstream
 fte
 Ftm
-fullcolor
 fullscreen
 fullwidth
 func
@@ -1011,7 +1002,6 @@ hwheel
 hwnd
 HWNDPARENT
 hxx
-hyperlink
 IAccessibility
 IAction
 IApi
@@ -1034,7 +1024,6 @@ ICore
 IData
 IDCANCEL
 IDD
-IDefault
 IDesktop
 IDictionary
 IDISHWND
@@ -1384,7 +1373,6 @@ mincore
 mindbogglingly
 mingw
 minkernel
-minmax
 minwin
 minwindef
 Mip
@@ -1545,7 +1533,6 @@ NOYIELD
 NOZORDER
 NPM
 npos
-NRCS
 NSTATUS
 ntapi
 ntcon
@@ -1722,7 +1709,6 @@ pguid
 pgup
 PHANDLE
 phhook
-phsl
 phwnd
 pid
 pidl
@@ -1752,7 +1738,6 @@ POBJECT
 Podcast
 POINTSLIST
 POLYTEXTW
-popclip
 popd
 POPF
 poppack
@@ -1935,15 +1920,13 @@ REGSTR
 reingest
 Relayout
 RELBINPATH
-remoting
 renderengine
 rendersize
 reparent
 reparenting
 replatformed
 Replymessage
 repositorypath
-rerendered
 rescap
 Resequence
 Reserialize
@@ -1989,7 +1972,6 @@ RMENU
 roadmap
 robomac
 roundtrip
-ROWSTOSCROLL
 rparen
 RRF
 RRRGGGBB
@@ -2051,7 +2033,6 @@ SCROLLSCALE
 SCROLLSCREENBUFFER
 Scrollup
 Scrolluppage
-SCS
 scursor
 sddl
 sdeleted
@@ -2121,7 +2102,6 @@ Shl
 shlguid
 shlobj
 shlwapi
-shobjidl
 SHORTPATH
 SHOWCURSOR
 SHOWMAXIMIZED
@@ -2194,14 +2174,11 @@ STARTWPARMSW
 Statusline
 stdafx
 STDAPI
-stdarg
 stdcall
-stddef
 stderr
 stdexcept
 stdin
 stdio
-stdlib
 STDMETHODCALLTYPE
 STDMETHODIMP
 stdout
@@ -2295,7 +2272,6 @@ telnetd
 telnetpp
 templated
 terminalcore
-terminalnuget
 TERMINALSCROLLING
 terminfo
 TEs
@@ -2351,7 +2327,6 @@ TMult
 tmultiple
 tmux
 todo
-Tofill
 tofrom
 tokenhelpers
 tokenized
@@ -2447,7 +2422,6 @@ undef
 Unescape
 unexpand
 Unfocus
-unfocuses
 unhighlighting
 unhosted
 unicode
@@ -2753,33 +2727,26 @@ WWith
 wx
 wxh
 xa
-xab
 xact
 xamarin
 xaml
 Xamlmeta
 xargs
 xaz
-xb
-xbc
 xbf
 xbutton
 XBUTTONDBLCLK
 XBUTTONDOWN
 XBUTTONUP
-xca
 XCast
-xce
 XCENTER
 XColors
 xcopy
 XCount
-xdd
 xdy
 xe
 XEncoding
 xff
-xffff
 XFile
 xlang
 XManifest

diff --git a/src/renderer/dx/CustomTextLayout.cpp b/src/renderer/dx/CustomTextLayout.cpp
@@ -847,57 +847,113 @@ CATCH_RETURN();
         auto mutableOrigin = origin;
 
         // Draw each run separately.
-        for (UINT32 runIndex = 0; runIndex < _runs.size(); ++runIndex)
+        for (INT32 runIndex = 0; runIndex < gsl::narrow<INT32>(_runs.size()); ++runIndex)
         {
             // Get the run
             const 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;
-            glyphRun.bidiLevel = run.bidiLevel;
-            glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale;
-            glyphRun.fontFace = run.fontFace.Get();
-            glyphRun.glyphAdvances = &_glyphAdvances.at(run.glyphStart);
-            glyphRun.glyphCount = run.glyphCount;
-            glyphRun.glyphIndices = &_glyphIndices.at(run.glyphStart);
-            glyphRun.glyphOffsets = &_glyphOffsets.at(run.glyphStart);
-            glyphRun.isSideways = false;
-
-            DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription;
-            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))
+            if (!WI_IsFlagSet(run.bidiLevel, 1))
             {
-                mutableOrigin.x = postOriginX;
+                RETURN_IF_FAILED(_DrawGlyphRun(clientDrawingContext, renderer, mutableOrigin, run));
             }
+            // This is the RTL behavior. We will advance to the last contiguous RTL run, draw that,
+            // and then keep on going backwards from there, and then move runIndex beyond.
+            // Let's say we have runs abcdEFGh, where runs EFG are RTL.
+            // Then we will draw them in the order abcdGFEh
+            else
+            {
+                const INT32 originalRunIndex = runIndex;
+                INT32 lastIndexRTL = runIndex;
+
+                // Step 1: Get to the last contiguous RTL run from here
+                while (lastIndexRTL < gsl::narrow<INT32>(_runs.size()) - 1) // only could ever advance if there's something left
+                {
+                    const Run& nextRun = _runs.at(gsl::narrow_cast<size_t>(lastIndexRTL + 1));
+                    if (WI_IsFlagSet(nextRun.bidiLevel, 1))
+                    {
+                        lastIndexRTL++;
+                    }
+                    else
+                    {
+                        break;
+                    }
+                }
+
+                // Go from the last to the first and draw
+                for (runIndex = lastIndexRTL; runIndex >= originalRunIndex; runIndex--)
+                {
+                    const Run& currentRun = _runs.at(runIndex);
+                    RETURN_IF_FAILED(_DrawGlyphRun(clientDrawingContext, renderer, mutableOrigin, currentRun));
+                }
+                runIndex = lastIndexRTL; // and the for loop will take the increment to the last one
+            }
+        }
+    }
+    CATCH_RETURN();
+    return S_OK;
+}
 
-            // Try to draw it
-            RETURN_IF_FAILED(renderer->DrawGlyphRun(clientDrawingContext,
-                                                    mutableOrigin.x,
-                                                    mutableOrigin.y,
-                                                    DWRITE_MEASURING_MODE_NATURAL,
-                                                    &glyphRun,
-                                                    &glyphRunDescription,
-                                                    run.drawingEffect.Get()));
-
-            // Either way, we should be at this point by the end of writing this sequence,
-            // whether it was LTR or RTL.
+// Routine Description:
+// - Draw the given run
+// - The origin is updated to be after the run.
+// Arguments:
+// - clientDrawingContext - Optional pointer to information that the renderer might need
+//                          while attempting to graphically place the text onto the screen
+// - renderer - The interface to be used for actually putting text onto the screen
+// - origin - pixel point of top left corner on final surface for drawing
+// - run - the run to be drawn
+[[nodiscard]] HRESULT CustomTextLayout::_DrawGlyphRun(_In_opt_ void* clientDrawingContext,
+                                                      gsl::not_null<IDWriteTextRenderer*> renderer,
+                                                      D2D_POINT_2F& mutableOrigin,
+                                                      const Run& run) noexcept
+{
+    try
+    {
+        // Prepare the glyph run and description objects by converting our
+        // internal storage representation into something that matches DWrite's structures.
+        DWRITE_GLYPH_RUN glyphRun;
+        glyphRun.bidiLevel = run.bidiLevel;
+        glyphRun.fontEmSize = _format->GetFontSize() * run.fontScale;
+        glyphRun.fontFace = run.fontFace.Get();
+        glyphRun.glyphAdvances = &_glyphAdvances.at(run.glyphStart);
+        glyphRun.glyphCount = run.glyphCount;
+        glyphRun.glyphIndices = &_glyphIndices.at(run.glyphStart);
+        glyphRun.glyphOffsets = &_glyphOffsets.at(run.glyphStart);
+        glyphRun.isSideways = false;
+
+        DWRITE_GLYPH_RUN_DESCRIPTION glyphRunDescription;
+        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,
+                                                run.drawingEffect.Get()));
+
+        // Either way, we should be at this point by the end of writing this sequence,
+        // whether it was LTR or RTL.
+        mutableOrigin.x = postOriginX;
     }
     CATCH_RETURN();
     return S_OK;