[PATCH v3 3/5] d3dx9: Handle newlines in ID3DXFont_DrawText.

[Matteo Bruni]

On Tue, Mar 3, 2020 at 9:55 AM Sven Baars <sbaars at codeweavers.com> wrote:
>
> Signed-off-by: Sven Baars <sbaars at codeweavers.com>
> ---
>  dlls/d3dx9_36/font.c       | 109 ++++++++++++++++++++++++++-----------
>  dlls/d3dx9_36/tests/core.c |  14 ++---
>  2 files changed, 83 insertions(+), 40 deletions(-)

I like how you split the large DrawText() patch into smaller, self
contained pieces, much appreciated!

> diff --git a/dlls/d3dx9_36/font.c b/dlls/d3dx9_36/font.c
> index 8ca35b8c21..23e6e5c401 100644
> --- a/dlls/d3dx9_36/font.c
> +++ b/dlls/d3dx9_36/font.c
> @@ -509,14 +509,49 @@ static INT WINAPI ID3DXFontImpl_DrawTextA(ID3DXFont *iface, ID3DXSprite *sprite,
>      return ret;
>  }
>
> +#define LF 10
> +#define CR 13

It seems nicer to me to use the usual character constants '\n' and '\r' instead.

> +static const WCHAR *TEXT_NextLineW(HDC hdc, const WCHAR *str, int *count,
> +                                   WCHAR *dest, int *len, int width, SIZE *retsize)

A few style nitpicks: line continuations should be 8 white spaces, the
int variables should be unsigned int where it makes sense. Also the
function could probably use a name not resembling the Win32 API as
much, e.g. get_next_line().
BTW you can use ~100 columns per line.

count and len can be a bit confusing, maybe qualify them like
"dst_size". Another thing to consider is splitting the (input) size of
the "dest" string and the (output) length of the line into two
separate arguments.

> +{
> +    int max_len = *len;
> +    int i = 0, j = 0;
> +
> +    while (*count && str[i] != LF)
> +    {
> +        --(*count);
> +        if (j < max_len && str[i] != CR)
> +            dest[j++] = str[i];
> +        ++i;
> +    }

This will silently cut lines longer than max_len. Maybe print a WARN()
when that happens?

> +
> +    GetTextExtentExPointW(hdc, dest, j, width, NULL, NULL, retsize);
> +
> +    if (*count && str[i] == LF)
> +    {
> +        --(*count);
> +        ++i;
> +    }
> +
> +    *len = j;
> +    if (*count)
> +        return str + i;
> +    else
> +        return NULL;

The else is unnecessary here.

> +}
> +
> +#define MAX_BUFFER 1024
>  static INT WINAPI ID3DXFontImpl_DrawTextW(ID3DXFont *iface, ID3DXSprite *sprite,
>          const WCHAR *string, INT count, RECT *rect, DWORD format, D3DCOLOR color)
>  {
>      struct d3dx_font *font = impl_from_ID3DXFont(iface);
>      ID3DXSprite *target = sprite;
> +    const WCHAR *strPtr = string;

Let's avoid camelCase variable names. In this case I think you can
just update "string" along the way, no need for a separate variable.

> +    WCHAR line[MAX_BUFFER];
>      RECT textrect = {0};
> -    int lh, x, y;
> +    int lh, x, y, width;
>      int ret = 0;
> +    SIZE size;

size is effectively unused at the moment. Unless it becomes useful
afterwards, it should probably just be dropped from the helper
function arguments.