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