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 <bunglehead(a)gmail.com>:
> +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.

-- 
Regards,
Jactry Zeng