Re: [PATCH v3 1/4] dwrite/tests: Add some tests for Segoe UI font family.

[Nikolay Sivov]

> -
>
> +static void test_SegoeUI(void)
> +{
> - HRESULT hr;
> - IDWriteFactory2 *factory;
> - IDWriteFontCollection *collection;
> - IDWriteFontFamily *family;
> - IDWriteFont *font;
> - UINT32 index, count, i;
> - WCHAR name[256];
> - BOOL exists, found;
> -
> - hr = DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED,
>
> &IID_IDWriteFactory2, (IUnknown **)&factory);
> - if (hr != S_OK)
> - {
> -
>    win_skip("IDWriteFactory2 is not supported\n");
> -
>    return;
> - }
> -
> - hr = IDWriteFactory2_GetSystemFontCollection(factory, &collection,
>
> FALSE);
> - ok(hr == S_OK, "got %#x\n", hr);
> -
> - hr = IDWriteFontCollection_FindFamilyName(collection, L"Segoe UI",
>
> &index, &exists);
> - ok(hr == S_OK, "got %#x\n", hr);
> - if (!exists)
> - {
> -
>    skip("Segoe UI is not installed\n");
> -
>    IDWriteFontCollection_Release(collection);
> -
>    IDWriteFactory2_Release(factory);
> -
>    return;
> - }
> - ok(index != UINT_MAX && exists, "Segoe UI was not found\n");
> -
> - hr = IDWriteFontCollection_GetFontFamily(collection, index, &family);
> - ok(hr == S_OK, "got %#x\n", hr);
> -
> - count = IDWriteFontFamily_GetFontCount(family);
> - trace("family Segoe UI has %u fonts\n", count);
> -
> - found = FALSE;
> -
> - for (i = 0; i < count; i++)
> - {
> -
>    hr = IDWriteFontFamily_GetFont(family, i, &font);
> -
>    ok(hr == S_OK, "got %#x\n", hr);
> -
> -
>    get_font_name(font, name, ARRAY_SIZE(name));
> -
>    if (!wcscmp(name, L"Segoe UI Symbol"))
> -
>        found = TRUE;
> -
> -
>    hr = IDWriteFont_HasCharacter(font, 0x25d4, &exists);
> -
>    ok(hr == S_OK, "got %#x\n", hr);
> -
>    ok(!exists, "%u: %s has character 0x25d4\n", i,
>
> wine_dbgstr_w(name));
> -
> -
>    IDWriteFont_Release(font);
> - }
> -
> - ok(!found, "Segoe UI Symbol should not be part of Segoe UI family\n");
> -
> - IDWriteFontFamily_Release(family);
> - IDWriteFontCollection_Release(collection);
> - IDWriteFactory2_Release(factory);
>
> +}

All this does is checking if font is installed, and we know it is 
installed on all current Windows versions, it's not some directwrite 
functionality. Second patch will already trigger a todo case when font 
is installed in Wine.

[PATCH v3 1/4] dwrite/tests: Add some tests for Segoe UI font family.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > +static void test_SegoeUI(void)
> > +{
> > - HRESULT hr;
> > - IDWriteFactory2 *factory;
> > - IDWriteFontCollection *collection;
> > - IDWriteFontFamily *family;
> > - IDWriteFont *font;
> > - UINT32 index, count, i;
> > - WCHAR name[256];
> > - BOOL exists, found;
> > -
> > - hr = DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED,
> >
> > &IID_IDWriteFactory2, (IUnknown **)&factory);
> > - if (hr != S_OK)
> > - {
> > -
> >    win_skip("IDWriteFactory2 is not supported\n");
> > -
> >    return;
> > - }
> > -
> > - hr = IDWriteFactory2_GetSystemFontCollection(factory, &collection,
> >
> > FALSE);
> > - ok(hr == S_OK, "got %#x\n", hr);
> > -
> > - hr = IDWriteFontCollection_FindFamilyName(collection, L"Segoe UI",
> >
> > &index, &exists);
> > - ok(hr == S_OK, "got %#x\n", hr);
> > - if (!exists)
> > - {
> > -
> >    skip("Segoe UI is not installed\n");
> > -
> >    IDWriteFontCollection_Release(collection);
> > -
> >    IDWriteFactory2_Release(factory);
> > -
> >    return;
> > - }
> > - ok(index != UINT_MAX && exists, "Segoe UI was not found\n");
> > -
> > - hr = IDWriteFontCollection_GetFontFamily(collection, index, &family);
> > - ok(hr == S_OK, "got %#x\n", hr);
> > -
> > - count = IDWriteFontFamily_GetFontCount(family);
> > - trace("family Segoe UI has %u fonts\n", count);
> > -
> > - found = FALSE;
> > -
> > - for (i = 0; i < count; i++)
> > - {
> > -
> >    hr = IDWriteFontFamily_GetFont(family, i, &font);
> > -
> >    ok(hr == S_OK, "got %#x\n", hr);
> > -
> > -
> >    get_font_name(font, name, ARRAY_SIZE(name));
> > -
> >    if (!wcscmp(name, L"Segoe UI Symbol"))
> > -
> >        found = TRUE;
> > -
> > -
> >    hr = IDWriteFont_HasCharacter(font, 0x25d4, &exists);
> > -
> >    ok(hr == S_OK, "got %#x\n", hr);
> > -
> >    ok(!exists, "%u: %s has character 0x25d4\n", i,
> >
> > wine_dbgstr_w(name));
> > -
> > -
> >    IDWriteFont_Release(font);
> > - }
> > -
> > - ok(!found, "Segoe UI Symbol should not be part of Segoe UI family\n");
> > -
> > - IDWriteFontFamily_Release(family);
> > - IDWriteFontCollection_Release(collection);
> > - IDWriteFactory2_Release(factory);
> >
> > +}
>
> All this does is checking if font is installed, and we know it is 
> installed on all current Windows versions, it's not some directwrite 
> functionality. Second patch will already trigger a todo case when font 
> is installed in Wine.

