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.