[PATCH 1/2] riched20: Stub for ITextFont interface and implement ITextRange::GetFont. (try 3)

[Nikolay Sivov]

> +typedef struct ITextFontImpl {
> +    ITextFont ITextFont_iface;
> +    LONG ref;
> +
> +    ITextRangeImpl *txtRge;
> +    ITextSelectionImpl *txtSel;
> +} ITextFontImpl;
> +
I think 'range' and 'selection' would be more readable, but what's more 
important you don't use 'txtSel' at all.

> +static HRESULT WINAPI ITextFont_fnQueryInterface(ITextFont *me, REFIID riid, void **ppvObj)
If we want some unification, it's usually called 'iface', not 'me'.

> +        *ppvObj = me;
That's not how it should look.

> +static IRichEditOleImpl *get_reOle(ITextFontImpl *txtFont)
> +{
> +  if (txtFont->txtRge)
> +      return txtFont->txtRge->reOle;
> +  else
> +      return txtFont->txtSel->reOle;
> +}
Please use a better naming, like 'get_reole_from_font()' or something 
like that.

> +    FIXME("not implemented: %p\n", This);
> +    return E_NOTIMPL;
This should give a proper trace line with arguments, and it should 
appear on every call.

> +    ITextFontImpl *This = impl_from_ITextFont(me);
> +    if (pValue)
> +        return E_INVALIDARG;
> +    if (!get_reOle(This))
> +        return CO_E_RELEASED;
> +
> +    FIXME("Stub\n");
> +    *pValue = tomFalse;
> +    return S_OK;
That's another example of tracing done wrong.

>   static HRESULT WINAPI ITextRange_fnGetFont(ITextRange *me, ITextFont **pFont)
>   {
>       ITextRangeImpl *This = impl_from_ITextRange(me);
> +    ITextFontImpl *txtFont = NULL;
> +    HRESULT hres;
> +
>       if (!This->reOle)
>           return CO_E_RELEASED;
>   
> -    FIXME("not implemented %p\n", This);
> -    return E_NOTIMPL;
> +    TRACE("%p\n", This);
And another.