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)