Re: [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.

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

[Jactry Zeng]

Hi Nikolay,

Thanks for your review!
I will have another try.

2014-09-23 23:01 GMT+08:00 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.