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.