Skip to content

Commit add09b5

Browse files
johnml1135johnml1135
committed
LT-22324: address OpenType feature review follow-ups
1 parent ad4f754 commit add09b5

13 files changed

Lines changed: 223 additions & 29 deletions

File tree

‎FieldWorks.sln‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RenderTestInfrastructure",
2222
EndProject
2323
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RenderVerification", "Src\Common\RenderVerification\RenderVerification.csproj", "{6F7A4D9C-5B44-4D0E-ABAA-8D6F38F30C6A}"
2424
EndProject
25+
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RenderComparisonTests", "Src\Common\RenderVerification\RenderComparisonTests\RenderComparisonTests.csproj", "{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}"
26+
EndProject
2527
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Discourse", "Src\LexText\Discourse\Discourse.csproj", "{A51BAFC3-1649-584D-8D25-101884EE9EAA}"
2628
EndProject
2729
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "DiscourseTests", "Src\LexText\Discourse\DiscourseTests\DiscourseTests.csproj", "{1CE6483D-5D10-51AD-B2A7-FD7F82CCBAB2}"
@@ -347,6 +349,12 @@ Global
347349
{6F7A4D9C-5B44-4D0E-ABAA-8D6F38F30C6A}.Debug|x64.Build.0 = Debug|x64
348350
{6F7A4D9C-5B44-4D0E-ABAA-8D6F38F30C6A}.Release|x64.ActiveCfg = Release|x64
349351
{6F7A4D9C-5B44-4D0E-ABAA-8D6F38F30C6A}.Release|x64.Build.0 = Release|x64
352+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Bounds|x64.ActiveCfg = Release|x64
353+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Bounds|x64.Build.0 = Release|x64
354+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Debug|x64.ActiveCfg = Debug|x64
355+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Debug|x64.Build.0 = Debug|x64
356+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Release|x64.ActiveCfg = Release|x64
357+
{5AF55AED-9E72-42CB-9A1E-C61AE6FE4613}.Release|x64.Build.0 = Release|x64
350358
{A51BAFC3-1649-584D-8D25-101884EE9EAA}.Bounds|x64.ActiveCfg = Release|x64
351359
{A51BAFC3-1649-584D-8D25-101884EE9EAA}.Bounds|x64.Build.0 = Release|x64
352360
{A51BAFC3-1649-584D-8D25-101884EE9EAA}.Debug|x64.ActiveCfg = Debug|x64

‎Src/Common/FwUtils/FontFeatureSettings.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,19 @@ public static string Normalize(string features)
5353
return string.Join(",", Parse(features).Select(setting => setting.ToString()));
5454
}
5555

56+
/// <summary>
57+
/// Returns a deterministic representation for OpenType feature strings while preserving
58+
/// legacy numeric Graphite feature identifiers.
59+
/// </summary>
60+
public static string NormalizePreservingLegacy(string features)
61+
{
62+
if (string.IsNullOrWhiteSpace(features))
63+
return string.Empty;
64+
65+
var trimmed = features.Trim();
66+
return char.IsLetter(trimmed[0]) ? Normalize(trimmed) : trimmed;
67+
}
68+
5669
/// <summary>
5770
/// Returns whether a string is a valid four-character OpenType feature tag.
5871
/// </summary>

‎Src/Common/FwUtils/FwUtilsTests/FontFeatureSettingsTests.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,5 +38,17 @@ public void Normalize_ReturnsDeterministicRendererNeutralString()
3838
{
3939
Assert.That(FontFeatureSettings.Normalize(" smcp = 1, kern=0 "), Is.EqualTo("kern=0,smcp=1"));
4040
}
41+
42+
[Test]
43+
public void NormalizePreservingLegacy_PreservesNumericGraphiteFeatureIds()
44+
{
45+
Assert.That(FontFeatureSettings.NormalizePreservingLegacy(" 123=1,456=2 "), Is.EqualTo("123=1,456=2"));
46+
}
47+
48+
[Test]
49+
public void NormalizePreservingLegacy_NormalizesOpenTypeFeatures()
50+
{
51+
Assert.That(FontFeatureSettings.NormalizePreservingLegacy(" smcp = 1, kern=0 "), Is.EqualTo("kern=0,smcp=1"));
52+
}
4153
}
4254
}

