[PATCH] comctl32/taskdialog: Implement nDefaultButton and add tests
Fabian Maurer
dark.shadow4 at web.de
Sun Nov 5 05:17:15 CST 2017
> 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
More information about the wine-devel
mailing list