Re: [PATCH 1/1] riched20: allow setting background text color

[Huw Davies]

Hi Kenneth,

Thanks for your contribution to Wine.  Please see below for some
comments.

On Sat, Aug 06, 2016 at 02:17:34PM -0600, Kenneth Haley wrote:
> diff --git a/dlls/riched20/paint.c b/dlls/riched20/paint.c
> index 42434b9..df7af79 100644
> --- a/dlls/riched20/paint.c
> +++ b/dlls/riched20/paint.c
> @@ -318,11 +318,20 @@ static void get_selection_rect( ME_Context *c, ME_Run *run, int from, int to, in
>  static void draw_text( ME_Context *c, ME_Run *run, int x, int y, BOOL selected, RECT *sel_rect )
>  {
>      COLORREF text_color = get_text_color( c, run->style, selected );
> -    COLORREF back_color = selected ? ITextHost_TxGetSysColor( c->editor->texthost, COLOR_HIGHLIGHT ) : 0;
> +    COLORREF back_color = 0;
>      COLORREF old_text, old_back = 0;
>      const WCHAR *text = get_text( run, 0 );
>      ME_String *masked = NULL;
>
> +    if (selected)
> +      back_color = ITextHost_TxGetSysColor( c->editor->texthost, COLOR_HIGHLIGHT );
> +    else if(run->style->fmt.dwMask & CFM_BACKCOLOR)
> +      if (!(CFE_AUTOBACKCOLOR & run->style->fmt.dwEffects))
> +      {
> +          back_color = run->style->fmt.crBackColor;
> +          selected = TRUE;
> +      }
> +

You don't need two 'if's in the 'else' clause, just combine them with an &&.

I think we should introduce a new boolean 'paint_bkgnd' (or something)
rather than hijacking 'selected' here.  It would be set to TRUE if
'selected' or we have a background colour.  The code that follows this
would then use that rather than 'selected'.

Also, please use 4-space indents in this function and be consistant
about spaces after an 'if'.
> @@ -385,6 +394,18 @@ static void ME_DrawTextWithStyle(ME_Context *c, ME_Run *run, int x, int y,
>        }
>      }
>    }
> +  else if(run->style->fmt.dwMask & CFM_BACKCOLOR)

Again, space after 'if'.
> +  {
> +    get_selection_rect( c, run, 0, run->len, cy, &sel_rect );
> +    OffsetRect( &sel_rect, x, ymin );
> +    sel_rgn = CreateRectRgnIndirect( &sel_rect );
> +    clip = CreateRectRgn( 0, 0, 0, 0 );
> +    if (GetClipRgn( hDC, clip ) != 1)
> +    {
> +      DeleteObject( clip );
> +      clip = NULL;
> +    }
> +  }

This block is essentially a copy of the selected block.
It should be possible to rewrite the if condition in that
block so you don't need to duplicate this code.

Huw.