Re: [PATCH] comctl32/taskdialog: Implement buttons.

[Nikolay Sivov]

On 23.03.2017 21:22, Fabian Maurer wrote:
> Signed-off-by: Fabian Maurer
>
> dlls/comctl32/taskdialog.c | 224 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 161 insertions(+), 63 deletions(-)
>
> diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c
> index 6b8bcfcae6..037b85e082 100644
> --- a/dlls/comctl32/taskdialog.c
> +++ b/dlls/comctl32/taskdialog.c
> @@ -41,10 +41,13 @@ WINE_DEFAULT_DEBUG_CHANNEL(taskdialog);
>  #define ALIGN_LENGTH(_Len, _Align) _Len = ALIGNED_LENGTH(_Len, _Align)
>  #define ALIGN_POINTER(_Ptr, _Align) _Ptr = ALIGNED_POINTER(_Ptr, _Align)
>
> -static const UINT DIALOG_MIN_WIDTH = 180;
> +static const UINT DIALOG_MIN_WIDTH = 240;
>  static const UINT DIALOG_SPACING = 5;
> -static const UINT DIALOG_BUTTON_WIDTH = 50;
> -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> +static const UINT DIALOG_BUTTON_HEIGHT = 13;

This was taken from MSGBOX template, what's a point in making it different?
> +static const UINT DIALOG_SPACING_BUTTONS_LEFT = 30; /* minimum distance from the left dialog border */
> +static const UINT DIALOG_SPACING_BUTTON_H = 1; /* Distance between buttons */
> +static const UINT DIALOG_SPACING_BUTTON_W = 4; /* Distance between buttons */
>
> static const UINT ID_MAIN_INSTRUCTION = 0xf000;
>
> @@ -67,6 +70,15 @@ struct taskdialog_template_desc
>      HFONT font;
>  };
>
> +typedef struct
> +{
> - int id;
> - const WCHAR *text;
> - UINT width;
> - UINT x;
> - UINT y;
>
> +} button_info;
> -
>
> static void pixels_to_dialogunits(const struct taskdialog_template_desc *desc, LONG *width, LONG *height)
>  {
>      if (width)
> @@ -83,6 +95,44 @@ static void dialogunits_to_pixels(const struct taskdialog_template_desc *desc, L
>          *height = MulDiv(*height, desc->y_baseunit, 8);
>  }
>
> +/* used to calculate size for the controls */
> +static RECT text_get_rect(const struct taskdialog_template_desc *desc, const WCHAR *text)
> +{
> - const WCHAR *textW = NULL;
> - unsigned int length;
> - HFONT oldfont;
> - HDC hdc;
> - static const WCHAR nulW;
> - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /* padding left and right of the control */
> -
> - if (IS_INTRESOURCE(text))
> - {
> -
>    if (!(length = LoadStringW(desc->taskconfig->hInstance, (UINT_PTR)text, (WCHAR *)&textW, 0)))
> -
>    {
> -
>        WARN("Failed to load text\n");
> -
>        textW = &nulW;
> -
>        length = 0;
> -
>    }
> - }
> - else
> - {
> -
>    textW = text;
> -
>    length = strlenW(textW);
> - }
> -
> - hdc = GetDC(0);
> - oldfont = SelectObject(hdc, desc->font);
> -
> - dialogunits_to_pixels(desc, &rect.right, NULL);
> - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS | DT_CALCRECT | DT_WORDBREAK);
> - pixels_to_dialogunits(desc, &rect.right, &rect.bottom);
> -
> - SelectObject(hdc, oldfont);
> - ReleaseDC(0, hdc);
> -
> - return rect;
>
> +}
> -

Did you test that buttons are supposed to support multiple lines of text?
> static void template_write_data(char **ptr, const void *src, unsigned int size)
>  {
>      memcpy(*ptr, src, size);
> @@ -90,7 +140,7 @@ static void template_write_data(char **ptr, const void *src, unsigned int size)
>  }
>
> 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)
> -
>    DWORD style, HINSTANCE hInstance, const WCHAR *text, short x, short y, short cx, short cy)
>
> {
>      struct taskdialog_control *control = Alloc(sizeof(*control));
>      unsigned int size, class_size, text_size;
> @@ -117,7 +167,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 | WS_CHILD | style;

You don't use styles in this patch, this is unnecessary change.
>  template->dwExtendedStyle = 0;
>  template->x = x;
>  template->y = y;
>
> @@ -136,78 +186,126 @@ static unsigned int taskdialog_add_control(struct taskdialog_template_desc *desc
>
> static unsigned int taskdialog_add_main_instruction(struct taskdialog_template_desc *desc)
>  {
> - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /* padding left and right of the control */
> - const WCHAR *textW = NULL;
> - unsigned int size, length;
> - HFONT oldfont;
> - HDC hdc;
> -
>
> unsigned int size;
> -
>
> RECT rect;
>
> if (!desc->taskconfig->pszMainInstruction)
>     return 0;
> - if (IS_INTRESOURCE(desc->taskconfig->pszMainInstruction))
> - {
> -
>    if (!(length = LoadStringW(desc->taskconfig->hInstance, (UINT_PTR)desc->taskconfig->pszMainInstruction,
> -
>            (WCHAR *)&textW, 0)))
> -
>    {
> -
>        WARN("Failed to load main instruction text\n");
> -
>        return 0;
> -
>    }
> - }
> - else
> - {
> -
>    textW = desc->taskconfig->pszMainInstruction;
> -
>    length = strlenW(textW);
> - }
> -
> - hdc = GetDC(0);
> - oldfont = SelectObject(hdc, desc->font);
> -
> - dialogunits_to_pixels(desc, &rect.right, NULL);
> - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS | DT_CALCRECT | DT_WORDBREAK);
> - pixels_to_dialogunits(desc, &rect.right, &rect.bottom);
> -
> - SelectObject(hdc, oldfont);
> - ReleaseDC(0, hdc);
> - rect = text_get_rect(desc, desc->taskconfig->pszMainInstruction);
> - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION, WC_STATICW, desc->taskconfig->hInstance,
> - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION, WC_STATICW, 0, desc->taskconfig->hInstance,
>         desc->taskconfig->pszMainInstruction, DIALOG_SPACING, desc->dialog_height,
>         rect.right, rect.bottom);
> - desc->dialog_height += rect.bottom;
> - desc->dialog_height += rect.bottom + DIALOG_SPACING;
> return size;
>
> }
>
> -static unsigned int taskdialog_add_common_buttons(struct taskdialog_template_desc *desc)
> +static button_info make_button(struct taskdialog_template_desc *desc, int id, const WCHAR *text)
> +{
> - RECT rect;
> - button_info button;
> -
> - button.id = id;
> - button.text = text;
> - rect = text_get_rect(desc, text);
> - button.width = rect.right + 10;
> - if (button.width < DIALOG_BUTTON_WIDTH_MIN)
> -
>    button.width = DIALOG_BUTTON_WIDTH_MIN;
> -
> - return button;
>
> +}