‎Src/Common/SimpleRootSite/RenderEngineFactory.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public IRenderEngine get_Renderer(ILgWritingSystem ws, IVwGraphics vg)
4040
{
4141
LgCharRenderProps chrp = vg.FontCharProperties;
4242
string fontName = MarshalEx.UShortToString(chrp.szFaceName);
43+
bool usesDefaultFont = fontName == "<default font>";
4344
if (fontName == "<default font>")
4445
{
4546
fontName = ws.DefaultFontName;
@@ -52,7 +53,7 @@ public IRenderEngine get_Renderer(ILgWritingSystem ws, IVwGraphics vg)
5253
wsFontEngines = new Dictionary<Tuple<string, bool, bool, string>, Tuple<bool, IRenderEngine>>();
5354
m_fontEngines[ws] = wsFontEngines;
5455
}
55-
string fontFeatures = GetFontFeatures(fontName, chrp, ws);
56+
string fontFeatures = GetFontFeatures(chrp, ws, usesDefaultFont);
5657
if (chrp.szFontVar != null)
5758
{
5859
MarshalEx.StringToUShort(fontFeatures ?? string.Empty, chrp.szFontVar);
@@ -85,16 +86,16 @@ public IRenderEngine get_Renderer(ILgWritingSystem ws, IVwGraphics vg)
8586
return wsFontEngines[key].Item2;
8687
}
8788

88-
private static string GetFontFeatures(string fontName, LgCharRenderProps chrp, ILgWritingSystem ws)
89+
private static string GetFontFeatures(LgCharRenderProps chrp, ILgWritingSystem ws, bool usesDefaultFont)
8990
{
9091
string charRenderFeatures = chrp.szFontVar == null
9192
? string.Empty
92-
: FontFeatureSettings.Normalize(MarshalEx.UShortToString(chrp.szFontVar));
93+
: FontFeatureSettings.NormalizePreservingLegacy(MarshalEx.UShortToString(chrp.szFontVar));
9394
if (!string.IsNullOrEmpty(charRenderFeatures))
9495
return charRenderFeatures;
9596

96-
if (fontName == ws.DefaultFontName)
97-
return FontFeatureSettings.Normalize(ws.DefaultFontFeatures);
97+
if (usesDefaultFont)
98+
return FontFeatureSettings.NormalizePreservingLegacy(ws.DefaultFontFeatures);
9899
return string.Empty;
99100
}
100101

‎Src/Common/SimpleRootSite/SimpleRootSiteTests/RenderEngineFactoryTests.cs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,62 @@ public void get_Renderer_OpenTypeFeatures_ArePartOfCacheIdentity()
217217
}
218218
}
219219

