Re: OLEAUT32: implementation for IPersistPropertyBag_Load for OLEFont

[Robert Shearman]

Alex Villací­s Lasso wrote:
> This implementation is enough to satisfy VB6 when loading font objects 
> from property bags in projects.
>
> Changelog:
> - Added implementation for IPersistPropertyBag_Load on OLEFont

Looks good, but a few comments below:

...
> -
>    V_VT(&valueAttr) = VT_EMPTY;

It is probably better practice to use VariantInit here.
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrName, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
>
>
> -
>    case S_OK:
> -
>        if (V_VT(&valueAttr) == VT_BSTR) {
> -
>            /* Type is BSTR, use for name directly */
> -
>            OLEFontImpl_put_Name(this, V_BSTR(&valueAttr));
> -
> -
>            /* FIXME: Leaking memory here? */
> -
>        } else {
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Name has type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>        }

Perhaps use VariantChangeType here?
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }

Catch the errors you need to (why is E_INVALIDARG ok?) and either move 
these common debug statements to the end of the function (they should 
also be ERR's, not FIXME's). Also, you other unexpected errors, which is 
bad.
> - }
> -
> - if (iRes == S_OK) {
> -
>    CY iSize;
> -
>    iSize.s.Hi = 0;
> -
>    iSize.s.Lo = 0;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrSize, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            iSize.s.Lo = V_I2(&valueAttr) * 10000UL;
> -
>            break;
> -
>        case VT_UI2:
> -
>            iSize.s.Lo = V_UI2(&valueAttr) * 10000UL;
> -
>            break;
> -
>        case VT_I4:
> -
>            iSize.s.Lo = V_I4(&valueAttr) * 10000UL;
> -
>            break;
> -
>        case VT_UI4:
> -
>            iSize.s.Lo = V_UI4(&valueAttr) * 10000UL;
> -
>            break;
> -
>        case VT_R4:
> -
>            iSize.s.Lo = (ULONG)(V_R4(&valueAttr) * 10000UL);
> -
>            break;
> -
>        case VT_R8:
> -
>            iSize.s.Lo = (ULONG)(V_R8(&valueAttr) * 10000UL);
> -
>            break;

Again, perhaps use VariantChangeType here?
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Size type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>            break;
> -
>        }
> -
>        if (iRes != E_NOTIMPL) IFont_put_Size(this, iSize);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }

