[PATCH 3/3] gdi32: Fix text metrics in synthetic bold bitmap fonts. (rebased)

[Akihiro Sagawa]

Fixes Bug 44473.

Signed-off-by: Akihiro Sagawa <sagawa.aki(a)gmail.com>
---
 dlls/gdi32/freetype.c   | 9 +++++++--
 dlls/gdi32/tests/font.c | 3 ---
 2 files changed, 7 insertions(+), 5 deletions(-)

[Huw Davies]

On Sun, Feb 11, 2018 at 10:27:56PM +0900, Akihiro Sagawa wrote:
> Fixes Bug 44473.
>
> Signed-off-by: Akihiro Sagawa <sagawa.aki(a)gmail.com>
> ---
>  dlls/gdi32/freetype.c   | 9 +++++++--
>  dlls/gdi32/tests/font.c | 3 ---
>  2 files changed, 7 insertions(+), 5 deletions(-)

> diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
> index ba8855a..5d53129 100644
> --- a/dlls/gdi32/freetype.c
> +++ b/dlls/gdi32/freetype.c
> @@ -7523,6 +7523,9 @@ static BOOL get_bitmap_text_metrics(GdiFont *font)
>          TM.tmPitchAndFamily = FT_IS_FIXED_WIDTH(ft_face) ? 0 : TMPF_FIXED_PITCH;
>          TM.tmCharSet = font->charset;
>      }
> +
> +    if(font->fake_bold)
> +        TM.tmWeight = FW_BOLD;
>  #undef TM
>
> return TRUE;
> @@ -7552,10 +7555,12 @@ static void scale_font_metrics(const GdiFont *font, LPTEXTMETRICW ptm)
>      SCALE_Y(ptm->tmDescent);
>      SCALE_Y(ptm->tmInternalLeading);
>      SCALE_Y(ptm->tmExternalLeading);
> -    SCALE_Y(ptm->tmOverhang);
>
> -    if(FT_IS_SCALABLE(font->ft_face) && font->fake_bold)
> +    SCALE_X(ptm->tmOverhang);
> +    if(font->fake_bold)
>      {
> +        if(!FT_IS_SCALABLE(font->ft_face))
> +            ptm->tmOverhang += fabs(font->font_desc.matrix.eM11);
>          ptm->tmAveCharWidth++;
>          ptm->tmMaxCharWidth++;
>      }

It looks odd to me that tmOverHang gets incremented by essentially the
scaling factor, yet the widths only get incremented by one.  Shouldn't
these all be consistent?

Huw.

[Akihiro Sagawa]

On Mon, 12 Feb 2018 09:27:06 +0000, Huw Davies wrote:
> On Sun, Feb 11, 2018 at 10:27:56PM +0900, Akihiro Sagawa wrote:
> > @@ -7552,10 +7555,12 @@ static void scale_font_metrics(const GdiFont *font, LPTEXTMETRICW ptm)
> >      SCALE_Y(ptm->tmDescent);
> >      SCALE_Y(ptm->tmInternalLeading);
> >      SCALE_Y(ptm->tmExternalLeading);
> > -    SCALE_Y(ptm->tmOverhang);
> >
> > -    if(FT_IS_SCALABLE(font->ft_face) && font->fake_bold)
> > +    SCALE_X(ptm->tmOverhang);
> > +    if(font->fake_bold)
> >      {
> > +        if(!FT_IS_SCALABLE(font->ft_face))
> > +            ptm->tmOverhang += fabs(font->font_desc.matrix.eM11);
> >          ptm->tmAveCharWidth++;
> >          ptm->tmMaxCharWidth++;
> >      }
>
> It looks odd to me that tmOverHang gets incremented by essentially the
> scaling factor, yet the widths only get incremented by one.  Shouldn't
> these all be consistent?

Indeed, this looks odd. However, following my test shows tmOverhang is
incremented only 1 (in MM_TEXT mode) neverthless scaling factor.
| +        ok(data[i+1].tm.tmOverhang == 1,
| +           "expected 1, got %d\n", data[i+1].tm.tmOverhang);
If tmOverhang would be affected by scalling factor, this test wouldn't
pass when data[3].tm.tmOverhang is evaluated.

Moreover, my manual test shows if height == 200, the tmOverhang value is
still only 1 with Courier font.

Thanks reviewing,
Akihiro Sagawa

[Huw Davies]

On Tue, Feb 13, 2018 at 10:40:14PM +0900, Akihiro Sagawa wrote:
> On Mon, 12 Feb 2018 09:27:06 +0000, Huw Davies wrote:
> > On Sun, Feb 11, 2018 at 10:27:56PM +0900, Akihiro Sagawa wrote:
> > > @@ -7552,10 +7555,12 @@ static void scale_font_metrics(const GdiFont *font, LPTEXTMETRICW ptm)
> > >      SCALE_Y(ptm->tmDescent);
> > >      SCALE_Y(ptm->tmInternalLeading);
> > >      SCALE_Y(ptm->tmExternalLeading);
> > > -    SCALE_Y(ptm->tmOverhang);
> > >
> > > -    if(FT_IS_SCALABLE(font->ft_face) && font->fake_bold)
> > > +    SCALE_X(ptm->tmOverhang);
> > > +    if(font->fake_bold)
> > >      {
> > > +        if(!FT_IS_SCALABLE(font->ft_face))
> > > +            ptm->tmOverhang += fabs(font->font_desc.matrix.eM11);
> > >          ptm->tmAveCharWidth++;
> > >          ptm->tmMaxCharWidth++;
> > >      }
> >
> > It looks odd to me that tmOverHang gets incremented by essentially the
> > scaling factor, yet the widths only get incremented by one.  Shouldn't
> > these all be consistent?
>
> Indeed, this looks odd. However, following my test shows tmOverhang is
> incremented only 1 (in MM_TEXT mode) neverthless scaling factor.
> | +        ok(data[i+1].tm.tmOverhang == 1,
> | +           "expected 1, got %d\n", data[i+1].tm.tmOverhang);
> If tmOverhang would be affected by scalling factor, this test wouldn't
> pass when data[3].tm.tmOverhang is evaluated.
>
> Moreover, my manual test shows if height == 200, the tmOverhang value is
> still only 1 with Courier font.

If I change:
    ptm->tmOverhang += fabs(font->font_desc.matrix.eM11);
to:
    ptm->tmOverhang++;

the tests still pass.

Huw.

[Akihiro Sagawa]

On Tue, 13 Feb 2018 16:25:22 +0000, Huw Davies wrote:
> > > It looks odd to me that tmOverHang gets incremented by essentially the
> > > scaling factor, yet the widths only get incremented by one.  Shouldn't
> > > these all be consistent?
> >
> > Indeed, this looks odd. However, following my test shows tmOverhang is
> > incremented only 1 (in MM_TEXT mode) neverthless scaling factor.
> > | +        ok(data[i+1].tm.tmOverhang == 1,
> > | +           "expected 1, got %d\n", data[i+1].tm.tmOverhang);
> > If tmOverhang would be affected by scalling factor, this test wouldn't
> > pass when data[3].tm.tmOverhang is evaluated.
> >
> > Moreover, my manual test shows if height == 200, the tmOverhang value is
> > still only 1 with Courier font.
>
> If I change:
>     ptm->tmOverhang += fabs(font->font_desc.matrix.eM11);
> to:
>     ptm->tmOverhang++;
>
> the tests still pass.

I see. I'll add more tests with another mapping mode to prove consistent.

Thanks pointing out that,
Akihiro Sagawa