220+
[Test]
221+
public void get_Renderer_ExplicitFontNameMatchingDefault_DoesNotApplyDefaultFontFeatures()
222+
{
223+
using (var control = new Form())
224+
using (var gm = new GraphicsManager(control))
225+
using (var reFactory = new RenderEngineFactory())
226+
{
227+
gm.Init(1.0f);
228+
try
229+
{
230+
var wsManager = new WritingSystemManager();
231+
CoreWritingSystemDefinition ws = wsManager.Set("en-US");
232+
ws.DefaultFont = new FontDefinition("Arial") { Features = "smcp=1" };
233+
234+
var chrp = CreateCharRenderProps(ws.Handle, "Arial", string.Empty);
235+
gm.VwGraphics.SetupGraphics(ref chrp);
236+
237+
reFactory.get_Renderer(ws, gm.VwGraphics);
238+
Assert.That(MarshalEx.UShortToString(gm.VwGraphics.FontCharProperties.szFontVar), Is.EqualTo(string.Empty));
239+
wsManager.Save();
240+
}
241+
finally
242+
{
243+
gm.Uninit();
244+
}
245+
}
246+
}
247+
248+
[Test]
249+
public void get_Renderer_DefaultNumericGraphiteFeatures_ArePreserved()
250+
{
251+
using (var control = new Form())
252+
using (var gm = new GraphicsManager(control))
253+
using (var reFactory = new RenderEngineFactory())
254+
{
255+
gm.Init(1.0f);
256+
try
257+
{
258+
var wsManager = new WritingSystemManager();
259+
CoreWritingSystemDefinition ws = wsManager.Set("en-US");
260+
ws.DefaultFont = new FontDefinition("Arial") { Features = "123=1,456=2" };
261+
262+
var chrp = CreateCharRenderProps(ws.Handle, "<default font>", string.Empty);
263+
gm.VwGraphics.SetupGraphics(ref chrp);
264+
265+
reFactory.get_Renderer(ws, gm.VwGraphics);
266+
Assert.That(MarshalEx.UShortToString(gm.VwGraphics.FontCharProperties.szFontVar), Is.EqualTo("123=1,456=2"));
267+
wsManager.Save();
268+
}
269+
finally
270+
{
271+
gm.Uninit();
272+
}
273+
}
274+
}
275+
220276
private static LgCharRenderProps CreateCharRenderProps(
221277
int ws,
222278
string fontName,

‎Src/FwCoreDlgs/FwCoreDlgControls/DefaultFontsControl.cs‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,10 @@ protected void SetSelectedFonts()
242242
// setup controls for default font
243243
SetFontInCombo(m_defaultFontComboBox, m_ws.DefaultFontName);
244244
m_defaultFontFeaturesButton.WritingSystemFactory = m_ws.WritingSystemFactory;
245-
m_defaultFontFeaturesButton.FontFeatures = m_ws.DefaultFontFeatures;
246-
m_defaultFontFeaturesButton.UseGraphiteFeatures = m_ws.IsGraphiteEnabled;
247-
m_defaultFontFeaturesButton.FontName = m_defaultFontComboBox.Text;
248-
m_defaultFontFeaturesButton.SetupFontFeatures();
245+
m_defaultFontFeaturesButton.RefreshFeatureContext(
246+
m_defaultFontComboBox.Text,
247+
m_ws.DefaultFontFeatures,
248+
m_ws.IsGraphiteEnabled);
249249

250250
bool isGraphiteFont = m_defaultFontFeaturesButton.IsGraphiteFont;
251251
m_graphiteGroupBox.Enabled = isGraphiteFont || m_defaultFontFeaturesButton.HasFontFeatures;
@@ -304,10 +304,10 @@ private void m_defaultFontComboBox_SelectedIndexChanged(object sender, EventArgs
304304

305305
if (m_ws.DefaultFont != null)
306306
{
307-
m_defaultFontFeaturesButton.FontFeatures = m_ws.DefaultFont.Features;
308-
m_defaultFontFeaturesButton.UseGraphiteFeatures = false;
309-
m_defaultFontFeaturesButton.FontName = m_defaultFontComboBox.Text;
310-
m_defaultFontFeaturesButton.SetupFontFeatures();
307+
m_defaultFontFeaturesButton.RefreshFeatureContext(
308+
m_defaultFontComboBox.Text,
309+
m_ws.DefaultFont.Features,
310+
false);
311311
}
312312

313313
bool isGraphiteFont = m_defaultFontFeaturesButton.IsGraphiteFont;
@@ -337,8 +337,10 @@ private void m_enableGraphiteCheckBox_Click(object sender, EventArgs e)
337337
if (m_ws == null)
338338
return;
339339
m_ws.IsGraphiteEnabled = m_enableGraphiteCheckBox.Checked;
340-
m_defaultFontFeaturesButton.UseGraphiteFeatures = m_ws.IsGraphiteEnabled;
341-
m_defaultFontFeaturesButton.SetupFontFeatures();
340+
m_defaultFontFeaturesButton.RefreshFeatureContext(
341+
m_defaultFontComboBox.Text,
342+
m_defaultFontFeaturesButton.FontFeatures,
343+
m_ws.IsGraphiteEnabled);
342344
}
343345

344346
#endregion

‎Src/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Collections.Generic;
77
using System.Diagnostics;
88
using System.Drawing;
9+
using System.Globalization;
910
using System.Linq;
1011
using System.Runtime.InteropServices;
1112
using System.Windows.Forms;
@@ -288,10 +289,22 @@ public string FontFeatures
288289
set
289290
{
290291
CheckDisposed();
291-
m_fontFeatures = FontFeatureSettings.Normalize(value);
292+
m_fontFeatures = FontFeatureSettings.NormalizePreservingLegacy(value);
292293
}
293294
}
294295

