[PATCH 1/6] dwrite/tests: Do not segfault in tests if NULL is returned.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/tests/layout.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index 43a00b7e22e..d186f47cc94 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4714,6 +4714,7 @@ if (font) { ok(scale == 1.0f, "got %f\n", scale); ok(font != NULL, "got %p\n", font); +if (font) { exists = FALSE; hr = IDWriteFont_GetInformationalStrings(font, DWRITE_INFORMATIONAL_STRING_WIN32_FAMILY_NAMES, &strings, &exists); ok(hr == S_OK && exists, "got 0x%08x, exists %d\n", hr, exists); @@ -4722,6 +4723,7 @@ if (font) { ok(!lstrcmpW(buffW, L"Tahoma"), "Unexpected string %s.\n", wine_dbgstr_w(buffW)); IDWriteLocalizedStrings_Release(strings); IDWriteFont_Release(font); +} /* 2. Hiragana character, force Tahoma font does not support Japanese */ g_source = str2W; @@ -4735,6 +4737,7 @@ if (font) { ok(scale == 1.0f, "got %f\n", scale); ok(font != NULL, "got %p\n", font); +if (font) { exists = FALSE; hr = IDWriteFont_GetInformationalStrings(font, DWRITE_INFORMATIONAL_STRING_WIN32_FAMILY_NAMES, &strings, &exists); ok(hr == S_OK && exists, "got 0x%08x, exists %d\n", hr, exists); @@ -4744,6 +4747,7 @@ todo_wine ok(lstrcmpW(buffW, L"Tahoma"), "Unexpected string %s.\n", wine_dbgstr_w(buffW)); IDWriteLocalizedStrings_Release(strings); IDWriteFont_Release(font); +} IDWriteFontFallback_Release(fallback); IDWriteFactory2_Release(factory2); -- 2.30.1
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/tests/layout.c | 41 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index d186f47cc94..b0930e9344a 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4501,6 +4501,7 @@ static void test_SetWordWrapping(void) /* Collection dedicated to fallback testing */ static const WCHAR g_blahfontW[] = {'B','l','a','h',0}; +static const WCHAR g_fontNotInCollectionW[] = {'n','o','t','B','l','a','h',0}; static HRESULT WINAPI fontcollection_QI(IDWriteFontCollection *iface, REFIID riid, void **obj) { if (IsEqualIID(riid, &IID_IDWriteFontCollection) || IsEqualIID(riid, &IID_IUnknown)) { @@ -4562,6 +4563,9 @@ static HRESULT WINAPI fontcollection_FindFamilyName(IDWriteFontCollection *iface *index = 123456; *exists = TRUE; return S_OK; + } else if (!lstrcmpW(name, g_fontNotInCollectionW)) { + *exists = FALSE; + return S_OK; } ok(0, "unexpected call, name %s\n", wine_dbgstr_w(name)); return E_NOTIMPL; @@ -4749,6 +4753,43 @@ todo_wine IDWriteFont_Release(font); } + /* Explicit collection, but forcing a font that does not exist in it. */ + /* 1. Latin part */ + g_source = str2W; + mappedlength = 0; + scale = 0.0f; + font = NULL; + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 3, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, + DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); +todo_wine { + ok(hr == S_OK, "got 0x%08x\n", hr); + ok(mappedlength == 1, "got %u\n", mappedlength); +} + ok(scale == 1.0f, "got %f\n", scale); +todo_wine + ok(font != NULL, "got %p\n", font); +if (font) { + IDWriteFont_Release(font); +} + + /* 2. Hiragana character */ + g_source = str2W; + mappedlength = 0; + scale = 0.0f; + font = NULL; + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 1, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, + DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); +todo_wine { + ok(hr == S_OK, "got 0x%08x\n", hr); + ok(mappedlength == 1, "got %u\n", mappedlength); +} + ok(scale == 1.0f, "got %f\n", scale); +todo_wine + ok(font != NULL, "got %p\n", font); +if (font) { + IDWriteFont_Release(font); +} + IDWriteFontFallback_Release(fallback); IDWriteFactory2_Release(factory2); } -- 2.30.1
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/tests/layout.c | 53 +++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index b0930e9344a..1c7cdad006b 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4591,7 +4591,7 @@ static IDWriteFontCollection fallbackcollection = { &fallbackcollectionvtbl }; static void test_MapCharacters(void) { - static const WCHAR str2W[] = {'a',0x3058,'b',0}; + static const WCHAR str2W[] = {'a',0x3058,0x3059,'b',0}; IDWriteLocalizedStrings *strings; IDWriteFontFallback *fallback; IDWriteFactory2 *factory2; @@ -4672,12 +4672,12 @@ todo_wine if (font) { IDWriteFont_Release(font); } - /* string 'a\x3058b' */ + /* string 'a\x3058\x3059b' */ g_source = str2W; mappedlength = 0; scale = 0.0f; font = NULL; - hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 3, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 4, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); @@ -4697,7 +4697,7 @@ if (font) { DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); - ok(mappedlength == 1, "got %u\n", mappedlength); + ok(mappedlength == 2, "got %u\n", mappedlength); } ok(scale == 1.0f, "got %f\n", scale); todo_wine @@ -4711,7 +4711,7 @@ if (font) { mappedlength = 0; scale = 0.0f; font = NULL; - hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 3, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 4, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 1, "got %u\n", mappedlength); @@ -4741,6 +4741,31 @@ if (font) { ok(scale == 1.0f, "got %f\n", scale); ok(font != NULL, "got %p\n", font); +if (font) { + exists = FALSE; + hr = IDWriteFont_GetInformationalStrings(font, DWRITE_INFORMATIONAL_STRING_WIN32_FAMILY_NAMES, &strings, &exists); + ok(hr == S_OK && exists, "got 0x%08x, exists %d\n", hr, exists); + hr = IDWriteLocalizedStrings_GetString(strings, 0, buffW, ARRAY_SIZE(buffW)); + ok(hr == S_OK, "got 0x%08x\n", hr); +todo_wine + ok(lstrcmpW(buffW, L"Tahoma"), "Unexpected string %s.\n", wine_dbgstr_w(buffW)); + IDWriteLocalizedStrings_Release(strings); + IDWriteFont_Release(font); +} + + /* 3. Hiragana character and Latin character, force Tahoma font does not support Japanese */ + g_source = str2W; + mappedlength = 0; + scale = 0.0f; + font = NULL; + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 3, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, + DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); + ok(hr == S_OK, "got 0x%08x\n", hr); +todo_wine + ok(mappedlength == 2, "got %u\n", mappedlength); + ok(scale == 1.0f, "got %f\n", scale); + ok(font != NULL, "got %p\n", font); + if (font) { exists = FALSE; hr = IDWriteFont_GetInformationalStrings(font, DWRITE_INFORMATIONAL_STRING_WIN32_FAMILY_NAMES, &strings, &exists); @@ -4759,7 +4784,7 @@ todo_wine mappedlength = 0; scale = 0.0f; font = NULL; - hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 3, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 4, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); @@ -4790,6 +4815,22 @@ if (font) { IDWriteFont_Release(font); } + /* 3. Hiragana character and Latin character */ + g_source = str2W; + mappedlength = 0; + scale = 0.0f; + font = NULL; + hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 3, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, + DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); + ok(hr == S_OK, "got 0x%08x\n", hr); +todo_wine + ok(mappedlength == 2, "got %u\n", mappedlength); + ok(scale == 1.0f, "got %f\n", scale); + ok(font != NULL, "got %p\n", font); +if (font) { + IDWriteFont_Release(font); +} + IDWriteFontFallback_Release(fallback); IDWriteFactory2_Release(factory2); } -- 2.30.1
Add two fonts which are easily found on modern Linux installations. It might be wise to add others in the future, so that any random Linux installation has at least one of them. Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/analyzer.c | 4 +++- dlls/dwrite/tests/layout.c | 7 ------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/dlls/dwrite/analyzer.c b/dlls/dwrite/analyzer.c index 6b74a23540a..e4bc8d42943 100644 --- a/dlls/dwrite/analyzer.c +++ b/dlls/dwrite/analyzer.c @@ -208,8 +208,10 @@ const char *debugstr_sa_script(UINT16 script) /* system font falback configuration */ static const WCHAR meiryoW[] = {'M','e','i','r','y','o',0}; +static const WCHAR droidW[] = {'D','r','o','i','d',' ','S','a','n','s',' ','F','a','l','l','b','a','c','k',0}; +static const WCHAR notoW[] = {'N','o','t','o',' ','S','e','r','i','f',' ','C','J','K',' ','S','C',0}; -static const WCHAR *cjk_families[] = { meiryoW }; +static const WCHAR *cjk_families[] = { meiryoW, droidW, notoW }; static const DWRITE_UNICODE_RANGE cjk_ranges[] = { diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index 1c7cdad006b..9feb9e224fc 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4695,12 +4695,9 @@ if (font) { font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 2, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 2, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); @@ -4747,7 +4744,6 @@ if (font) { ok(hr == S_OK && exists, "got 0x%08x, exists %d\n", hr, exists); hr = IDWriteLocalizedStrings_GetString(strings, 0, buffW, ARRAY_SIZE(buffW)); ok(hr == S_OK, "got 0x%08x\n", hr); -todo_wine ok(lstrcmpW(buffW, L"Tahoma"), "Unexpected string %s.\n", wine_dbgstr_w(buffW)); IDWriteLocalizedStrings_Release(strings); IDWriteFont_Release(font); @@ -4761,7 +4757,6 @@ todo_wine hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 3, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); ok(hr == S_OK, "got 0x%08x\n", hr); -todo_wine ok(mappedlength == 2, "got %u\n", mappedlength); ok(scale == 1.0f, "got %f\n", scale); ok(font != NULL, "got %p\n", font); @@ -4772,7 +4767,6 @@ if (font) { ok(hr == S_OK && exists, "got 0x%08x, exists %d\n", hr, exists); hr = IDWriteLocalizedStrings_GetString(strings, 0, buffW, ARRAY_SIZE(buffW)); ok(hr == S_OK, "got 0x%08x\n", hr); -todo_wine ok(lstrcmpW(buffW, L"Tahoma"), "Unexpected string %s.\n", wine_dbgstr_w(buffW)); IDWriteLocalizedStrings_Release(strings); IDWriteFont_Release(font); @@ -4823,7 +4817,6 @@ if (font) { hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 3, &fallbackcollection, g_blahfontW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); ok(hr == S_OK, "got 0x%08x\n", hr); -todo_wine ok(mappedlength == 2, "got %u\n", mappedlength); ok(scale == 1.0f, "got %f\n", scale); ok(font != NULL, "got %p\n", font); -- 2.30.1
On 3/8/21 12:31 PM, Giovanni Mascellani wrote:
Add two fonts which are easily found on modern Linux installations. It might be wise to add others in the future, so that any random Linux installation has at least one of them. Have you explored possibility of using fontconfig API to fill such lists at runtime? I don't really know how GTK/Qt handles that, maybe something hard coded is still valuable, but it seems we should at least try to respect user configuration. Same for patch 5/6.
Hi, Il 11/03/21 21:57, Nikolay Sivov ha scritto:
On 3/8/21 12:31 PM, Giovanni Mascellani wrote: >> Add two fonts which are easily found on modern Linux installations.
It might be wise to add others in the future, so that any random >> Linux installation has at least one of them. > Have you explored possibility of using fontconfig API to fill such > lists at runtime? I don't really know how GTK/Qt handles that, maybe > something hard coded is still valuable, but it seems we should at > least try to respect user configuration. Same for patch 5/6. I haven't explored it. My patch (and patch set) was meant to be a quick fix to make a very broken setting a little less broken; introducing fontconfig, while probably being a more proper solution, would require much more work. I think it is better to get there more gradually, including this patch set as it is now (which already fixes some games, like Lumberjack's Dynasty) and leaving more complex stuff for the future.
Thanks, Giovanni.
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/analyzer.c | 16 ++++++++++++++++ dlls/dwrite/tests/layout.c | 9 --------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/dlls/dwrite/analyzer.c b/dlls/dwrite/analyzer.c index e4bc8d42943..daa553adbaf 100644 --- a/dlls/dwrite/analyzer.c +++ b/dlls/dwrite/analyzer.c @@ -220,6 +220,21 @@ static const DWRITE_UNICODE_RANGE cjk_ranges[] = { 0x4e00, 0x9fff }, /* CJK Unified Ideographs */ }; +static const WCHAR timesW[] = {'T','i','m','e','s',' ','N','e','w',' ','R','o','m','a','n',0}; +static const WCHAR liberationW[] = {'L','i','b','e','r','a','t','i','o','n',' ','S','e','r','i','f',0}; +static const WCHAR dejavuW[] = {'D','e','j','a','V','u',' ','S','e','r','i','f',0}; + +static const WCHAR *latin_families[] = { timesW, liberationW, dejavuW }; + +static const DWRITE_UNICODE_RANGE latin_ranges[] = +{ + { 0x0000, 0x05ff }, + { 0x1d00, 0x2eff }, + { 0xa700, 0xa7ff }, + { 0xfb00, 0xfb4f }, + { 0xfe20, 0xfe23 }, +}; + struct fallback_mapping { DWRITE_UNICODE_RANGE *ranges; UINT32 ranges_count; @@ -236,6 +251,7 @@ static const struct fallback_mapping fontfallback_neutral_data[] = { (WCHAR **)families, ARRAY_SIZE(families) } MAPPING_RANGE(cjk_ranges, cjk_families), + MAPPING_RANGE(latin_ranges, latin_families), #undef MAPPING_RANGE }; diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index 9feb9e224fc..8574af5465f 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4645,12 +4645,9 @@ static void test_MapCharacters(void) font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 1, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 1, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); @@ -4662,12 +4659,9 @@ if (font) { font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 3, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 3, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); @@ -4679,12 +4673,9 @@ if (font) { font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 4, NULL, NULL, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 1, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); -- 2.30.1
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dwrite/analyzer.c | 51 ++++++++++++++++++++------------------ dlls/dwrite/tests/layout.c | 10 -------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/dlls/dwrite/analyzer.c b/dlls/dwrite/analyzer.c index daa553adbaf..c811caf0a66 100644 --- a/dlls/dwrite/analyzer.c +++ b/dlls/dwrite/analyzer.c @@ -2066,7 +2066,7 @@ static HRESULT fallback_map_characters(IDWriteFont *font, const WCHAR *text, UIN /* stop on first unsupported character */ exists = FALSE; hr = IDWriteFont_HasCharacter(font, text[i], &exists); - if (hr == S_OK && exists) + if (SUCCEEDED(hr) && exists) ++*mapped_length; else break; @@ -2084,11 +2084,12 @@ static HRESULT fallback_get_fallback_font(struct dwrite_fontfallback *fallback, UINT32 i; *mapped_font = NULL; + *mapped_length = 0; mapping = find_fallback_mapping(fallback, text[0]); if (!mapping) { WARN("No mapping range for %#x.\n", text[0]); - return E_FAIL; + return S_OK; } /* Now let's see what fallback can handle. Pick first font that could be created. */ @@ -2103,19 +2104,18 @@ static HRESULT fallback_get_fallback_font(struct dwrite_fontfallback *fallback, if (!*mapped_font) { WARN("Failed to create fallback font.\n"); - return E_FAIL; + return S_OK; } hr = fallback_map_characters(*mapped_font, text, length, mapped_length); - if (FAILED(hr)) - WARN("Mapping with fallback family %s failed, hr %#x.\n", debugstr_w(mapping->families[i]), hr); if (!*mapped_length) { + WARN("Mapping with fallback family %s failed.\n", debugstr_w(mapping->families[i])); IDWriteFont_Release(*mapped_font); *mapped_font = NULL; } - return *mapped_length ? S_OK : E_FAIL; + return hr; } static HRESULT WINAPI fontfallback_MapCharacters(IDWriteFontFallback1 *iface, IDWriteTextAnalysisSource *source, @@ -2150,30 +2150,33 @@ static HRESULT WINAPI fontfallback_MapCharacters(IDWriteFontFallback1 *iface, ID if (basefamily && *basefamily) { hr = create_matching_font(basecollection, basefamily, weight, style, stretch, ret_font); - if (FAILED(hr)) - goto done; - hr = fallback_map_characters(*ret_font, text, length, mapped_length); + /* It is not a fatal error for create_matching_font to + fail. We still have other fallbacks to try. */ + + if (SUCCEEDED(hr)) { + hr = fallback_map_characters(*ret_font, text, length, mapped_length); + if (FAILED(hr)) + goto done; + } + } + + if (!*mapped_length) { + if (*ret_font) { + IDWriteFont_Release(*ret_font); + *ret_font = NULL; + } + + hr = fallback_get_fallback_font(fallback, text, length, weight, style, stretch, mapped_length, ret_font); if (FAILED(hr)) goto done; } if (!*mapped_length) { - IDWriteFont *mapped_font; - - hr = fallback_get_fallback_font(fallback, text, length, weight, style, stretch, mapped_length, &mapped_font); - if (FAILED(hr)) { - /* fallback wasn't found, keep base font if any, so we can get at least some visual output */ - if (*ret_font) { - *mapped_length = length; - hr = S_OK; - } - } - else { - if (*ret_font) - IDWriteFont_Release(*ret_font); - *ret_font = mapped_font; - } + /* fallback wasn't found, ask the caller to skip one character + and try again; FIXME: skip the appropriate number of + characters instead of just one */ + *mapped_length = 1; } done: diff --git a/dlls/dwrite/tests/layout.c b/dlls/dwrite/tests/layout.c index 8574af5465f..5a04b19b77c 100644 --- a/dlls/dwrite/tests/layout.c +++ b/dlls/dwrite/tests/layout.c @@ -4771,12 +4771,9 @@ if (font) { font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 0, 4, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 1, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); @@ -4789,12 +4786,9 @@ if (font) { font = NULL; hr = IDWriteFontFallback_MapCharacters(fallback, &analysissource, 1, 1, &fallbackcollection, g_fontNotInCollectionW, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_STRETCH_NORMAL, &mappedlength, &font, &scale); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(mappedlength == 1, "got %u\n", mappedlength); -} ok(scale == 1.0f, "got %f\n", scale); -todo_wine ok(font != NULL, "got %p\n", font); if (font) { IDWriteFont_Release(font); @@ -5053,21 +5047,17 @@ static void test_fallback(void) count = 0; hr = IDWriteTextLayout_GetClusterMetrics(layout, clusters, 4, &count); -todo_wine { ok(hr == S_OK, "Failed to get cluster metrics, hr %#x.\n", hr); ok(count == 4, "Unexpected count %u.\n", count); -} for (i = 0, width = 0.0; i < count; i++) width += clusters[i].width; memset(&metrics, 0xcc, sizeof(metrics)); hr = IDWriteTextLayout_GetMetrics(layout, &metrics); ok(hr == S_OK, "Failed to get layout metrics, hr %#x.\n", hr); -todo_wine { ok(metrics.width > 0.0 && metrics.width == width, "Unexpected width %.2f, expected %.2f.\n", metrics.width, width); ok(metrics.height > 0.0, "Unexpected height %.2f.\n", metrics.height); ok(metrics.lineCount == 1, "Unexpected line count %u.\n", metrics.lineCount); -} IDWriteTextLayout_Release(layout); IDWriteTextFormat_Release(format); -- 2.30.1
I don't see it crashing now anywhere on test vms. If that becomes an issue later on after implementation changes, this test diff should be included there.
Hi, thanks for the review! Il 11/03/21 21:55, Nikolay Sivov ha scritto:
I don't see it crashing now anywhere on test vms. If that becomes an issue later on after implementation changes, this test diff should be included there.
It used to crash on my machine, because of a different set of installed fonts. I don't think the tests should be written just for the test VMs: any machine should reasonably be able to run them correctly and without crashes. The reason for this patch is that in C you should never dereference a NULL pointer, so the moment you get a pointer that might be NULL, you should test it before using it. If you want practical reasons, consider the case of a developer who wants to run the tests and has to waste their time chasing a segmentation fault caused by them having a different (but entirely legitimate) font set installed on their machine. In other words, this patch makes the program more correct and gives less headaches to anybody using it, so why is it a problem? Giovanni.
On 3/12/21 10:26 AM, Giovanni Mascellani wrote:
Hi,
thanks for the review!
Il 11/03/21 21:55, Nikolay Sivov ha scritto:
I don't see it crashing now anywhere on test vms. If that becomes an issue later on after implementation changes, this test diff should be included there.
It used to crash on my machine, because of a different set of installed fonts. I don't think the tests should be written just for the test VMs: any machine should reasonably be able to run them correctly and without crashes. The reason for this patch is that in C you should never dereference a NULL pointer, so the moment you get a pointer that might be NULL, you should test it before using it. If you want practical reasons, consider the case of a developer who wants to run the tests and has to waste their time chasing a segmentation fault caused by them having a different (but entirely legitimate) font set installed on their machine.
As far as I can tell this test is meant to show that explicit collection/font specified with MapCharacters() is actually used. It will only test if 'a' is supported in Tahoma, which should always be the case with wine's or native Tahoma. So unless you have a custom font named Tahoma without basic Latin support, which takes precedence over wine font, I don't understand why it fails for you. Could you explain? In ideal test environment we would be using a set of specifically created fonts of course, so we don't depend on system fonts for tests where it's not desirable, but that might be a better fit for externally maintained test programs.
In other words, this patch makes the program more correct and gives less headaches to anybody using it, so why is it a problem?
It's not a problem, but I want to understand what's going on before suppressing failures or crashes.
Giovanni.
Hi, Il 12/03/21 09:51, Nikolay Sivov ha scritto:
As far as I can tell this test is meant to show that explicit collection/font specified with MapCharacters() is actually used. It will only test if 'a' is supported in Tahoma, which should always be the case with wine's or native Tahoma. So unless you have a custom font named Tahoma without basic Latin support, which takes precedence over wine font, I don't understand why it fails for you. Could you explain?
Hm, I cannot reproduce the crash any more, so maybe it was related to the other patches that I later discarded. Your explanation makes sense, so I guess I should just withdraw this patch. Giovanni.
participants (2)
-
Giovanni Mascellani -
Nikolay Sivov