[PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file

[Nikolay Sivov]

On 10/2/2014 13:10, Joachim Priesner wrote:
>> On 10/1/2014 16:35, Joachim Priesner wrote:
>>> +#include <stdarg.h>
>>> +#include <string.h>
>>> +
>>> +#include "comctl32.h"
>>> +#include "shlwapi.h"
>>> +
>>> +#include "windef.h"
>>> +#include "winbase.h"
>>> +#include "wingdi.h"
>>> +#include "winternl.h"
>>> +#include "dlgs.h"
>>> +#include "wine/debug.h"
>>> +#include "wine/unicode.h"
>> It's unlikely you need all of those.
> I'll need them for patch 2 of 3 though, and wanted to balance the size of the patches. Shall I move them to the later patch?
Yes, I think it's better to add those step by step.
>
> That said, if I resend the patches: Do patches in a series have to retain all functionality when applied on their own? If not, I could make the patch smaller by not moving the old TaskDialogIndirect implementation to the new file, but creating a stub instead.
Patch series should apply from first to last patch incrementally, 
meaning to apply patch n you have to apply patches 1..n-1 first (for n > 
1 of course).

You did it right - first patch just moves to a separate file, then next 
adds more and so on.

I looked at patches 2 and 3, and got some more:

> +HICON TASKDIALOG_GetIcon(HINSTANCE hInstance, PCWSTR pszIcon)
Please avoid prefixed names like that, instead call it 'icon'.

> +    /* If hInstance is NULL and pszMainIcon is not one if the TD_*_ICON constants, the icon
> +     * is taken from imageres.dll. Until this is implemented, hard-code some of the offsets
> +     * for standard icons in imageres.dll. */
Is this documented somewhere?

> +            windowTitleBuffer = HeapAlloc(GetProcessHeap(), 0, (len + 1) * sizeof(WCHAR));
comctl32 code traditionally uses Alloc()/Free().

> +    unsigned char hasButton = 0;
This should be BOOL.

> +    static const WCHAR task_dialog_resource_nameW[] = { 'T','A','S','K','D','I','A','L','O','G',0 };
It's easier to define a resource id for that.

> +    if (contentTextBuffer)
> +        HeapFree(GetProcessHeap(), 0, contentTextBuffer);
> +    if (mainInstructionBuffer)
> +        HeapFree(GetProcessHeap(), 0, mainInstructionBuffer);
You don't have to check for nulls.
>
>>> +#define TD_WARNING_ICON         MAKEINTRESOURCEW(-1)
>>> +#define TD_ERROR_ICON           MAKEINTRESOURCEW(-2)
>>> +#define TD_INFORMATION_ICON     MAKEINTRESOURCEW(-3)
>>> +#define TD_SHIELD_ICON          MAKEINTRESOURCEW(-4)
>> Where this comes from? I mean particular id values.
> Those are the definitions from Windows' CommCtrl.h. Is it okay to use these directly or do I have to write a test program that prints the value of these constants (and get the same results)?
Oh, sorry. I missed that you add those to a public header. It's fine to 
use them of course.
>
> P.S. On a general note I forgot to add: This patch is heavily based on dlls/user32/msgbox.c.
That's not a problem.