Re: [PATCH] comctl32/taskdialog: Add callback and tests

[Nikolay Sivov]

On 28.07.2017 19:53, Fabian Maurer wrote:
> +static HRESULT callback(struct taskdialog_info *dialog_info, UINT uNotification, WPARAM wParam, LPARAM lParam)
> +{
> - if(dialog_info->callback)
> -
>    return dialog_info->callback(dialog_info->hwnd, uNotification, wParam, lParam, dialog_info->callback_data);
> - return S_OK;
>
> +}

Please use consistent naming and formatting, like space after 'if' for
example, and uNotification -> notification.
> -
>
> static INT_PTR CALLBACK taskdialog_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
>  {
> -
>
> static const WCHAR taskdialog_info_propnameW[] = {'T','a','s','k','D','i','a','l','o','g','I','n','f','o',0};
> -
>
> struct taskdialog_info *dialog_info;
> -
>
> TRACE("hwnd=%p msg=0x%04x wparam=%lx lparam=%lx\n", hwnd, msg, wParam, lParam);
> -
>
> if(msg != WM_INITDIALOG && msg != WM_NCDESTROY)
> -
>    dialog_info = GetPropW(hwnd, taskdialog_info_propnameW);
> -

Why WM_NCDESTROY is a special case?
>  switch (msg)
>  {
> -
>    case WM_INITDIALOG:
> -
>        dialog_info = (struct taskdialog_info *)lParam;
> -
>        dialog_info->hwnd = hwnd;
> -
>        SetPropW(hwnd, taskdialog_info_propnameW, dialog_info);
> -
> -
>        callback(dialog_info, TDN_DIALOG_CONSTRUCTED, 0, 0);
> -
>        callback(dialog_info, TDN_CREATED, 0, 0);
> -
>        break;

This looks suspicious, documentation is not strict on this, but it seems
TDN_CREATED corresponds for to WM_SHOWWINDOW stage.
>      case WM_COMMAND:
>          if (HIWORD(wParam) == BN_CLICKED)
>          {
>              WORD command_id = LOWORD(wParam);
> -
>            EndDialog(hwnd, command_id);
> -
>            click_button(dialog_info, command_id);
>            return TRUE;
>        }
>        break;
> -
>    case WM_DESTROY:
> -
>        callback(dialog_info, TDN_DESTROYED, 0, 0);
> -
>        RemovePropW(hwnd, taskdialog_info_propnameW);
> -
>        break;
> -
> -
>    /* Custom messages*/

What do you mean by "custom"?
> -
> -
>    case TDM_CLICK_BUTTON:
> -
>        click_button(dialog_info, wParam);
> -
>        break;
>  }
>  return FALSE;
>
> }
> @@ -507,17 +556,24 @@ HRESULT WINAPI TaskDialogIndirect(const TASKDIALOGCONFIG *taskconfig, int *butto
>                                    int *radio_button, BOOL *verification_flag_checked)
>  {
>      DLGTEMPLATE *template;
> -
>
> struct taskdialog_info dialog_info;
> INT ret;
>
> TRACE("%p, %p, %p, %p\n", taskconfig, button, radio_button, verification_flag_checked);
> -
>
> if(!taskconfig || taskconfig->cbSize != sizeof(TASKDIALOGCONFIG))
> -
>    return E_INVALIDARG;

Tests you added only check cbSize == 0 case.
> -
> - dialog_info.callback = taskconfig->pfCallback;
> - dialog_info.callback_data = taskconfig->lpCallbackData;
> - template = create_taskdialog_template(taskconfig);
> - ret = DialogBoxIndirectParamW(taskconfig->hInstance, template, taskconfig->hwndParent, taskdialog_proc, 0);
> -
>
> ret = DialogBoxIndirectParamW(taskconfig->hInstance, template, taskconfig->hwndParent, taskdialog_proc, (LPARAM)&dialog_info);
> Free(template);
>
> if (button) *button = ret;
> if (radio_button) *radio_button = taskconfig->nDefaultButton;
> - if (verification_flag_checked) *verification_flag_checked = TRUE;
> - if (verification_flag_checked) *verification_flag_checked = FALSE;

Why do you need this flag value change?
>  return S_OK;
>
> }
> diff --git a/dlls/comctl32/tests/taskdialog.c b/dlls/comctl32/tests/taskdialog.c
> index c937127897..867361617c 100644
> --- a/dlls/comctl32/tests/taskdialog.c
> +++ b/dlls/comctl32/tests/taskdialog.c
> @@ -24,13 +24,250 @@
>  #include "winuser.h"
>  #include "commctrl.h"
>
> +#include "wine/list.h"
>  #include "wine/test.h"
>  #include "v6util.h"
> +#include "msg.h"
> -
>
> +#define WM_TD_CALLBACK (WM_APP) /* Custom dummy message to wrap callback notifications */
> -
>
> +#define NUM_MSG_SEQUENCES     1
> +#define TASKDIALOG_SEQ_INDEX  0
> -
>
> +#define TEST_NUM_BUTTONS 20 /* Number of custom buttons to test with */
> -
>
> +#define ID_START 20 /* Lower IDs might be used by the system */
> +#define ID_START_BUTTON (ID_START + 0)
>
> static HRESULT (WINAPI *pTaskDialogIndirect)(const TASKDIALOGCONFIG *, int *, int *, BOOL *);
>  static HRESULT (WINAPI *pTaskDialog)(HWND, HINSTANCE, const WCHAR *, const WCHAR *, const WCHAR *,
>          TASKDIALOG_COMMON_BUTTON_FLAGS, const WCHAR *, int *);
>
> +static struct msg_sequence *sequences[NUM_MSG_SEQUENCES];
> -
>
> +/* Message lists to test against */
> -
>
> +struct message_send_info
> +{
> - UINT send_message;
> - WPARAM send_wparam;
> - LPARAM send_lparam;
> -
> - BOOL post; /* post instead of send */
> - const CHAR *title_target; /* control text, 0 means it's send to the dialog form instead */
>
> +};
> -
>
> +struct message_info
> +{
> - UINT recv_message; /* Message the callback receives */
> - WPARAM recv_wparam;
> - LPARAM recv_lparam;
> -
> - HRESULT ret; /* Value the callback should return */
> -
> - struct message_send_info send[9];  /* Message to send to trigger the next callback message */
>
> +};
> -
>
> +static const struct message_info *current_message_info;
> -
>
> +static const struct message_info mes_return_press_ok[] = {
> - { TDN_CREATED, 0, 0, S_OK, {
> -
>    { WM_KEYDOWN, VK_RETURN, 0, TRUE },
> -
>    { 0 }}},
> - { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}},
> - { 0 }
>
> +};
> -
>
> +static const struct message_info mes_cancel_button_press[] = {
> - { TDN_CREATED, 0, 0, S_OK, {
> -
>    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> -
>    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> -
>    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> -
>    { 0 }}},
> - { TDN_BUTTON_CLICKED, IDOK, 0, S_FALSE, {{ 0 }}},
> - { TDN_BUTTON_CLICKED, IDOK, 0, 0xFF, {{ 0 }}}, /* Random return value tested here */
> - { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}},
> - { 0 }
>
> +};
> -
>
> +static const struct message_info mes_send_button_directly[] = {
> - { TDN_CREATED, 0, 0, S_OK, {
> -
>    { WM_LBUTTONDOWN, MK_LBUTTON, 0, TRUE, "02" },
> -
>    { WM_LBUTTONUP,   0, 0, TRUE, "02" },
> -
>    { 0 }}},
> - { TDN_BUTTON_CLICKED, ID_START + 2, 0, S_OK, {{ 0 }}},
> - { 0 }
>
> +};

