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