296+
/// <summary>
297+
/// Updates the current font feature context and refreshes discovery exactly once.
298+
/// </summary>
299+
internal void RefreshFeatureContext(string fontName, string fontFeatures, bool useGraphiteFeatures)
300+
{
301+
CheckDisposed();
302+
m_fontName = fontName;
303+
m_fontFeatures = FontFeatureSettings.NormalizePreservingLegacy(fontFeatures);
304+
m_useGraphiteFeatures = useGraphiteFeatures;
305+
SetupFontFeatures();
306+
}
307+
295308
/// <summary>
296309
/// Gets or sets a value indicating whether Graphite feature discovery should be preferred
297310
/// when the current font supports both Graphite and OpenType features.
@@ -687,7 +700,9 @@ protected override void OnClick(EventArgs e)
687700

688701
var menu = components.ContextMenu("ContextMenu");
689702
m_ids = m_featureProvider.GetFeatureIds();
690-
var parserFeatureString = GraphiteFontFeatures.ConvertFontFeatureCodesToIds(m_fontFeatures);
703+
var parserFeatureString = m_featureProvider is OpenTypeFontFeatureProvider
704+
? ConvertRendererNeutralFeatureStringToIds(m_fontFeatures)
705+
: GraphiteFontFeatures.ConvertFontFeatureCodesToIds(m_fontFeatures);
691706
m_values = ParseFeatureString(m_ids, parserFeatureString);
692707
Debug.Assert(m_ids.Length == m_values.Length);
693708

@@ -986,6 +1001,14 @@ private static int ConvertFontFeatureCodeToId(string fontFeature)
9861001
return BitConverter.ToInt32(numbers, 0);
9871002
}
9881003

1004+
internal static string ConvertRendererNeutralFeatureStringToIds(string fontFeatures)
1005+
{
1006+
return string.Join(",", FontFeatureSettings.Parse(fontFeatures)
1007+
.Select(setting => string.Format(CultureInfo.InvariantCulture, "{0}={1}",
1008+
ConvertFontFeatureCodeToId(setting.Tag),
1009+
setting.Value)));
1010+
}
1011+
9891012
private static class OpenTypeFontFeatureReader
9901013
{
9911014
private const uint GdiError = 0xFFFFFFFF;

‎Src/FwCoreDlgs/FwCoreDlgControls/FwCoreDlgControlsTests/TestFontFeaturesButton.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,27 @@ public void FontFeatures_NormalizesRendererNeutralTags()
8686
}
8787
}
8888

