Skip to content

Commit

Permalink
Force LTR / logical order for text in GdiEngine (microsoft#12722)
Browse files Browse the repository at this point in the history
Some applications like `vim -H` implement their own BiDi reordering.
Previously we used `PolyTextOutW` which supported such arrangements,
but with a0527a1 and the switch to `ExtTextOutW` we broke such applications.
This commit restores the old behavior by reimplementing the basics
of `ExtTextOutW`'s internal workings while enforcing LTR ordering.

## Validation Steps Performed
* Create a text file with "ץחסק פחופפסנ חס קוח ז׳חסש ץקקטק פחטסץ"
  Viewing the text file with `vim -H` presents the contents as expected ✅
* Printing enwik8 is as fast as before ✅
* Font fallback for various eastern scripts in enwik8 works as expected ✅
* `DECDWL` double-width sequences ✅
* Horizontal scrolling (apart from producing expected artifacts) ✅

Closes microsoft#12294

(cherry picked from commit d97d9f0)
  • Loading branch information
lhecker authored and DHowett committed Mar 23, 2022
1 parent 31c66ef commit 660c000
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 20 deletions.
6 changes: 6 additions & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ CHARSETINFO
chcp
checkbox
checkboxes
Checkin
chh
Childitem
chk
Expand Down Expand Up @@ -915,6 +916,7 @@ getwriter
GFEh
Gfun
gfx
GGI
GHIJK
GHIJKL
GHIJKLM
Expand Down Expand Up @@ -1092,6 +1094,7 @@ ifndef
IFont
ifstream
IGNOREEND
IGNORELANGUAGE
IHigh
IHosted
iid
Expand Down Expand Up @@ -1458,6 +1461,7 @@ mscorlib
msctf
msctls
msdata
MSDL
msdn
msft
MSGCMDLINEF
Expand Down Expand Up @@ -2229,6 +2233,7 @@ srect
srv
srvinit
srvpipe
ssa
ssh
sstream
stackoverflow
Expand Down Expand Up @@ -2554,6 +2559,7 @@ USESHOWWINDOW
USESIZE
USESTDHANDLES
ushort
usp
USRDLL
utf
utils
Expand Down
6 changes: 5 additions & 1 deletion src/renderer/gdi/gdirenderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ namespace Microsoft::Console::Render
const int nIndex,
const LONG dwNewLong) noexcept;

static bool FontHasWesternScript(HDC hdc);

bool _fPaintStarted;

til::rect _invalidCharacters;
Expand Down Expand Up @@ -138,13 +140,15 @@ namespace Microsoft::Console::Render
COLORREF _lastFg;
COLORREF _lastBg;

enum class FontType : size_t
enum class FontType : uint8_t
{
Undefined,
Default,
Italic,
Soft
};
FontType _lastFontType;
bool _fontHasWesternScript = false;

XFORM _currentLineTransform;
LineRendition _currentLineRendition;
Expand Down
7 changes: 6 additions & 1 deletion src/renderer/gdi/lib/gdi.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<RootNamespace>gdi</RootNamespace>
<ProjectName>RendererGdi</ProjectName>
<TargetName>ConRenderGdi</TargetName>
<ConfigurationType>StaticLibrary</ConfigurationType>
<ConfigurationType>StaticLibrary</ConfigurationType>
</PropertyGroup>
<Import Project="$(SolutionDir)src\common.build.pre.props" />
<ItemGroup>
Expand All @@ -22,6 +22,11 @@
<ClInclude Include="..\gdirenderer.hpp" />
<ClInclude Include="..\precomp.h" />
</ItemGroup>
<ItemDefinitionGroup>
<Lib>
<AdditionalDependencies>usp10.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Lib>
</ItemDefinitionGroup>
<!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->
<Import Project="$(SolutionDir)src\common.build.post.props" />
</Project>
49 changes: 46 additions & 3 deletions src/renderer/gdi/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@

using namespace Microsoft::Console::Render;

// This is an excerpt of GDI's FontHasWesternScript() as
// used by InternalTextOut() which is part of ExtTextOutW().
bool GdiEngine::FontHasWesternScript(HDC hdc)
{
WORD glyphs[4];
return (GetGlyphIndicesW(hdc, L"dMr\"", 4, glyphs, GGI_MARK_NONEXISTING_GLYPHS) == 4) &&
(glyphs[0] != 0xFFFF && glyphs[1] != 0xFFFF && glyphs[2] != 0xFFFF && glyphs[3] != 0xFFFF);
}

// Routine Description:
// - Prepares internal structures for a painting operation.
// Arguments:
Expand Down Expand Up @@ -54,6 +63,8 @@ using namespace Microsoft::Console::Render;
_debugContext = GetDC(_debugWindow);
#endif