This looks too complicated, I'd just check notifications callback
received, using 'struct message' data, same way WM_NOTIFY tests work.
> -
>
> +/* Create a message to test against */
> +static struct message create_test_message(UINT message, WPARAM wParam, LPARAM lParam)
> +{
> - struct message mes;
> -
> - mes.message = WM_TD_CALLBACK;
> - mes.id = message;
> - mes.flags = sent|wparam|lparam|id;
> - mes.wParam = wParam;
> - mes.lParam = lParam;
> -
> - return mes;
>
> +}
> -
>
> +/* Our only way to get a button handle, since GetDlgItem and FindWindowEx don't work for the official taskdialog */
> -
>
> +static HWND taskdialog_child;
> +BOOL CALLBACK enum_taskdialog_children_proc(HWND hwnd, LPARAM lParam)
> +{
> - CHAR text[100];
> - const CHAR *title = (const CHAR *)lParam;
> -
> - GetWindowTextA(hwnd, text, sizeof(text));
> -
> - if(lstrcmpA(text, title) == 0)
> - {
> -
>    taskdialog_child = hwnd;
> -
>    return FALSE;
> - }
> -
> - return TRUE;
>
> +}
> -
>
> +static HWND get_child_from_title(HWND hwnd_parent, const CHAR *title)
> +{
> - taskdialog_child = NULL;
> - EnumChildWindows(hwnd_parent, enum_taskdialog_children_proc, (LPARAM)title);
> - return taskdialog_child;
>
> +}

I don't think we need to get that deep.
> - init_msg_sequences(sequences, NUM_MSG_SEQUENCES);
> - test_TaskDialogIndirect();

This name is too generic, in a test function like that you can test
invalid parameter handling for example. Then have another function for
callback notifications, and another one for TDM_* messages.

I think for first iteration test should simply check
creation/destruction notifications and callback user data value.

[PATCH] comctl32/taskdialog: Add callback and tests

[Fabian Maurer]

Hello,

sorry I thought I already sent this, but it apparently didn't work.
> This looks suspicious, documentation is not strict on this, but it seems
> TDN_CREATED corresponds for to WM_SHOWWINDOW stage.

I'm not sure, I'll look into it.
> > -
> >    /* Custom messages*/
>
> What do you mean by "custom"?

I meant taskdialog specific messages. Guess I'll change it to "Taskdialog 
messages"
> > - if (verification_flag_checked) *verification_flag_checked = TRUE;
> > - if (verification_flag_checked) *verification_flag_checked = FALSE;
>
> Why do you need this flag value change?