Thanks for the review.

Intent of the test is to verify that "Segoe UI" font family doesn't include
"Segoe UI Symbol", i.e. it makes sure that the suffix "Symbol" is not part
of other ligitimate ones like "Regular", "Bold" or "Italic". Do you think
that's not relevant or essential for testing?

[PATCH v3 1/4] dwrite/tests: Add some tests for Segoe UI font family.

[Nikolay Sivov]

On 4/5/22 13:19, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > > +static void test_SegoeUI(void)
> > > +{
> > > - HRESULT hr;
> > > - IDWriteFactory2 *factory;
> > > - IDWriteFontCollection *collection;
> > > - IDWriteFontFamily *family;
> > > - IDWriteFont *font;
> > > - UINT32 index, count, i;
> > > - WCHAR name[256];
> > > - BOOL exists, found;
> > > -
> > > - hr = DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED,
> > >
> > > &IID_IDWriteFactory2, (IUnknown **)&factory);
> > > - if (hr != S_OK)
> > > - {
> > > -
> > >    win_skip("IDWriteFactory2 is not supported\n");
> > > -
> > >    return;
> > > - }
> > > -
> > > - hr = IDWriteFactory2_GetSystemFontCollection(factory, &collection,
> > >
> > > FALSE);
> > > - ok(hr == S_OK, "got %#x\n", hr);
> > > -
> > > - hr = IDWriteFontCollection_FindFamilyName(collection, L"Segoe UI",
> > >
> > > &index, &exists);
> > > - ok(hr == S_OK, "got %#x\n", hr);
> > > - if (!exists)
> > > - {
> > > -
> > >    skip("Segoe UI is not installed\n");
> > > -
> > >    IDWriteFontCollection_Release(collection);
> > > -
> > >    IDWriteFactory2_Release(factory);
> > > -
> > >    return;
> > > - }
> > > - ok(index != UINT_MAX && exists, "Segoe UI was not found\n");
> > > -
> > > - hr = IDWriteFontCollection_GetFontFamily(collection, index, &family);
> > > - ok(hr == S_OK, "got %#x\n", hr);
> > > -
> > > - count = IDWriteFontFamily_GetFontCount(family);
> > > - trace("family Segoe UI has %u fonts\n", count);
> > > -
> > > - found = FALSE;
> > > -
> > > - for (i = 0; i < count; i++)
> > > - {
> > > -
> > >    hr = IDWriteFontFamily_GetFont(family, i, &font);
> > > -
> > >    ok(hr == S_OK, "got %#x\n", hr);
> > > -
> > > -
> > >    get_font_name(font, name, ARRAY_SIZE(name));
> > > -
> > >    if (!wcscmp(name, L"Segoe UI Symbol"))
> > > -
> > >        found = TRUE;
> > > -
> > > -
> > >    hr = IDWriteFont_HasCharacter(font, 0x25d4, &exists);
> > > -
> > >    ok(hr == S_OK, "got %#x\n", hr);
> > > -
> > >    ok(!exists, "%u: %s has character 0x25d4\n", i,
> > >
> > > wine_dbgstr_w(name));
> > > -
> > > -
> > >    IDWriteFont_Release(font);
> > > - }
> > > -
> > > - ok(!found, "Segoe UI Symbol should not be part of Segoe UI family\n");
> > > -
> > > - IDWriteFontFamily_Release(family);
> > > - IDWriteFontCollection_Release(collection);
> > > - IDWriteFactory2_Release(factory);
> > >
> > > +}
> >
> > All this does is checking if font is installed, and we know it is
> > installed on all current Windows versions, it's not some directwrite
> > functionality. Second patch will already trigger a todo case when font
> > is installed in Wine.
>
> Thanks for the review.
>
> Intent of the test is to verify that "Segoe UI" font family doesn't include
> "Segoe UI Symbol", i.e. it makes sure that the suffix "Symbol" is not part
> of other ligitimate ones like "Regular", "Bold" or "Italic". Do you think
> that's not relevant or essential for testing?

