Re: [PATCH] comctl32/taskdialog: Implement nDefaultButton and add tests

[Nikolay Sivov]

On 04.11.2017 18:22, Fabian Maurer wrote:
> Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>
> ---
>  dlls/comctl32/taskdialog.c       |  21 +++++--
>  dlls/comctl32/tests/taskdialog.c | 115 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c
> index 5c4af2ab03..4f80311a61 100644
> --- a/dlls/comctl32/taskdialog.c
> +++ b/dlls/comctl32/taskdialog.c
> @@ -151,7 +151,7 @@ static void taskdialog_get_text_extent(const struct taskdialog_template_desc *de
>  }
>
> static unsigned int taskdialog_add_control(struct taskdialog_template_desc *desc, WORD id, const WCHAR *class,
> -        HINSTANCE hInstance, const WCHAR *text, short x, short y, short cx, short cy)
> +        HINSTANCE hInstance, const WCHAR *text, DWORD style, short x, short y, short cx, short cy)
>  {
>      struct taskdialog_control *control = Alloc(sizeof(*control));
>      unsigned int size, class_size, text_size;
> @@ -178,7 +178,7 @@ static unsigned int taskdialog_add_control(struct taskdialog_template_desc *desc
>      control->template = template = Alloc(size);
>      control->template_size = size;
>
> -    template->style = WS_VISIBLE;
> +    template->style = WS_VISIBLE | style;
>      template->dwExtendedStyle = 0;
>      template->x = x;
>      template->y = y;
> @@ -206,7 +206,7 @@ static unsigned int taskdialog_add_static_label(struct taskdialog_template_desc
>      taskdialog_get_text_extent(desc, str, TRUE, &sz);
>
> desc->dialog_height += DIALOG_SPACING;
> -    size = taskdialog_add_control(desc, id, WC_STATICW, desc->taskconfig->hInstance, str, DIALOG_SPACING,
> +    size = taskdialog_add_control(desc, id, WC_STATICW, desc->taskconfig->hInstance, str, 0, DIALOG_SPACING,
>              desc->dialog_height, sz.cx, sz.cy);
>      desc->dialog_height += sz.cy + DIALOG_SPACING;
>      return size;
> @@ -268,6 +268,7 @@ static unsigned int taskdialog_add_buttons(struct taskdialog_template_desc *desc
>      unsigned int location_x, *line_widths, alignment = ~0u;
>      const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
>      struct taskdialog_button_desc *buttons;
> +    int default_button;
>
> /* Allocate enough memory for the custom and the default buttons. Maximum 6 default buttons possible. */
>      buttons_size = 6;
> @@ -340,18 +341,26 @@ static unsigned int taskdialog_add_buttons(struct taskdialog_template_desc *desc
>              alignment = new_alignment;
>      }
>
> +    /* Check if the default button is actually valid, if no default to the first created button */
> +    default_button = buttons[0].id;
> +    for (i = 0; i < count; i++)
> +        if (buttons[i].id == taskconfig->nDefaultButton)
> +            default_button = taskconfig->nDefaultButton;
> +

We already iterate over custom buttons and check every common button
flag before this point, so it shouldn't be necessary to have another loop.
> @@ -583,7 +592,7 @@ HRESULT WINAPI TaskDialogIndirect(const TASKDIALOGCONFIG *taskconfig, int *butto
>      Free(template);
>
> if (button) *button = ret;
> -    if (radio_button) *radio_button = taskconfig->nDefaultButton;
> +    if (radio_button) *radio_button = FALSE;
>      if (verification_flag_checked) *verification_flag_checked = TRUE;

Same as with verification flag, we don't implement radio buttons
currently so it's better to leave it as is to avoid possible regressions
until we add support for them. Also you're supposed to return button id,
not binary flags.
> +static TASKDIALOG_BUTTON* buttons_make(void)
> +{
> +    static const WCHAR str_format[] = {'%','0','2','d',0};
> +    static TASKDIALOG_BUTTON buttons[TEST_NUM_BUTTONS];
> +    static WCHAR titles[TEST_NUM_BUTTONS * 3]; /* Each button has two digits as title, plus null-terminator */
> +    int i;
> +
> +    for (i = 0; i < TEST_NUM_BUTTONS; i++)
> +    {
> +        WCHAR *text = &titles[i * 3];
> +        wsprintfW(text, str_format, i);
> +
> +        buttons[i].pszButtonText = text;
> +        buttons[i].nButtonID = ID_START_BUTTON + i;
> +    }
> +    return buttons;
> +}
> +

Please merge this to test function.

[PATCH] comctl32/taskdialog: Implement nDefaultButton and add tests

[Fabian Maurer]

> We already iterate over custom buttons and check every common button
> flag before this point, so it shouldn't be necessary to have another loop.

I'm open to suggestion, but how would you do that?

We could add three checks each time before taskdialog_init_button is called, 
or alternatively one check in taskdialog_init_button. Like giving it a pointer 
to default_button, and checking if the id equal the taskdialog default button.
But I find my current solution to be cleaner to be honest, what's your 
suggestion?
> > @@ -583,7 +592,7 @@ HRESULT WINAPI TaskDialogIndirect(const
> > TASKDIALOGCONFIG *taskconfig, int *butto> 
> >      Free(template);
> >
> > if (button) *button = ret;
> >
> > -    if (radio_button) *radio_button = taskconfig->nDefaultButton;
> > +    if (radio_button) *radio_button = FALSE;
> >
> > if (verification_flag_checked) *verification_flag_checked = TRUE;
>
> Same as with verification flag, we don't implement radio buttons
> currently so it's better to leave it as is to avoid possible regressions
> until we add support for them. Also you're supposed to return button id,
> not binary flags.

Yes, it should be 0 instead of FALSE. But the way it is, returning 
"taskconfig->nDefaultButton", that's obviously wrong. The tests show it has to 
be 0, and even if we implement it, the result when not setting radio buttons 
won't change, right?
> > +static TASKDIALOG_BUTTON* buttons_make(void)
> > +{
> > +    static const WCHAR str_format[] = {'%','0','2','d',0};
> > +    static TASKDIALOG_BUTTON buttons[TEST_NUM_BUTTONS];
> > +    static WCHAR titles[TEST_NUM_BUTTONS * 3]; /* Each button has two
> > digits as title, plus null-terminator */ +    int i;
> > +
> > +    for (i = 0; i < TEST_NUM_BUTTONS; i++)
> > +    {
> > +        WCHAR *text = &titles[i * 3];
> > +        wsprintfW(text, str_format, i);
> > +
> > +        buttons[i].pszButtonText = text;
> > +        buttons[i].nButtonID = ID_START_BUTTON + i;
> > +    }
> > +    return buttons;
> > +}
> > +
>
> Please merge this to test function.

Wouldn't it make sense to leave it separate? It might be used in another test 
function. It also nicely encapsulates the static variables, that was the 
primary reason I put it into its own function.

Regards,
Fabian Maurer

[PATCH] comctl32/taskdialog: Implement nDefaultButton and add tests

[Nikolay Sivov]

On 05.11.2017 14:17, Fabian Maurer wrote:
> > We already iterate over custom buttons and check every common button
> > flag before this point, so it shouldn't be necessary to have another loop.
>
> I'm open to suggestion, but how would you do that?
>
> We could add three checks each time before taskdialog_init_button is called, 
> or alternatively one check in taskdialog_init_button. Like giving it a pointer 
> to default_button, and checking if the id equal the taskdialog default button.
> But I find my current solution to be cleaner to be honest, what's your 
> suggestion?

Yes, something like that. taskdialog_init_button() has all the info to
do that.
> > > @@ -583,7 +592,7 @@ HRESULT WINAPI TaskDialogIndirect(const
> > > TASKDIALOGCONFIG *taskconfig, int *butto> 
> > >      Free(template);
> > >
> > > if (button) *button = ret;
> > >
> > > -    if (radio_button) *radio_button = taskconfig->nDefaultButton;
> > > +    if (radio_button) *radio_button = FALSE;
> > >
> > > if (verification_flag_checked) *verification_flag_checked = TRUE;
> >
> > Same as with verification flag, we don't implement radio buttons
> > currently so it's better to leave it as is to avoid possible regressions
> > until we add support for them. Also you're supposed to return button id,
> > not binary flags.
>
> Yes, it should be 0 instead of FALSE. But the way it is, returning 
> "taskconfig->nDefaultButton", that's obviously wrong. The tests show it has to 
> be 0, and even if we implement it, the result when not setting radio buttons 
> won't change, right?

It is wrong now, but it's been like that for a while, I don't want to
risk that.
> > > +static TASKDIALOG_BUTTON* buttons_make(void)
> > > +{
> > > +    static const WCHAR str_format[] = {'%','0','2','d',0};
> > > +    static TASKDIALOG_BUTTON buttons[TEST_NUM_BUTTONS];
> > > +    static WCHAR titles[TEST_NUM_BUTTONS * 3]; /* Each button has two
> > > digits as title, plus null-terminator */ +    int i;
> > > +
> > > +    for (i = 0; i < TEST_NUM_BUTTONS; i++)
> > > +    {
> > > +        WCHAR *text = &titles[i * 3];
> > > +        wsprintfW(text, str_format, i);
> > > +
> > > +        buttons[i].pszButtonText = text;
> > > +        buttons[i].nButtonID = ID_START_BUTTON + i;
> > > +    }
> > > +    return buttons;
> > > +}
> > > +
> >
> > Please merge this to test function.
>
> Wouldn't it make sense to leave it separate? It might be used in another test 
> function. It also nicely encapsulates the static variables, that was the 
> primary reason I put it into its own function.

I don't think it makes sense to have it separated, not in a way it's
done currently.
> Regards,
> Fabian Maurer