Re: [PATCH 3/5] shell32: Implement SHOpenPropSheet

[Nikolay Sivov]

On 8/21/2011 19:20, Jay Yang wrote:
> ---
>   dlls/shell32/shell32.spec |    3 ++
>   dlls/shell32/shellord.c   |   88 +++++++++++++++++++++++++++++++++++++++++++++
>   include/shlobj.h          |    4 ++
>   3 files changed, 95 insertions(+), 0 deletions(-)
> It's not clear for me why you placed this in shellord.c, it's supposed 
> to contain exported by ordinal calls only, as I understand it. If 
> Julliard approves new file for propsheets then you should place this in 
> it. IMO it's ok to place all of this in shlmenu.c for example.
> +BOOL WINAPI SHOpenPropSheetA(LPCSTR pszCaption, HKEY *ahkey, UINT ckeys, const CLSID *pclsidDef, IDataObject *pdtobj, IShellBrowser *psb, LPCSTR pStartPage)
> +{
> +    BOOL ret;
> +    LPWSTR wszCaption,wszStartPage;
> +    UINT length;
> +    length = MultiByteToWideChar(CP_ACP, 0, pszCaption, -1, NULL, 0);
> +    wszCaption = HeapAlloc(GetProcessHeap(),0,sizeof(WCHAR)*length);
> +    length = MultiByteToWideChar(CP_ACP, 0, pszCaption, -1, wszCaption, length);
> +    length = MultiByteToWideChar(CP_ACP, 0, pStartPage, -1, NULL, 0);
> +    wszStartPage = HeapAlloc(GetProcessHeap(),0,sizeof(WCHAR)*length);
> +    length = MultiByteToWideChar(CP_ACP, 0, pStartPage, -1, wszStartPage, length);
> +    ret = SHOpenPropSheetW(wszCaption,ahkey,ckeys,pclsidDef,pdtobj,psb,wszStartPage);
> +    HeapFree(GetProcessHeap(),0,wszCaption);
> +    HeapFree(GetProcessHeap(),0,wszStartPage);
> +    return ret;
> +}
> Hmm, actually there's HEAP_strdupAtoW() for that (you need to make in 
> non-static though).
> +	ZeroMemory(&psh, sizeof(PROPSHEETHEADERW));
> +	psh.dwSize = sizeof (PROPSHEETHEADERW);
> +	psh.hwndParent = NULL;
> +	psh.dwFlags = PSH_PROPTITLE;
> +    if(pStartPage)
> +    {
> +        psh.dwFlags |= PSH_USEPSTARTPAGE;
> +        psh.u2.pStartPage = pStartPage;
> +    }
> Please don't mix tabs with spaces, use 4 spaces everywhere.
> +    if(pszCaption)
> +        psh.pszCaption = pszCaption;
> +    else
> +        psh.pszCaption = empty_str;
> Is it really needed? I mean pszCaption == NULL is not the same as empty 
> string in this context?
> +    IShellPropSheetExt *def_ext;
> +    IShellExtInit *init_iface;
> +    PROPSHEETHEADERW psh;
> +	HPROPSHEETPAGE pages[99];
> It's not clear again where this size comes from, just enough for anyone? 
> Also I think it's better to reduce scope as much as possible, def_ext 
> doesn't need to be global for example.