The test for MapCharacters() in the second patch is using 
GetFamilyNames() on returned instance, that's enough to tell that such 
family exists.

For special suffixes and synthesized names, the lists we are using are 
exhaustive in terms of WPF font model. It's possible it changed since 
then of course, but to see the full picture it's necessary to dump all 
families groupings for default collection, and compare with what we have 
on same set of fonts.

[PATCH v3 1/4] dwrite/tests: Add some tests for Segoe UI font family.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > All this does is checking if font is installed, and we know it is
> > > installed on all current Windows versions, it's not some directwrite
> > > functionality. Second patch will already trigger a todo case when font
> > > is installed in Wine.
> >
> > Thanks for the review.
> >
> > Intent of the test is to verify that "Segoe UI" font family doesn't include
> > "Segoe UI Symbol", i.e. it makes sure that the suffix "Symbol" is not part
> > of other ligitimate ones like "Regular", "Bold" or "Italic". Do you think
> > that's not relevant or essential for testing?
>
> The test for MapCharacters() in the second patch is using 
> GetFamilyNames() on returned instance, that's enough to tell that such 
> family exists.
>
> For special suffixes and synthesized names, the lists we are using are 
> exhaustive in terms of WPF font model. It's possible it changed since 
> then of course, but to see the full picture it's necessary to dump all 
> families groupings for default collection, and compare with what we have 
> on same set of fonts.

Thanks, that makes sense. Basically, once I saw "Segoe UI"/"Segoe UI Symbol"
mixup in the logs of the application that I'm working on, I wanted to make
sure that "Symbol" suffix doesn't mean using some specific charmap in the font,
and somehow explain missing 0x25d4 glyph.

What do you think of remaining patches in the series? Should I rebase and
resend them once the signed off patch is committed?

[PATCH v3 1/4] dwrite/tests: Add some tests for Segoe UI font family.

[Nikolay Sivov]

On 4/5/22 16:27, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > > > All this does is checking if font is installed, and we know it is
> > > > installed on all current Windows versions, it's not some directwrite
> > > > functionality. Second patch will already trigger a todo case when font
> > > > is installed in Wine.
> > >
> > > Thanks for the review.
> > >
> > > Intent of the test is to verify that "Segoe UI" font family doesn't include
> > > "Segoe UI Symbol", i.e. it makes sure that the suffix "Symbol" is not part
> > > of other ligitimate ones like "Regular", "Bold" or "Italic". Do you think
> > > that's not relevant or essential for testing?
> >
> > The test for MapCharacters() in the second patch is using
> > GetFamilyNames() on returned instance, that's enough to tell that such
> > family exists.
> >
> > For special suffixes and synthesized names, the lists we are using are
> > exhaustive in terms of WPF font model. It's possible it changed since
> > then of course, but to see the full picture it's necessary to dump all
> > families groupings for default collection, and compare with what we have
> > on same set of fonts.
>
> Thanks, that makes sense. Basically, once I saw "Segoe UI"/"Segoe UI Symbol"
> mixup in the logs of the application that I'm working on, I wanted to make
> sure that "Symbol" suffix doesn't mean using some specific charmap in the font,
> and somehow explain missing 0x25d4 glyph.

I think for newer fonts that abandoned this idea, but MS symbol charmap 
is of course still supported.
This is reflected by IsSymbol() method.
> What do you think of remaining patches in the series? Should I rebase and
> resend them once the signed off patch is committed?

No need to rebase right now. I think main question is whether we want a 
new font or not, and it's not up to me to decide on that.