Because that's what the tests told me windows behaves like.
> >  return S_OK;
> >
> > }
> >
> > diff --git a/dlls/comctl32/tests/taskdialog.c
> > b/dlls/comctl32/tests/taskdialog.c index c937127897..867361617c 100644
> > --- a/dlls/comctl32/tests/taskdialog.c
> > +++ b/dlls/comctl32/tests/taskdialog.c
> > @@ -24,13 +24,250 @@
> >
> > #include "winuser.h"
> >  #include "commctrl.h"
> >
> > +#include "wine/list.h"
> >
> > #include "wine/test.h"
> >  #include "v6util.h"
> >
> > +#include "msg.h"
> > -
> >
> > +#define WM_TD_CALLBACK (WM_APP) /* Custom dummy message to wrap callback
> > notifications */ +
> > +#define NUM_MSG_SEQUENCES     1
> > +#define TASKDIALOG_SEQ_INDEX  0
> > -
> >
> > +#define TEST_NUM_BUTTONS 20 /* Number of custom buttons to test with */
> > -
> >
> > +#define ID_START 20 /* Lower IDs might be used by the system */
> > +#define ID_START_BUTTON (ID_START + 0)
> >
> > static HRESULT (WINAPI *pTaskDialogIndirect)(const TASKDIALOGCONFIG *,
> >  int *, int *, BOOL *); static HRESULT (WINAPI *pTaskDialog)(HWND,
> >  HINSTANCE, const WCHAR *, const WCHAR *, const WCHAR *,>  
> >          TASKDIALOG_COMMON_BUTTON_FLAGS, const WCHAR *, int *);
> >
> > +static struct msg_sequence *sequences[NUM_MSG_SEQUENCES];
> > -
> >
> > +/* Message lists to test against */
> > -
> >
> > +struct message_send_info
> > +{
> > - UINT send_message;
> > - WPARAM send_wparam;
> > - LPARAM send_lparam;
> > -
> > - BOOL post; /* post instead of send */
> > - const CHAR *title_target; /* control text, 0 means it's send to the
> >
> > dialog form instead */ +};
> > -
> >
> > +struct message_info
> > +{
> > - UINT recv_message; /* Message the callback receives */
> > - WPARAM recv_wparam;
> > - LPARAM recv_lparam;
> > -
> > - HRESULT ret; /* Value the callback should return */
> > -
> > - struct message_send_info send[9];  /* Message to send to trigger the
> >
> > next callback message */ +};
> > -
> >
> > +static const struct message_info *current_message_info;
> > -
> >
> > +static const struct message_info mes_return_press_ok[] = {
> > - { TDN_CREATED, 0, 0, S_OK, {
> > -
> >    { WM_KEYDOWN, VK_RETURN, 0, TRUE },
> > -
> >    { 0 }}},
> > - { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}},
> > - { 0 }
> >
> > +};
> > -
> >
> > +static const struct message_info mes_cancel_button_press[] = {
> > - { TDN_CREATED, 0, 0, S_OK, {
> > -
> >    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> > -
> >    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> > -
> >    { TDM_CLICK_BUTTON, IDOK, 0, TRUE },
> > -
> >    { 0 }}},
> > - { TDN_BUTTON_CLICKED, IDOK, 0, S_FALSE, {{ 0 }}},
> > - { TDN_BUTTON_CLICKED, IDOK, 0, 0xFF, {{ 0 }}}, /* Random return value
> >
> > tested here */ +    { TDN_BUTTON_CLICKED, IDOK, 0, S_OK, {{ 0 }}},
> > - { 0 }
> >
> > +};
> > -
> >
> > +static const struct message_info mes_send_button_directly[] = {
> > - { TDN_CREATED, 0, 0, S_OK, {
> > -
> >    { WM_LBUTTONDOWN, MK_LBUTTON, 0, TRUE, "02" },
> > -
> >    { WM_LBUTTONUP,   0, 0, TRUE, "02" },
> > -
> >    { 0 }}},
> > - { TDN_BUTTON_CLICKED, ID_START + 2, 0, S_OK, {{ 0 }}},
> > - { 0 }
> >
> > +};
>
> This looks too complicated, I'd just check notifications callback
> received, using 'struct message' data, same way WM_NOTIFY tests work.

I have more tests currently available, like cancelling tests. They're not part 
of this patch, but I figured I'll do it right from the beginning.
> > +static HWND get_child_from_title(HWND hwnd_parent, const CHAR *title)
> > +{
> > - taskdialog_child = NULL;
> > - EnumChildWindows(hwnd_parent, enum_taskdialog_children_proc,
> >
> > (LPARAM)title); +    return taskdialog_child;
> > +}
>
> I don't think we need to get that deep.

Are you sure? I figured it would make sense to actually test for simulated 
mouse clicks instead of just sending the taskdialog specific click messages.
> I think for first iteration test should simply check
> creation/destruction notifications and callback user data value.

I'll take care of the issues and resend the patch as simplified version.

Regards
Fabian Maurer