89+
[Test]
90+
public void FontFeatures_PreservesLegacyGraphiteFeatureIds()
91+
{
92+
using (var button = new FontFeaturesButton())
93+
{
94+
button.FontFeatures = " 123=1,456=2 ";
95+
96+
Assert.That(button.FontFeatures, Is.EqualTo("123=1,456=2"));
97+
}
98+
}
99+
100+
[Test]
101+
public void ConvertRendererNeutralFeatureStringToIds_UsesOpenTypeTagsDirectly()
102+
{
103+
var expected = FeatureId("kern") + "=0," + FeatureId("smcp") + "=1";
104+
105+
Assert.That(
106+
FontFeaturesButton.ConvertRendererNeutralFeatureStringToIds(" smcp = 1, kern=0 "),
107+
Is.EqualTo(expected));
108+
}
109+
89110
private static int FeatureId(string tag)
90111
{
91112
var reversedTagBytes = tag.Reverse().Select(Convert.ToByte).ToArray();

‎Src/views/Test/TestUniscribeEngine.h‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,15 @@ namespace TestViews
4141
int dxWidth = 0;
4242
#if defined(WIN32) || defined(_M_X64)
4343
int dxMax = 4000;
44-
HDC hdc = ::CreateCompatibleDC(::GetDC(::GetDesktopWindow()));
45-
HBITMAP hbm = ::CreateCompatibleBitmap(hdc, dxMax, dxMax);
46-
::SelectObject(hdc, hbm);
44+
HWND hwndDesktop = ::GetDesktopWindow();
45+
HDC hdcDesktop = ::GetDC(hwndDesktop);
46+
unitpp::assert_true("GetDC should return the desktop DC", hdcDesktop != NULL);
47+
HDC hdc = ::CreateCompatibleDC(hdcDesktop);
48+
unitpp::assert_true("CreateCompatibleDC should return a memory DC", hdc != NULL);
49+
HBITMAP hbm = ::CreateCompatibleBitmap(hdcDesktop, dxMax, dxMax);
50+
unitpp::assert_true("CreateCompatibleBitmap should return a bitmap", hbm != NULL);
51+
HGDIOBJ hbmOld = ::SelectObject(hdc, hbm);
52+
unitpp::assert_true("SelectObject should select the bitmap into the memory DC", hbmOld != NULL);
4753
::SetMapMode(hdc, MM_TEXT);
4854

4955
IVwGraphicsWin32Ptr qvg;
@@ -79,8 +85,10 @@ namespace TestViews
7985
CheckHr(qseg->get_Width(0, qvg, &dxWidth));
8086

8187
qvg.Clear();
88+
::SelectObject(hdc, hbmOld);
8289
::DeleteObject(hbm);
8390
::DeleteDC(hdc);
91+
::ReleaseDC(hwndDesktop, hdcDesktop);
8492
#endif
8593
return dxWidth;
8694
}

‎Src/views/lib/UniscribeSegment.cpp‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ Last reviewed: Not yet.
1717
#pragma hdrstop
1818
// any other headers (not precompiled)
1919
#include "LayoutCache.h"
20+
#include <limits.h>
2021

2122
#undef THIS_FILE
2223
DEFINE_THIS_FILE
@@ -131,8 +132,16 @@ static bool TryParseFontFeatureRecords(const OLECHAR * prgchFontVar,
131132
bool fHaveDigit = false;
132133
while (*pch >= L'0' && *pch <= L'9')
133134
{
135+
int digit = *pch - L'0';
136+
if (value > (LONG_MAX - digit) / 10)
137+
{
138+
fHaveDigit = false;
139+
while (*pch >= L'0' && *pch <= L'9')
140+
++pch;
141+
break;
142+
}
134143
fHaveDigit = true;
135-
value = value * 10 + (*pch - L'0');
144+
value = value * 10 + digit;
136145
++pch;
137146
}
138147

@@ -219,7 +228,11 @@ static bool ShapePlaceRunWithOpenType(UniscribeRunInfo & uri, int cglyphMax,
219228
uri.prgGlyph, vglyphProps.Begin(), uri.cglyph, uri.prgAdvance, uri.prgoff, &abc));
220229
}
221230
if (SUCCEEDED(hr))
231+
{
232+
for (int iglyph = 0; iglyph < uri.cglyph; ++iglyph)
233+
uri.prgsva[iglyph] = vglyphProps[iglyph].sva;
222234
break;
235+
}
223236
}
224237
uri.fScriptPlaceFailed = FAILED(hr);
225238
return SUCCEEDED(hr);

0 commit comments

Comments
 (0)