This does not handle "text" correctly.
> -
>
> +static unsigned int taskdialog_add_buttons(struct taskdialog_template_desc *desc)
>  {
> - short button_x = desc->dialog_width - DIALOG_BUTTON_WIDTH - DIALOG_SPACING;
> DWORD flags = desc->taskconfig->dwCommonButtons;
> - const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
> - UINT alignment = DIALOG_SPACING_BUTTONS_LEFT; /* minimum distance from the left dialog border */
> - UINT location_x;
> - BOOL first_row = TRUE;
> - button_info *buttons;
> - int count = 0;
> - int i;
> unsigned int size = 0;
>
> -#define TASKDIALOG_ADD_COMMON_BUTTON(id) \
> - do { \
> -
>    size += taskdialog_add_control(desc, ID##id, WC_BUTTONW, COMCTL32_hModule, MAKEINTRESOURCEW(IDS_BUTTON_##id), \
> -
>        button_x, desc->dialog_height + DIALOG_SPACING, DIALOG_BUTTON_WIDTH, DIALOG_BUTTON_HEIGHT); \
> -
>    button_x -= DIALOG_BUTTON_WIDTH + DIALOG_SPACING; \
> - } while(0)
> - if (flags & TDCBF_CLOSE_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(CLOSE);
> - if (flags & TDCBF_CANCEL_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(CANCEL);
> - if (flags & TDCBF_RETRY_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(RETRY);
> - if (flags & TDCBF_NO_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(NO);
> - if (flags & TDCBF_YES_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(YES);
> - /* Allocate enough memory for the custom and the default buttons */
> - if (taskconfig->cButtons && taskconfig->pButtons)
> -
>    buttons = HeapAlloc(GetProcessHeap(), 0, (taskconfig->cButtons + 6) * sizeof(button_info));
> - else
> - {
> -
>    buttons = HeapAlloc(GetProcessHeap(), 0, 6 * sizeof(button_info));
> - }
> -
> - /* Custom buttons */
> - if (taskconfig->cButtons && taskconfig->pButtons)
> - {
> -
>    for (i = 0; i < taskconfig->cButtons; i++)
> -
>    {
> -
>        buttons[count++] = make_button(desc, taskconfig->pButtons[i].nButtonID, taskconfig->pButtons[i].pszButtonText);
> -
>    }
> - }
> -
> - /* Default buttons */
> if (flags & TDCBF_OK_BUTTON)
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> - /* Always add OK button */
> - if (list_empty(&desc->controls))
> -
>    TASKDIALOG_ADD_COMMON_BUTTON(OK);
>
> -#undef TASKDIALOG_ADD_COMMON_BUTTON
> -
> - /* make room for common buttons row */
> - desc->dialog_height +=  DIALOG_BUTTON_HEIGHT + 2 * DIALOG_SPACING;
> -
>    buttons[count++] = make_button(desc, IDOK,     MAKEINTRESOURCEW(IDS_BUTTON_OK));
> - if (flags & TDCBF_YES_BUTTON)
> -
>    buttons[count++] = make_button(desc, IDYES,    MAKEINTRESOURCEW(IDS_BUTTON_YES));
> - if (flags & TDCBF_NO_BUTTON)
> -
>    buttons[count++] = make_button(desc, IDNO,     MAKEINTRESOURCEW(IDS_BUTTON_NO));
> - if (flags & TDCBF_RETRY_BUTTON)
> -
>    buttons[count++] = make_button(desc, IDRETRY,  MAKEINTRESOURCEW(IDS_BUTTON_RETRY));
> - if (flags & TDCBF_CANCEL_BUTTON)
> -
>    buttons[count++] = make_button(desc, IDCANCEL, MAKEINTRESOURCEW(IDS_BUTTON_CANCEL));
> - if (flags & TDCBF_CLOSE_BUTTON)
> -
>    buttons[count++] = make_button(desc, IDCLOSE,  MAKEINTRESOURCEW(IDS_BUTTON_CLOSE));
> -
> - /* There must be at least one button */
> - if (count == 0)
> -
>    buttons[count++] = make_button(desc, IDOK, MAKEINTRESOURCEW(IDS_BUTTON_OK));
> -
> - /* Position buttons */
> - location_x = alignment;
> - for (i = 0; i < count; i++)
> - {
> -
>    /* When beginning new row, align the first */
> -
>    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W > desc->dialog_width)
> -
>    {
> -
>        if (first_row)
> -
>        {
> -
>            int diff = desc->dialog_width - location_x;
> -
> -
>            first_row = FALSE;
> -
>            for (int j = 0; j < i; j++) /* Align first row to the right */
> -
>                buttons[j].x += diff;
> -
>            alignment = buttons[0].x; /* left-align all coming rows to the first row */
> -
>        }
> -
>        location_x = alignment;
> -
>        desc->dialog_height += DIALOG_BUTTON_HEIGHT + DIALOG_SPACING_BUTTON_H;
> -
>    }
> -
> -
>    buttons[i].x = location_x;
> -
>    buttons[i].y = desc->dialog_height;
> -
> -
>    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> - }
> - if (first_row) /* Always align first row to the right */
> - {
> -
>    int diff = desc->dialog_width - (buttons[count - 1].x + buttons[count - 1].width + DIALOG_SPACING_BUTTON_W);
> -
>    for (int i = 0; i < count; i++)
> -
>        buttons[i].x += diff;
> - }

I don't like this. Why do you need temporary array, and iterating twice
seems excessive.
> -
> -
> /* Now that we got them all positioned, create all buttons */
> - for (i = 0; i < count; i++)
> - {
> -
>    size += taskdialog_add_control(desc, buttons[i].id, WC_BUTTONW, BS_PUSHBUTTON, COMCTL32_hModule,
> -
>                buttons[i].text, buttons[i].x, buttons[i].y, buttons[i].width, DIALOG_BUTTON_HEIGHT);
> - }

This won't work for custom buttons.
> -
> - /* Add height for last row and spacing */
> - desc->dialog_height +=  DIALOG_BUTTON_HEIGHT + DIALOG_SPACING;
> -
> - HeapFree(GetProcessHeap(), 0, buttons);
> return size;
>
> }
>
> @@ -295,7 +393,7 @@ static DLGTEMPLATE *create_taskdialog_template(const TASKDIALOGCONFIG *taskconfi
>      desc.dialog_width = min(desc.dialog_width, screen_width);
>  size += taskdialog_add_main_instruction(&desc);
> - size += taskdialog_add_common_buttons(&desc);
> - size += taskdialog_add_buttons(&desc);

I'd rather have it separated if possible. I'll do some testing myself,
before commenting further.
>  template = Alloc(size);
>  if (!template)

[PATCH] comctl32/taskdialog: Implement buttons.

[Fabian Maurer]

> > -static const UINT DIALOG_MIN_WIDTH = 180;
> > +static const UINT DIALOG_MIN_WIDTH = 240;
> >
> > static const UINT DIALOG_SPACING = 5;
> >
> > -static const UINT DIALOG_BUTTON_WIDTH = 50;
> > -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> > +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> > +static const UINT DIALOG_BUTTON_HEIGHT = 13;
>
> This was taken from MSGBOX template, what's a point in making it different?

I just tested it on my Win7 box and tried to get it to look as similar as 
possible. Where do the MSGBOX template values come from? I can remove that 
change, if those values are better.
> Did you test that buttons are supposed to support multiple lines of text?

Normal buttons don't allow multiple lines of text. But we only use the right 
side of the rectangle anyways, so it works as expected. Problem is, on windows 
and on wine the buttons clips the text when it gets too long, so I'm not fully 
it's the correct way to implement this. Should I change this?
There's also the fact that buttons get larger in height on windows, even if 
the text is a single line only. Should I reproduce this?
Moreover, wine displays newlines as boxes while this isn't the case on 
windows. Not sure how to deal with this though.
> > 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) +        DWORD style, HINSTANCE hInstance, const WCHAR
> > *text, short x, short y, short cx, short cy)> 
> >  {
> >  struct taskdialog_control *control = Alloc(sizeof(*control));
> >  unsigned int size, class_size, text_size;
> >
> > @@ -117,7 +167,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 | WS_CHILD | style;
>
> You don't use styles in this patch, this is unnecessary change.

I'm using BS_PUSHBUTTON for the buttons, is this unnecessary?
> >  template->dwExtendedStyle = 0;
> >  template->x = x;
> >  template->y = y;
> >
> > @@ -136,78 +186,126 @@ static unsigned int taskdialog_add_control(struct
> > taskdialog_template_desc *desc> 
> >  static unsigned int taskdialog_add_main_instruction(struct
> >  taskdialog_template_desc *desc) {
> > - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /*
> >
> > padding left and right of the control */ -    const WCHAR *textW = NULL;
> > - unsigned int size, length;
> > - HFONT oldfont;
> > - HDC hdc;
> > -
> >
> > unsigned int size;
> > -
> >
> > RECT rect;
> >
> > if (!desc->taskconfig->pszMainInstruction)
> > return 0;
> >
> > - if (IS_INTRESOURCE(desc->taskconfig->pszMainInstruction))
> > - {
> > -
> >    if (!(length = LoadStringW(desc->taskconfig->hInstance,
> >
> > (UINT_PTR)desc->taskconfig->pszMainInstruction, -                (WCHAR
> > *)&textW, 0)))
> > -
> >    {
> > -
> >        WARN("Failed to load main instruction text\n");
> > -
> >        return 0;
> > -
> >    }
> > - }
> > - else
> > - {
> > -
> >    textW = desc->taskconfig->pszMainInstruction;
> > -
> >    length = strlenW(textW);
> > - }
> > -
> > - hdc = GetDC(0);
> > - oldfont = SelectObject(hdc, desc->font);
> > -
> > - dialogunits_to_pixels(desc, &rect.right, NULL);
> > - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS |
> >
> > DT_CALCRECT | DT_WORDBREAK); -    pixels_to_dialogunits(desc,
> > &rect.right, &rect.bottom);
> > -
> > - SelectObject(hdc, oldfont);
> > - ReleaseDC(0, hdc);
> > - rect = text_get_rect(desc, desc->taskconfig->pszMainInstruction);
> > - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION, WC_STATICW,
> >
> > desc->taskconfig->hInstance, +    size = taskdialog_add_control(desc,
> > ID_MAIN_INSTRUCTION, WC_STATICW, 0, desc->taskconfig->hInstance,> 
> >              desc->taskconfig->pszMainInstruction, DIALOG_SPACING,
> >              desc->dialog_height,
> >              rect.right, rect.bottom);
> > - desc->dialog_height += rect.bottom;
> > -
> >
> > desc->dialog_height += rect.bottom + DIALOG_SPACING;
> >
> > return size;
> >
> > }
> >
> > -static unsigned int taskdialog_add_common_buttons(struct
> > taskdialog_template_desc *desc) +static button_info make_button(struct
> > taskdialog_template_desc *desc, int id, const WCHAR *text) +{
> > - RECT rect;
> > - button_info button;
> > -
> > - button.id = id;
> > - button.text = text;
> > - rect = text_get_rect(desc, text);
> > - button.width = rect.right + 10;
> > - if (button.width < DIALOG_BUTTON_WIDTH_MIN)
> > -
> >    button.width = DIALOG_BUTTON_WIDTH_MIN;
> > -
> > - return button;
> >
> > +}
>
> This does not handle "text" correctly.

Sorry, but I don't see the problem here. Do you mind explaining?
In both text_get_rect and later on taskdialog_add_control both text or a 
resource-ID are allowed.
> > +static unsigned int taskdialog_add_buttons(struct
> > taskdialog_template_desc *desc)> 
> >  {
> > - short button_x = desc->dialog_width - DIALOG_BUTTON_WIDTH -
> >
> > DIALOG_SPACING;> 
> >      DWORD flags = desc->taskconfig->dwCommonButtons;
> > - const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
> > - UINT alignment = DIALOG_SPACING_BUTTONS_LEFT; /* minimum distance
> >
> > from the left dialog border */ +    UINT location_x;
> > -
> >
> > BOOL first_row = TRUE;
> > -
> >
> > button_info *buttons;
> > -
> >
> > int count = 0;
> > -
> >
> > int i;
> >
> > unsigned int size = 0;
> >
> > -#define TASKDIALOG_ADD_COMMON_BUTTON(id) \
> > - do { \
> > -
> >    size += taskdialog_add_control(desc, ID##id, WC_BUTTONW,
> >
> > COMCTL32_hModule, MAKEINTRESOURCEW(IDS_BUTTON_##id), \ -           
> > button_x, desc->dialog_height + DIALOG_SPACING, DIALOG_BUTTON_WIDTH,
> > DIALOG_BUTTON_HEIGHT); \ -        button_x -= DIALOG_BUTTON_WIDTH +
> > DIALOG_SPACING; \
> > - } while(0)
> > - if (flags & TDCBF_CLOSE_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(CLOSE);
> > - if (flags & TDCBF_CANCEL_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(CANCEL);
> > - if (flags & TDCBF_RETRY_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(RETRY);
> > - if (flags & TDCBF_NO_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(NO);
> > - if (flags & TDCBF_YES_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(YES);
> > - /* Allocate enough memory for the custom and the default buttons */
> > - if (taskconfig->cButtons && taskconfig->pButtons)
> > -
> >    buttons = HeapAlloc(GetProcessHeap(), 0, (taskconfig->cButtons +
> > -
> > - sizeof(button_info)); +    else
> > - {
> > -
> >    buttons = HeapAlloc(GetProcessHeap(), 0, 6 *
> >
> > sizeof(button_info));
> > - }
> > -
> > - /* Custom buttons */
> > - if (taskconfig->cButtons && taskconfig->pButtons)
> > - {
> > -
> >    for (i = 0; i < taskconfig->cButtons; i++)
> > -
> >    {
> > -
> >        buttons[count++] = make_button(desc,
> >
> > taskconfig->pButtons[i].nButtonID,
> > taskconfig->pButtons[i].pszButtonText); +        }
> > -
> >
> > }
> > -
> > -
> >
> > /* Default buttons */
> >
> > if (flags & TDCBF_OK_BUTTON)
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > - /* Always add OK button */
> > - if (list_empty(&desc->controls))
> > -
> >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> >
> > -#undef TASKDIALOG_ADD_COMMON_BUTTON
> > -
> > - /* make room for common buttons row */
> > - desc->dialog_height +=  DIALOG_BUTTON_HEIGHT + 2 * DIALOG_SPACING;
> > -
> >    buttons[count++] = make_button(desc, IDOK,    
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +    if (flags & TDCBF_YES_BUTTON)
> > -
> >    buttons[count++] = make_button(desc, IDYES,   
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_YES)); +    if (flags & TDCBF_NO_BUTTON)
> > -
> >    buttons[count++] = make_button(desc, IDNO,    
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_NO)); +    if (flags & TDCBF_RETRY_BUTTON)
> > -
> >    buttons[count++] = make_button(desc, IDRETRY, 
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_RETRY)); +    if (flags &
> > TDCBF_CANCEL_BUTTON)
> > -
> >    buttons[count++] = make_button(desc, IDCANCEL,
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_CANCEL)); +    if (flags &
> > TDCBF_CLOSE_BUTTON)
> > -
> >    buttons[count++] = make_button(desc, IDCLOSE, 
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_CLOSE)); +
> > - /* There must be at least one button */
> > - if (count == 0)
> > -
> >    buttons[count++] = make_button(desc, IDOK,
> >
> > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +
> > - /* Position buttons */
> > - location_x = alignment;
> > - for (i = 0; i < count; i++)
> > - {
> > -
> >    /* When beginning new row, align the first */
> > -
> >    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W >
> >
> > desc->dialog_width) +        {
> > -
> >        if (first_row)
> > -
> >        {
> > -
> >            int diff = desc->dialog_width - location_x;
> > -
> > -
> >            first_row = FALSE;
> > -
> >            for (int j = 0; j < i; j++) /* Align first row to the
> >
> > right */ +                    buttons[j].x += diff;
> > -
> >            alignment = buttons[0].x; /* left-align all coming rows
> >
> > to the first row */ +            }
> > -
> >        location_x = alignment;
> > -
> >        desc->dialog_height += DIALOG_BUTTON_HEIGHT +
> >
> > DIALOG_SPACING_BUTTON_H; +        }
> > -
> > -
> >    buttons[i].x = location_x;
> > -
> >    buttons[i].y = desc->dialog_height;
> > -
> > -
> >    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> > - }
> > - if (first_row) /* Always align first row to the right */
> > - {
> > -
> >    int diff = desc->dialog_width - (buttons[count - 1].x +
> >
> > buttons[count - 1].width + DIALOG_SPACING_BUTTON_W); +        for (int i
> > = 0; i < count; i++)
> > -
> >        buttons[i].x += diff;
> > - }
>
> I don't like this. Why do you need temporary array, and iterating twice
> seems excessive.

The problem is, a later button might influence the position of the first button. 
That's why I created them as temporary array and then change their position 
once I know how.
I also could iterate the loop twice I guess, but that sounded a bit ugly, too. 
If you have a better idea to address this problem, I'd be happy to hear it.
> > - for (i = 0; i < count; i++)
> > - {
> > -
> >    size += taskdialog_add_control(desc, buttons[i].id, WC_BUTTONW,
> >
> > BS_PUSHBUTTON, COMCTL32_hModule, +                    buttons[i].text,
> > buttons[i].x, buttons[i].y, buttons[i].width, DIALOG_BUTTON_HEIGHT); +   
> > }
>
> This won't work for custom buttons.

Right, I missed that, thanks for pointing it out.
> > - size += taskdialog_add_common_buttons(&desc);
> > - size += taskdialog_add_buttons(&desc);
>
> I'd rather have it separated if possible. I'll do some testing myself,
> before commenting further.

I see. Just figured it would make sense to handle all buttons in one function, 
since they are aligned together - if no command-links are used. Is there a 
reason you want it separated?

Regards,
Fabian Maurer

[PATCH] comctl32/taskdialog: Implement buttons.

[Fabian Maurer]

On Freitag, 24. März 2017 16:50:38 CEST Fabian Maurer wrote:
> > > -static const UINT DIALOG_MIN_WIDTH = 180;
> > > +static const UINT DIALOG_MIN_WIDTH = 240;
> > >
> > > static const UINT DIALOG_SPACING = 5;
> > >
> > > -static const UINT DIALOG_BUTTON_WIDTH = 50;
> > > -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> > > +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> > > +static const UINT DIALOG_BUTTON_HEIGHT = 13;
> >
> > This was taken from MSGBOX template, what's a point in making it
> > different?
>
> I just tested it on my Win7 box and tried to get it to look as similar as
> possible. Where do the MSGBOX template values come from? I can remove that
> change, if those values are better.
> > Did you test that buttons are supposed to support multiple lines of text?
>
> Normal buttons don't allow multiple lines of text. But we only use the right
> side of the rectangle anyways, so it works as expected. Problem is, on
> windows and on wine the buttons clips the text when it gets too long, so
> I'm not fully it's the correct way to implement this. Should I change this?
> There's also the fact that buttons get larger in height on windows, even if
> the text is a single line only. Should I reproduce this?
> Moreover, wine displays newlines as boxes while this isn't the case on
> windows. Not sure how to deal with this though.
> > > 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) +        DWORD style, HINSTANCE hInstance, const WCHAR
> > > *text, short x, short y, short cx, short cy)>
> > >
> > > {
> > >  struct taskdialog_control *control = Alloc(sizeof(*control));
> > >  unsigned int size, class_size, text_size;
> > >
> > > @@ -117,7 +167,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 | WS_CHILD | style;
> >
> > You don't use styles in this patch, this is unnecessary change.
>
> I'm using BS_PUSHBUTTON for the buttons, is this unnecessary?
> > >  template->dwExtendedStyle = 0;
> > >  template->x = x;
> > >  template->y = y;
> > >
> > > @@ -136,78 +186,126 @@ static unsigned int taskdialog_add_control(struct
> > > taskdialog_template_desc *desc>
> > >
> > > static unsigned int taskdialog_add_main_instruction(struct
> > >  taskdialog_template_desc *desc) {
> > > - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /*
> > >
> > > padding left and right of the control */ -    const WCHAR *textW = NULL;
> > > - unsigned int size, length;
> > > - HFONT oldfont;
> > > - HDC hdc;
> > > -
> > >
> > > unsigned int size;
> > > -
> > >
> > > RECT rect;
> > >
> > > if (!desc->taskconfig->pszMainInstruction)
> > > return 0;
> > >
> > > - if (IS_INTRESOURCE(desc->taskconfig->pszMainInstruction))
> > > - {
> > > -
> > >    if (!(length = LoadStringW(desc->taskconfig->hInstance,
> > >
> > > (UINT_PTR)desc->taskconfig->pszMainInstruction, -                (WCHAR
> > > *)&textW, 0)))
> > > -
> > >    {
> > > -
> > >        WARN("Failed to load main instruction text\n");
> > > -
> > >        return 0;
> > > -
> > >    }
> > > - }
> > > - else
> > > - {
> > > -
> > >    textW = desc->taskconfig->pszMainInstruction;
> > > -
> > >    length = strlenW(textW);
> > > - }
> > > -
> > > - hdc = GetDC(0);
> > > - oldfont = SelectObject(hdc, desc->font);
> > > -
> > > - dialogunits_to_pixels(desc, &rect.right, NULL);
> > > - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS |
> > >
> > > DT_CALCRECT | DT_WORDBREAK); -    pixels_to_dialogunits(desc,
> > > &rect.right, &rect.bottom);
> > > -
> > > - SelectObject(hdc, oldfont);
> > > - ReleaseDC(0, hdc);
> > > - rect = text_get_rect(desc, desc->taskconfig->pszMainInstruction);
> > > - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION,
> > >
> > > WC_STATICW,
> > > desc->taskconfig->hInstance, +    size = taskdialog_add_control(desc,
> > > ID_MAIN_INSTRUCTION, WC_STATICW, 0, desc->taskconfig->hInstance,>
> > >          desc->taskconfig->pszMainInstruction, DIALOG_SPACING,
> > >          desc->dialog_height,
> > >          rect.right, rect.bottom);
> > >
> > > - desc->dialog_height += rect.bottom;
> > > -
> > >
> > > desc->dialog_height += rect.bottom + DIALOG_SPACING;
> > >
> > > return size;
> > >
> > > }
> > >
> > > -static unsigned int taskdialog_add_common_buttons(struct
> > > taskdialog_template_desc *desc) +static button_info make_button(struct
> > > taskdialog_template_desc *desc, int id, const WCHAR *text) +{
> > > - RECT rect;
> > > - button_info button;
> > > -
> > > - button.id = id;
> > > - button.text = text;
> > > - rect = text_get_rect(desc, text);
> > > - button.width = rect.right + 10;
> > > - if (button.width < DIALOG_BUTTON_WIDTH_MIN)
> > > -
> > >    button.width = DIALOG_BUTTON_WIDTH_MIN;
> > > -
> > > - return button;
> > >
> > > +}
> >
> > This does not handle "text" correctly.
>
> Sorry, but I don't see the problem here. Do you mind explaining?
> In both text_get_rect and later on taskdialog_add_control both text or a
> resource-ID are allowed.
> > > +static unsigned int taskdialog_add_buttons(struct
> > > taskdialog_template_desc *desc)>
> > >
> > > {
> > > - short button_x = desc->dialog_width - DIALOG_BUTTON_WIDTH -
> > >
> > > DIALOG_SPACING;>
> > >  DWORD flags = desc->taskconfig->dwCommonButtons;
> > >
> > > - const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
> > > - UINT alignment = DIALOG_SPACING_BUTTONS_LEFT; /* minimum distance
> > >
> > > from the left dialog border */ +    UINT location_x;
> > > -
> > >
> > > BOOL first_row = TRUE;
> > > -
> > >
> > > button_info *buttons;
> > > -
> > >
> > > int count = 0;
> > > -
> > >
> > > int i;
> > >
> > > unsigned int size = 0;
> > >
> > > -#define TASKDIALOG_ADD_COMMON_BUTTON(id) \
> > > - do { \
> > > -
> > >    size += taskdialog_add_control(desc, ID##id, WC_BUTTONW,
> > >
> > > COMCTL32_hModule, MAKEINTRESOURCEW(IDS_BUTTON_##id), \ -
> > > button_x, desc->dialog_height + DIALOG_SPACING, DIALOG_BUTTON_WIDTH,
> > > DIALOG_BUTTON_HEIGHT); \ -        button_x -= DIALOG_BUTTON_WIDTH +
> > > DIALOG_SPACING; \
> > > - } while(0)
> > > - if (flags & TDCBF_CLOSE_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(CLOSE);
> > > - if (flags & TDCBF_CANCEL_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(CANCEL);
> > > - if (flags & TDCBF_RETRY_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(RETRY);
> > > - if (flags & TDCBF_NO_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(NO);
> > > - if (flags & TDCBF_YES_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(YES);
> > > - /* Allocate enough memory for the custom and the default buttons */
> > > - if (taskconfig->cButtons && taskconfig->pButtons)
> > > -
> > >    buttons = HeapAlloc(GetProcessHeap(), 0, (taskconfig->cButtons
> > > -
> > > -
> > > - sizeof(button_info)); +    else
> > > - {
> > > -
> > >    buttons = HeapAlloc(GetProcessHeap(), 0, 6 *
> > >
> > > sizeof(button_info));
> > > - }
> > > -
> > > - /* Custom buttons */
> > > - if (taskconfig->cButtons && taskconfig->pButtons)
> > > - {
> > > -
> > >    for (i = 0; i < taskconfig->cButtons; i++)
> > > -
> > >    {
> > > -
> > >        buttons[count++] = make_button(desc,
> > >
> > > taskconfig->pButtons[i].nButtonID,
> > > taskconfig->pButtons[i].pszButtonText); +        }
> > > -
> > >
> > > }
> > > -
> > > -
> > >
> > > /* Default buttons */
> > >
> > > if (flags & TDCBF_OK_BUTTON)
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > > - /* Always add OK button */
> > > - if (list_empty(&desc->controls))
> > > -
> > >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > >
> > > -#undef TASKDIALOG_ADD_COMMON_BUTTON
> > > -
> > > - /* make room for common buttons row */
> > > - desc->dialog_height +=  DIALOG_BUTTON_HEIGHT + 2 * DIALOG_SPACING;
> > > -
> > >    buttons[count++] = make_button(desc, IDOK,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +    if (flags & TDCBF_YES_BUTTON)
> > > -
> > >    buttons[count++] = make_button(desc, IDYES,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_YES)); +    if (flags & TDCBF_NO_BUTTON)
> > > -
> > >    buttons[count++] = make_button(desc, IDNO,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_NO)); +    if (flags & TDCBF_RETRY_BUTTON)
> > > -
> > >    buttons[count++] = make_button(desc, IDRETRY,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_RETRY)); +    if (flags &
> > > TDCBF_CANCEL_BUTTON)
> > > -
> > >    buttons[count++] = make_button(desc, IDCANCEL,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_CANCEL)); +    if (flags &
> > > TDCBF_CLOSE_BUTTON)
> > > -
> > >    buttons[count++] = make_button(desc, IDCLOSE,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_CLOSE)); +
> > > - /* There must be at least one button */
> > > - if (count == 0)
> > > -
> > >    buttons[count++] = make_button(desc, IDOK,
> > >
> > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +
> > > - /* Position buttons */
> > > - location_x = alignment;
> > > - for (i = 0; i < count; i++)
> > > - {
> > > -
> > >    /* When beginning new row, align the first */
> > > -
> > >    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W >
> > >
> > > desc->dialog_width) +        {
> > > -
> > >        if (first_row)
> > > -
> > >        {
> > > -
> > >            int diff = desc->dialog_width - location_x;
> > > -
> > > -
> > >            first_row = FALSE;
> > > -
> > >            for (int j = 0; j < i; j++) /* Align first row to the
> > >
> > > right */ +                    buttons[j].x += diff;
> > > -
> > >            alignment = buttons[0].x; /* left-align all coming rows
> > >
> > > to the first row */ +            }
> > > -
> > >        location_x = alignment;
> > > -
> > >        desc->dialog_height += DIALOG_BUTTON_HEIGHT +
> > >
> > > DIALOG_SPACING_BUTTON_H; +        }
> > > -
> > > -
> > >    buttons[i].x = location_x;
> > > -
> > >    buttons[i].y = desc->dialog_height;
> > > -
> > > -
> > >    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> > > - }
> > > - if (first_row) /* Always align first row to the right */
> > > - {
> > > -
> > >    int diff = desc->dialog_width - (buttons[count - 1].x +
> > >
> > > buttons[count - 1].width + DIALOG_SPACING_BUTTON_W); +        for (int i
> > > = 0; i < count; i++)
> > > -
> > >        buttons[i].x += diff;
> > > - }
> >
> > I don't like this. Why do you need temporary array, and iterating twice
> > seems excessive.
>
> The problem is, a later button might influence the position of the first
> button. That's why I created them as temporary array and then change their
> position once I know how.
> I also could iterate the loop twice I guess, but that sounded a bit ugly,
> too. If you have a better idea to address this problem, I'd be happy to
> hear it.
> > > - for (i = 0; i < count; i++)
> > > - {
> > > -
> > >    size += taskdialog_add_control(desc, buttons[i].id, WC_BUTTONW,
> > >
> > > BS_PUSHBUTTON, COMCTL32_hModule, +                    buttons[i].text,
> > > buttons[i].x, buttons[i].y, buttons[i].width, DIALOG_BUTTON_HEIGHT); +
> > > }
> >
> > This won't work for custom buttons.
>
> Right, I missed that, thanks for pointing it out.
> > > - size += taskdialog_add_common_buttons(&desc);
> > > - size += taskdialog_add_buttons(&desc);
> >
> > I'd rather have it separated if possible. I'll do some testing myself,
> > before commenting further.
>
> I see. Just figured it would make sense to handle all buttons in one
> function, since they are aligned together - if no command-links are used.
> Is there a reason you want it separated?
>
> Regards,
> Fabian Maurer

Any update on this?

[PATCH] comctl32/taskdialog: Implement buttons.

[Nikolay Sivov]

On 03.04.2017 13:53, Fabian Maurer wrote:
> On Freitag, 24. März 2017 16:50:38 CEST Fabian Maurer wrote:
> > > > -static const UINT DIALOG_MIN_WIDTH = 180;
> > > > +static const UINT DIALOG_MIN_WIDTH = 240;
> > > >
> > > > static const UINT DIALOG_SPACING = 5;
> > > >
> > > > -static const UINT DIALOG_BUTTON_WIDTH = 50;
> > > > -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> > > > +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> > > > +static const UINT DIALOG_BUTTON_HEIGHT = 13;
> > >
> > > This was taken from MSGBOX template, what's a point in making it
> > > different?
> >
> > I just tested it on my Win7 box and tried to get it to look as similar as
> > possible. Where do the MSGBOX template values come from? I can remove that
> > change, if those values are better.
> > > Did you test that buttons are supposed to support multiple lines of text?
> >
> > Normal buttons don't allow multiple lines of text. But we only use the right
> > side of the rectangle anyways, so it works as expected. Problem is, on
> > windows and on wine the buttons clips the text when it gets too long, so
> > I'm not fully it's the correct way to implement this. Should I change this?
> > There's also the fact that buttons get larger in height on windows, even if
> > the text is a single line only. Should I reproduce this?
> > Moreover, wine displays newlines as boxes while this isn't the case on
> > windows. Not sure how to deal with this though.
> > > > 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) +        DWORD style, HINSTANCE hInstance, const WCHAR
> > > > *text, short x, short y, short cx, short cy)>
> > > >
> > > > {
> > > >  struct taskdialog_control *control = Alloc(sizeof(*control));
> > > >  unsigned int size, class_size, text_size;
> > > >
> > > > @@ -117,7 +167,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 | WS_CHILD | style;
> > >
> > > You don't use styles in this patch, this is unnecessary change.
> >
> > I'm using BS_PUSHBUTTON for the buttons, is this unnecessary?
> > > >  template->dwExtendedStyle = 0;
> > > >  template->x = x;
> > > >  template->y = y;
> > > >
> > > > @@ -136,78 +186,126 @@ static unsigned int taskdialog_add_control(struct
> > > > taskdialog_template_desc *desc>
> > > >
> > > > static unsigned int taskdialog_add_main_instruction(struct
> > > >  taskdialog_template_desc *desc) {
> > > > - RECT rect = { 0, 0, desc->dialog_width - DIALOG_SPACING * 2, 0}; /*
> > > >
> > > > padding left and right of the control */ -    const WCHAR *textW = NULL;
> > > > - unsigned int size, length;
> > > > - HFONT oldfont;
> > > > - HDC hdc;
> > > > -
> > > >
> > > > unsigned int size;
> > > > -
> > > >
> > > > RECT rect;
> > > >
> > > > if (!desc->taskconfig->pszMainInstruction)
> > > > return 0;
> > > >
> > > > - if (IS_INTRESOURCE(desc->taskconfig->pszMainInstruction))
> > > > - {
> > > > -
> > > >    if (!(length = LoadStringW(desc->taskconfig->hInstance,
> > > >
> > > > (UINT_PTR)desc->taskconfig->pszMainInstruction, -                (WCHAR
> > > > *)&textW, 0)))
> > > > -
> > > >    {
> > > > -
> > > >        WARN("Failed to load main instruction text\n");
> > > > -
> > > >        return 0;
> > > > -
> > > >    }
> > > > - }
> > > > - else
> > > > - {
> > > > -
> > > >    textW = desc->taskconfig->pszMainInstruction;
> > > > -
> > > >    length = strlenW(textW);
> > > > - }
> > > > -
> > > > - hdc = GetDC(0);
> > > > - oldfont = SelectObject(hdc, desc->font);
> > > > -
> > > > - dialogunits_to_pixels(desc, &rect.right, NULL);
> > > > - DrawTextW(hdc, textW, length, &rect, DT_LEFT | DT_EXPANDTABS |
> > > >
> > > > DT_CALCRECT | DT_WORDBREAK); -    pixels_to_dialogunits(desc,
> > > > &rect.right, &rect.bottom);
> > > > -
> > > > - SelectObject(hdc, oldfont);
> > > > - ReleaseDC(0, hdc);
> > > > - rect = text_get_rect(desc, desc->taskconfig->pszMainInstruction);
> > > > - size = taskdialog_add_control(desc, ID_MAIN_INSTRUCTION,
> > > >
> > > > WC_STATICW,
> > > > desc->taskconfig->hInstance, +    size = taskdialog_add_control(desc,
> > > > ID_MAIN_INSTRUCTION, WC_STATICW, 0, desc->taskconfig->hInstance,>
> > > >          desc->taskconfig->pszMainInstruction, DIALOG_SPACING,
> > > >          desc->dialog_height,
> > > >          rect.right, rect.bottom);
> > > >
> > > > - desc->dialog_height += rect.bottom;
> > > > -
> > > >
> > > > desc->dialog_height += rect.bottom + DIALOG_SPACING;
> > > >
> > > > return size;
> > > >
> > > > }
> > > >
> > > > -static unsigned int taskdialog_add_common_buttons(struct
> > > > taskdialog_template_desc *desc) +static button_info make_button(struct
> > > > taskdialog_template_desc *desc, int id, const WCHAR *text) +{
> > > > - RECT rect;
> > > > - button_info button;
> > > > -
> > > > - button.id = id;
> > > > - button.text = text;
> > > > - rect = text_get_rect(desc, text);
> > > > - button.width = rect.right + 10;
> > > > - if (button.width < DIALOG_BUTTON_WIDTH_MIN)
> > > > -
> > > >    button.width = DIALOG_BUTTON_WIDTH_MIN;
> > > > -
> > > > - return button;
> > > >
> > > > +}
> > >
> > > This does not handle "text" correctly.
> >
> > Sorry, but I don't see the problem here. Do you mind explaining?
> > In both text_get_rect and later on taskdialog_add_control both text or a
> > resource-ID are allowed.
> > > > +static unsigned int taskdialog_add_buttons(struct
> > > > taskdialog_template_desc *desc)>
> > > >
> > > > {
> > > > - short button_x = desc->dialog_width - DIALOG_BUTTON_WIDTH -
> > > >
> > > > DIALOG_SPACING;>
> > > >  DWORD flags = desc->taskconfig->dwCommonButtons;
> > > >
> > > > - const TASKDIALOGCONFIG *taskconfig = desc->taskconfig;
> > > > - UINT alignment = DIALOG_SPACING_BUTTONS_LEFT; /* minimum distance
> > > >
> > > > from the left dialog border */ +    UINT location_x;
> > > > -
> > > >
> > > > BOOL first_row = TRUE;
> > > > -
> > > >
> > > > button_info *buttons;
> > > > -
> > > >
> > > > int count = 0;
> > > > -
> > > >
> > > > int i;
> > > >
> > > > unsigned int size = 0;
> > > >
> > > > -#define TASKDIALOG_ADD_COMMON_BUTTON(id) \
> > > > - do { \
> > > > -
> > > >    size += taskdialog_add_control(desc, ID##id, WC_BUTTONW,
> > > >
> > > > COMCTL32_hModule, MAKEINTRESOURCEW(IDS_BUTTON_##id), \ -
> > > > button_x, desc->dialog_height + DIALOG_SPACING, DIALOG_BUTTON_WIDTH,
> > > > DIALOG_BUTTON_HEIGHT); \ -        button_x -= DIALOG_BUTTON_WIDTH +
> > > > DIALOG_SPACING; \
> > > > - } while(0)
> > > > - if (flags & TDCBF_CLOSE_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(CLOSE);
> > > > - if (flags & TDCBF_CANCEL_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(CANCEL);
> > > > - if (flags & TDCBF_RETRY_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(RETRY);
> > > > - if (flags & TDCBF_NO_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(NO);
> > > > - if (flags & TDCBF_YES_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(YES);
> > > > - /* Allocate enough memory for the custom and the default buttons */
> > > > - if (taskconfig->cButtons && taskconfig->pButtons)
> > > > -
> > > >    buttons = HeapAlloc(GetProcessHeap(), 0, (taskconfig->cButtons
> > > > -
> > > > -
> > > > - sizeof(button_info)); +    else
> > > > - {
> > > > -
> > > >    buttons = HeapAlloc(GetProcessHeap(), 0, 6 *
> > > >
> > > > sizeof(button_info));
> > > > - }
> > > > -
> > > > - /* Custom buttons */
> > > > - if (taskconfig->cButtons && taskconfig->pButtons)
> > > > - {
> > > > -
> > > >    for (i = 0; i < taskconfig->cButtons; i++)
> > > > -
> > > >    {
> > > > -
> > > >        buttons[count++] = make_button(desc,
> > > >
> > > > taskconfig->pButtons[i].nButtonID,
> > > > taskconfig->pButtons[i].pszButtonText); +        }
> > > > -
> > > >
> > > > }
> > > > -
> > > > -
> > > >
> > > > /* Default buttons */
> > > >
> > > > if (flags & TDCBF_OK_BUTTON)
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > > > - /* Always add OK button */
> > > > - if (list_empty(&desc->controls))
> > > > -
> > > >    TASKDIALOG_ADD_COMMON_BUTTON(OK);
> > > >
> > > > -#undef TASKDIALOG_ADD_COMMON_BUTTON
> > > > -
> > > > - /* make room for common buttons row */
> > > > - desc->dialog_height +=  DIALOG_BUTTON_HEIGHT + 2 * DIALOG_SPACING;
> > > > -
> > > >    buttons[count++] = make_button(desc, IDOK,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +    if (flags & TDCBF_YES_BUTTON)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDYES,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_YES)); +    if (flags & TDCBF_NO_BUTTON)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDNO,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_NO)); +    if (flags & TDCBF_RETRY_BUTTON)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDRETRY,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_RETRY)); +    if (flags &
> > > > TDCBF_CANCEL_BUTTON)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDCANCEL,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_CANCEL)); +    if (flags &
> > > > TDCBF_CLOSE_BUTTON)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDCLOSE,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_CLOSE)); +
> > > > - /* There must be at least one button */
> > > > - if (count == 0)
> > > > -
> > > >    buttons[count++] = make_button(desc, IDOK,
> > > >
> > > > MAKEINTRESOURCEW(IDS_BUTTON_OK)); +
> > > > - /* Position buttons */
> > > > - location_x = alignment;
> > > > - for (i = 0; i < count; i++)
> > > > - {
> > > > -
> > > >    /* When beginning new row, align the first */
> > > > -
> > > >    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W >
> > > >
> > > > desc->dialog_width) +        {
> > > > -
> > > >        if (first_row)
> > > > -
> > > >        {
> > > > -
> > > >            int diff = desc->dialog_width - location_x;
> > > > -
> > > > -
> > > >            first_row = FALSE;
> > > > -
> > > >            for (int j = 0; j < i; j++) /* Align first row to the
> > > >
> > > > right */ +                    buttons[j].x += diff;
> > > > -
> > > >            alignment = buttons[0].x; /* left-align all coming rows
> > > >
> > > > to the first row */ +            }
> > > > -
> > > >        location_x = alignment;
> > > > -
> > > >        desc->dialog_height += DIALOG_BUTTON_HEIGHT +
> > > >
> > > > DIALOG_SPACING_BUTTON_H; +        }
> > > > -
> > > > -
> > > >    buttons[i].x = location_x;
> > > > -
> > > >    buttons[i].y = desc->dialog_height;
> > > > -
> > > > -
> > > >    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> > > > - }
> > > > - if (first_row) /* Always align first row to the right */
> > > > - {
> > > > -
> > > >    int diff = desc->dialog_width - (buttons[count - 1].x +
> > > >
> > > > buttons[count - 1].width + DIALOG_SPACING_BUTTON_W); +        for (int i
> > > > = 0; i < count; i++)
> > > > -
> > > >        buttons[i].x += diff;
> > > > - }
> > >
> > > I don't like this. Why do you need temporary array, and iterating twice
> > > seems excessive.
> >
> > The problem is, a later button might influence the position of the first
> > button. That's why I created them as temporary array and then change their
> > position once I know how.
> > I also could iterate the loop twice I guess, but that sounded a bit ugly,
> > too. If you have a better idea to address this problem, I'd be happy to
> > hear it.
> > > > - for (i = 0; i < count; i++)
> > > > - {
> > > > -
> > > >    size += taskdialog_add_control(desc, buttons[i].id, WC_BUTTONW,
> > > >
> > > > BS_PUSHBUTTON, COMCTL32_hModule, +                    buttons[i].text,
> > > > buttons[i].x, buttons[i].y, buttons[i].width, DIALOG_BUTTON_HEIGHT); +
> > > > }
> > >
> > > This won't work for custom buttons.
> >
> > Right, I missed that, thanks for pointing it out.
> > > > - size += taskdialog_add_common_buttons(&desc);
> > > > - size += taskdialog_add_buttons(&desc);
> > >
> > > I'd rather have it separated if possible. I'll do some testing myself,
> > > before commenting further.
> >
> > I see. Just figured it would make sense to handle all buttons in one
> > function, since they are aligned together - if no command-links are used.
> > Is there a reason you want it separated?
> >
> > Regards,
> > Fabian Maurer
>
> Any update on this?

In addition to issues I mentioned already, I don't think this works
properly (or at all) for custom buttons. What will happen if custom
button has common id, or several custom buttons share the same id?
>

[PATCH] comctl32/taskdialog: Implement buttons.

[Fabian Maurer]

> In addition to issues I mentioned already, I don't think this works
> properly (or at all) for custom buttons. What will happen if custom
> button has common id, or several custom buttons share the same id?

Then they both will return the same id. How would you distinguish them, if 
they have the same id? Of course I'd write tests to also cover this, but for a 
basic implementation corner cases like this are not my priority. First we need 
to get it working, then we can worry about corner cases.

I also commented on your other points, explaining my reasoning.

Regards,
Fabian Maurer

[PATCH] comctl32/taskdialog: Implement buttons.

[Nikolay Sivov]

On 03.04.2017 20:18, Fabian Maurer wrote:
> > In addition to issues I mentioned already, I don't think this works
> > properly (or at all) for custom buttons. What will happen if custom
> > button has common id, or several custom buttons share the same id?
>
> Then they both will return the same id. How would you distinguish them, if 
> they have the same id? Of course I'd write tests to also cover this, but for a 
> basic implementation corner cases like this are not my priority. First we need 
> to get it working, then we can worry about corner cases.

For some reason I thought duplicate ids are not permitted in user32
dialogs, that's not the case apparently.
> I also commented on your other points, explaining my reasoning.

I did some tests, so let's go over them again.
> -static const UINT DIALOG_MIN_WIDTH = 180;
> +static const UINT DIALOG_MIN_WIDTH = 240;

I'm not necessarily opposed, but this change does look related to button
support.
> -static const UINT DIALOG_BUTTON_WIDTH = 50;
> -static const UINT DIALOG_BUTTON_HEIGHT = 14;
> +static const UINT DIALOG_BUTTON_WIDTH_MIN = 45;
> +static const UINT DIALOG_BUTTON_HEIGHT = 13;

Let's keep it as is, for consistency with user32 template.
> +static const UINT DIALOG_SPACING_BUTTONS_LEFT = 30; /* minimum distance from the left dialog border */

I don't see this on Win10. We could as well use DIALOG_SPACING here too,
for both left and right spaces.
> +static const UINT DIALOG_SPACING_BUTTON_H = 1; /* Distance between buttons */
> +static const UINT DIALOG_SPACING_BUTTON_W = 4; /* Distance between buttons */

It's tempting to use DIALOG_SPACING here as well to reduce amount of magic.
> - if (IS_INTRESOURCE(text))
> - {
> -
>    if (!(length = LoadStringW(desc->taskconfig->hInstance, (UINT_PTR)text, (WCHAR *)&textW, 0)))
> -
>    {
> -
>        WARN("Failed to load text\n");
> -
>        textW = &nulW;
> -
>        length = 0;
> -
>    }
> - }
> - else
> - {
> -
>    textW = text;
> -
>    length = strlenW(textW);
> - }
> -

This doesn't handle NULL text properly. At least buttons can't have text
set to NULL.
> - template->style = WS_VISIBLE;
> - template->style = WS_VISIBLE | WS_CHILD | style;

This will make no difference for buttons, you can as well skip this change.
> - button.width = rect.right + 10;

Why 10?
> - /* Position buttons */
> - location_x = alignment;
> - for (i = 0; i < count; i++)
> - {
> -
>    /* When beginning new row, align the first */
> -
>    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W > desc->dialog_width)
> -
>    {
> -
>        if (first_row)
> -
>        {
> -
>            int diff = desc->dialog_width - location_x;
> -
> -
>            first_row = FALSE;
> -
>            for (int j = 0; j < i; j++) /* Align first row to the right */
> -
>                buttons[j].x += diff;
> -
>            alignment = buttons[0].x; /* left-align all coming rows to the first row */
> -
>        }
> -
>        location_x = alignment;
> -
>        desc->dialog_height += DIALOG_BUTTON_HEIGHT + DIALOG_SPACING_BUTTON_H;
> -
>    }
> -
> -
>    buttons[i].x = location_x;
> -
>    buttons[i].y = desc->dialog_height;
> -
> -
>    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> - }

This does not do the right thing. Having first row right-aligned is not
a goal. According to my manual testing all rows are left aligned, and
wrapped as soon as next button width exceeds dialog width, which by the
way is supposed to grow, if cxWidth is 0. We can ignore that for now I
guess.
> Regards,
> Fabian Maurer

[PATCH] comctl32/taskdialog: Implement buttons.

[Fabian Maurer]

Thanks for your comments, I fixed most of the issues you pointed out.
> > +static const UINT DIALOG_SPACING_BUTTON_H = 1; /* Distance between
> > buttons */ +static const UINT DIALOG_SPACING_BUTTON_W = 4; /* Distance
> > between buttons */
>
> It's tempting to use DIALOG_SPACING here as well to reduce amount of magic.

Did that for DIALOG_SPACING_BUTTON_W, but I'd keep DIALOG_SPACING_BUTTON_H, 
since the vertical distance between buttons on my windows machine is only 1px, 
5px sounds like a bit much in that case.
> This doesn't handle NULL text properly. At least buttons can't have text
> set to NULL

Added a check for that before trying to create the template.
> > - button.width = rect.right + 10;
>
> Why 10?

Should be dialog-spacing times two, missed that constant.
> > - /* Position buttons */
> > - location_x = alignment;
> > - for (i = 0; i < count; i++)
> > - {
> > -
> >    /* When beginning new row, align the first */
> > -
> >    if (location_x + buttons[i].width + DIALOG_SPACING_BUTTON_W >
> >
> > desc->dialog_width) +        {
> > -
> >        if (first_row)
> > -
> >        {
> > -
> >            int diff = desc->dialog_width - location_x;
> > -
> > -
> >            first_row = FALSE;
> > -
> >            for (int j = 0; j < i; j++) /* Align first row to the
> >
> > right */ +                    buttons[j].x += diff;
> > -
> >            alignment = buttons[0].x; /* left-align all coming rows
> >
> > to the first row */ +            }
> > -
> >        location_x = alignment;
> > -
> >        desc->dialog_height += DIALOG_BUTTON_HEIGHT +
> >
> > DIALOG_SPACING_BUTTON_H; +        }
> > -
> > -
> >    buttons[i].x = location_x;
> > -
> >    buttons[i].y = desc->dialog_height;
> > -
> > -
> >    location_x += buttons[i].width + DIALOG_SPACING_BUTTON_W;
> > - }
>
> This does not do the right thing. Having first row right-aligned is not
> a goal. According to my manual testing all rows are left aligned, and
> wrapped as soon as next button width exceeds dialog width, which by the
> way is supposed to grow, if cxWidth is 0. We can ignore that for now I
> guess.

Can't confirm that on my machine. The buttons are right aligned, and if there 
is more than one row, the second is left aligned to the first one.
For example, if you have one button, it's always on the right. If you have two 
rows of 4 buttons, it's aligned right, too.
While my code doesn't take into account all corner cases, like one button 
that's bugger than all others, it gets the main look done.

Regards,
Fabian Maurer

[PATCH] comctl32/taskdialog: Implement buttons.

[Fabian Maurer]

> > It's tempting to use DIALOG_SPACING here as well to reduce amount of
> > magic.
>
> Did that for DIALOG_SPACING_BUTTON_W, but I'd keep DIALOG_SPACING_BUTTON_H,
> since the vertical distance between buttons on my windows machine is only
> 1px, 5px sounds like a bit much in that case.
> > This does not do the right thing. Having first row right-aligned is not
> > a goal. According to my manual testing all rows are left aligned, and
> > wrapped as soon as next button width exceeds dialog width, which by the
> > way is supposed to grow, if cxWidth is 0. We can ignore that for now I
> > guess.
>
> Can't confirm that on my machine. The buttons are right aligned, and if
> there is more than one row, the second is left aligned to the first one.
> For example, if you have one button, it's always on the right. If you have
> two rows of 4 buttons, it's aligned right, too.
> While my code doesn't take into account all corner cases, like one button
> that's bugger than all others, it gets the main look done.

Any comments on this?

Regards,
Fabian Maurer

[PATCH] comctl32/taskdialog: Implement buttons.

[Nikolay Sivov]

On 24.04.2017 4:07, Fabian Maurer wrote:
> > > It's tempting to use DIALOG_SPACING here as well to reduce amount of
> > > magic.
> >
> > Did that for DIALOG_SPACING_BUTTON_W, but I'd keep DIALOG_SPACING_BUTTON_H,
> > since the vertical distance between buttons on my windows machine is only
> > 1px, 5px sounds like a bit much in that case.
> > > This does not do the right thing. Having first row right-aligned is not
> > > a goal. According to my manual testing all rows are left aligned, and
> > > wrapped as soon as next button width exceeds dialog width, which by the
> > > way is supposed to grow, if cxWidth is 0. We can ignore that for now I
> > > guess.
> >
> > Can't confirm that on my machine. The buttons are right aligned, and if
> > there is more than one row, the second is left aligned to the first one.
> > For example, if you have one button, it's always on the right. If you have
> > two rows of 4 buttons, it's aligned right, too.
> > While my code doesn't take into account all corner cases, like one button
> > that's bugger than all others, it gets the main look done.
>
> Any comments on this?

If all buttons are of the same size you won't notice of course. Try with
buttons with different text lengths, I was able to see a difference
pretty easily. I just don't see why do this incorrectly.
> Regards,
> Fabian Maurer