Same comment as before about errors.
> - }
> - if (iRes == S_OK) {
> -
>    signed short iCharset = 0;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrCharset, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            if (V_I2(&valueAttr) < 0) {
> -
>                FIXME("Unexpected value for Font Charset (%hd), using 0\n", V_I2(&valueAttr));
> -
>            } else {
> -
>                iCharset = V_I2(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_UI2:
> -
>            if (V_UI4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Charset (%ld), using 0\n", V_UI4(&valueAttr));
> -
>            } else {
> -
>                iCharset = V_UI2(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_I4:
> -
>            if (V_I4(&valueAttr) < 0 || V_I4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Charset (%ld), using 0\n", V_I4(&valueAttr));
> -
>            } else {
> -
>                iCharset = V_I4(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_UI4:
> -
>            if (V_UI4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Charset (%ld), using 0\n", V_UI4(&valueAttr));
> -
>            } else {
> -
>                iCharset = V_UI4(&valueAttr);
> -
>            }
> -
>            break;
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Charset type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;

Use VariantChangeType here and you will get all the bounds checking for 
free.
> -
>        }
> -
>        if (iRes != E_NOTIMPL) IFont_put_Charset(this, iCharset);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }
> - }
> -
> - if (iRes == S_OK) {
> -
>    signed short iWeight = FW_NORMAL;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrWeight, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            if (V_I2(&valueAttr) < 0) {
> -
>                FIXME("Unexpected value for Font Weight (%hd), using FW_NORMAL\n", V_I2(&valueAttr));
> -
>            } else {
> -
>                iWeight = V_I2(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_UI2:
> -
>            if (V_UI4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Weight (%ld), using FW_NORMAL\n", V_UI4(&valueAttr));
> -
>            } else {
> -
>                iWeight = V_UI2(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_I4:
> -
>            if (V_I4(&valueAttr) < 0 || V_I4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Weight (%ld), using FW_NORMAL\n", V_I4(&valueAttr));
> -
>            } else {
> -
>                iWeight = V_I4(&valueAttr);
> -
>            }
> -
>            break;
> -
>        case VT_UI4:
> -
>            if (V_UI4(&valueAttr) > 32767L) {
> -
>                FIXME("Unexpected value for Font Weight (%ld), using FW_NORMAL\n", V_UI4(&valueAttr));
> -
>            } else {
> -
>                iWeight = V_UI4(&valueAttr);
> -
>            }
> -
>            break;
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Weight type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>        }

Again, use VariantChangeType.
> -
>        if (iRes != E_NOTIMPL) IFont_put_Weight(this, iWeight);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }
> - }
> -
> - if (iRes == S_OK) {
> -
>    BOOL bUnderline = FALSE;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrUnderline, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            bUnderline = V_I2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_I4:
> -
>            bUnderline = V_I4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI2:
> -
>            bUnderline = V_UI2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI4:
> -
>            bUnderline = V_UI4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_BOOL:
> -
>            bUnderline = V_BOOL(&valueAttr);
> -
>            break;
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Underline type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>            break;
> -
>        }

Again, VariantChangeType.
> -
>        if (iRes != E_NOTIMPL) IFont_put_Underline(this, bUnderline);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }
> - }
> -
> - if (iRes == S_OK) {
> -
>    BOOL bItalic = FALSE;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrItalic, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            bItalic = V_I2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_I4:
> -
>            bItalic = V_I4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI2:
> -
>            bItalic = V_UI2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI4:
> -
>            bItalic = V_UI4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_BOOL:
> -
>            bItalic = V_BOOL(&valueAttr);
> -
>            break;
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Italic type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>            break;
> -
>        }

Again, use VariantChangeType.
> -
>        if (iRes != E_NOTIMPL) IFont_put_Italic(this, bItalic);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }
> - }
> -
> - if (iRes == S_OK) {
> -
>    BOOL bStrikethrough = FALSE;
> -
> -
>    V_VT(&valueAttr) = VT_EMPTY;
> -
>    iRes = IPropertyBag_Read(pPropBag, sAttrStrikethrough, &valueAttr, pErrorLog);
> -
>    switch (iRes) {
> -
>    case S_OK:
> -
>        switch (V_VT(&valueAttr)) {
> -
>        case VT_I2:
> -
>            bStrikethrough = V_I2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_I4:
> -
>            bStrikethrough = V_I4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI2:
> -
>            bStrikethrough = V_UI2(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_UI4:
> -
>            bStrikethrough = V_UI4(&valueAttr) ? TRUE : FALSE;
> -
>            break;
> -
>        case VT_BOOL:
> -
>            bStrikethrough = V_BOOL(&valueAttr);
> -
>            break;
> -
>        default:
> -
>            /* Don't know how to handle this type */
> -
>            FIXME("Strikethrough type %d, not implemented\n", V_VT(&valueAttr));
> -
>            iRes = E_NOTIMPL;
> -
>            break;
> -
>        }

Again, use VariantChangeType.
> -
>        if (iRes != E_NOTIMPL) IFont_put_Strikethrough(this, bStrikethrough);
> -
>        break;
> -
>    case E_POINTER:
> -
>        FIXME("\tiResult = [E_POINTER]\n");
> -
>        break;
> -
>    case E_INVALIDARG:
> -
>        FIXME("\tiResult = [undef]\n");
> -
>        iRes = S_OK;
> -
>        break;
> -
>    case E_FAIL:
> -
>        FIXME("\tiResult = [E_FAIL]\n");
> -
>        break;
> -
>    }
> - }
> - return iRes;
>
> }
>
> static HRESULT WINAPI OLEFontImpl_IPersistPropertyBag_Save(

Thanks,

Rob