Re: Nikolay Sivov : comctl32/taskdialog: Initial implementation of a minimal task dialog.

[Fabian Maurer]

> Author: Nikolay Sivov <nsivov(a)codeweavers.com>
> Date:   Tue Mar 21 17:31:20 2017 +0300
>
> comctl32/taskdialog: Initial implementation of a minimal task dialog.
>
> Based on patches by Fabian Maurer.

Thanks for taking the time to rework it, really appreciate it!
However, there's a few questions I'd like to ask.
> static const WCHAR nulW;
> Shouldn't this be set 0?
> if (radio_button) *radio_button = taskconfig->nDefaultButton;
> This probably should have been changed, it was already wrong in the old 
> implementation.
> /* write control entries */
>    LIST_FOR_EACH_ENTRY_SAFE(control, control2, &desc.controls, struct 
> taskdialog_control, entry)
>   {
>       ALIGN_POINTER(ptr, 3);
>
> template_write_data(&ptr, control->template, control->template_size);
>
> /* list item won't be needed later */
>        list_remove(&control->entry);
>        Free(control->template);
>        Free(control);
>    }
> Wouldn't it be shorter to also use "taskdialog_clear_controls" here? Just 
> curious on why you chose to write it again.

Also, wouldn't it make sense to keep the template generating code separated 
from the taskdialog specific bits? I intentionally split it, so we could use 
those function in another place, if needed. In my opinion, that also makes the 
code easier readable.

Finally, are you adding the rest from my latest patch-set too, or should I 
adapt the missing parts?

Regards,
Fabian Maurer

Nikolay Sivov : comctl32/taskdialog: Initial implementation of a minimal task dialog.

[Nikolay Sivov]

On 23.03.2017 3:40, Fabian Maurer wrote:
> > static const WCHAR nulW;
> > Shouldn't this be set 0?

It's static, so it's already initialized.
> > if (radio_button) *radio_button = taskconfig->nDefaultButton;
> > This probably should have been changed, it was already wrong in the old 
> > implementation.

Maybe it is, but I'd rather keep it as is, until we have actual support
for radio buttons.
> > /* write control entries */
> >    LIST_FOR_EACH_ENTRY_SAFE(control, control2, &desc.controls, struct 
> > taskdialog_control, entry)
> >   {
> >       ALIGN_POINTER(ptr, 3);
> >
> > template_write_data(&ptr, control->template, control->template_size);
> >
> > /* list item won't be needed later */
> >        list_remove(&control->entry);
> >        Free(control->template);
> >        Free(control);
> >    }
> > Wouldn't it be shorter to also use "taskdialog_clear_controls" here? Just 
> > curious on why you chose to write it again.

I didn't want to do another loop just to clear the list.
> Also, wouldn't it make sense to keep the template generating code separated 
> from the taskdialog specific bits? I intentionally split it, so we could use 
> those function in another place, if needed. In my opinion, that also makes the 
> code easier readable.

We can split it up once it's useful. Right now I don't see where we
could need it in comctl32.
> Finally, are you adding the rest from my latest patch-set too, or should I 
> adapt the missing parts?

No, please go ahead.
> Regards,
> Fabian Maurer