_lastFontType = FontType::Undefined;

return S_OK;
}

Expand Down Expand Up @@ -445,10 +456,42 @@ using namespace Microsoft::Console::Render;
for (size_t i = 0; i != _cPolyText; ++i)
{
const auto& t = _pPolyText[i];
if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags, &t.rcl, t.lpstr, t.n, t.pdx))

// The following if/else replicates the essentials of how ExtTextOutW() without ETO_IGNORELANGUAGE works.
// See InternalTextOut().
//
// Unlike the original, we don't check for `GetTextCharacterExtra(hdc) != 0`,
// because we don't ever call SetTextCharacterExtra() anyways.
//
// GH#12294:
// Additionally we set ss.fOverrideDirection to TRUE, because we need to present RTL
// text in logical order in order to be compatible with applications like `vim -H`.
if (_fontHasWesternScript && ScriptIsComplex(t.lpstr, t.n, SIC_COMPLEX) == S_FALSE)
{
hr = E_FAIL;
break;
if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags | ETO_IGNORELANGUAGE, &t.rcl, t.lpstr, t.n, t.pdx))
{
hr = E_FAIL;
break;
}
}
else
{
SCRIPT_STATE ss{};
ss.fOverrideDirection = TRUE;

SCRIPT_STRING_ANALYSIS ssa;
hr = ScriptStringAnalyse(_hdcMemoryContext, t.lpstr, t.n, 0, -1, SSA_GLYPHS | SSA_FALLBACK, 0, nullptr, &ss, t.pdx, nullptr, nullptr, &ssa);
if (FAILED(hr))
{
break;
}

hr = ScriptStringOut(ssa, t.x, t.y, t.uiFlags, &t.rcl, 0, 0, FALSE);
std::ignore = ScriptStringFree(&ssa);
if (FAILED(hr))
{
break;
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/renderer/gdi/precomp.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ Module Name:
// This includes support libraries from the CRT, STL, WIL, and GSL
#include "LibraryIncludes.h"

#include <windows.h>
#include <Windows.h>
#include <windowsx.h>
#include <usp10.h>

#ifndef _NTSTATUS_DEFINED
#define _NTSTATUS_DEFINED
Expand Down
19 changes: 5 additions & 14 deletions src/renderer/gdi/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ GdiEngine::GdiEngine() :
_fInvalidRectUsed(false),
_lastFg(INVALID_COLOR),
_lastBg(INVALID_COLOR),
_lastFontType(FontType::Default),
_lastFontType(FontType::Undefined),
_currentLineTransform(IDENTITY_XFORM),
_currentLineRendition(LineRendition::SingleWidth),
_fPaintStarted(false),
Expand Down Expand Up @@ -142,15 +142,6 @@ GdiEngine::~GdiEngine()
_hwndTargetWindow = hwnd;
_hdcMemoryContext = hdcNewMemoryContext;

// If we have a font, apply it to the context.
if (nullptr != _hfont)
{
LOG_HR_IF_NULL(E_FAIL, SelectFont(_hdcMemoryContext, _hfont));
}

// Record the fact that the selected font is the default.
_lastFontType = FontType::Default;

if (nullptr != hdcRealWindow)
{
LOG_HR_IF(E_FAIL, !(ReleaseDC(_hwndTargetWindow, hdcRealWindow)));
Expand Down Expand Up @@ -327,6 +318,7 @@ GdiEngine::~GdiEngine()
break;
}
_lastFontType = fontType;
_fontHasWesternScript = FontHasWesternScript(_hdcMemoryContext);
}

return S_OK;
Expand All @@ -348,9 +340,6 @@ GdiEngine::~GdiEngine()
// Select into DC
RETURN_HR_IF_NULL(E_FAIL, SelectFont(_hdcMemoryContext, hFont.get()));

// Record the fact that the selected font is the default.
_lastFontType = FontType::Default;

// Save off the font metrics for various other calculations
RETURN_HR_IF(E_FAIL, !(GetTextMetricsW(_hdcMemoryContext, &_tmFontMetrics)));

Expand Down Expand Up @@ -456,7 +445,9 @@ GdiEngine::~GdiEngine()
const SIZE cellSize,
const size_t centeringHint) noexcept
{
// If the soft font is currently selected, replace it with the default font.
// If we previously called SelectFont(_hdcMemoryContext, _softFont), it will
// still hold a reference to the _softFont object we're planning to overwrite.
// --> First revert back to the standard _hfont, lest we have dangling pointers.
if (_lastFontType == FontType::Soft)
{
RETURN_HR_IF_NULL(E_FAIL, SelectFont(_hdcMemoryContext, _hfont));
Expand Down

0 comments on commit 660c000

Please sign in to comment.