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

[Nikolay Sivov]

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.
> +#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.

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

[Joachim Priesner]

> 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?

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.
> > +#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)?

P.S. On a general note I forgot to add: This patch is heavily based on dlls/user32/msgbox.c.

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