Re: [3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box

[Nikolay Sivov]

On 19.02.2015 15:23, Joachim Priesner wrote:
> +        WCHAR *windowTitleBuffer = NULL;
> +        UINT len = LoadStringW(config->hInstance, LOWORD(config->pszWindowTitle), (LPWSTR)&ptr, 0);
> +        if (len)
> +        {
> +            windowTitleBuffer = Alloc((len + 1) * sizeof(WCHAR));
> +            if (!windowTitleBuffer)
> +            {
> +                EndDialog(hwnd, E_OUTOFMEMORY);
> +                return;
> +            }
> +            memcpy(windowTitleBuffer, ptr, len * sizeof(WCHAR));
> +            windowTitleBuffer[len] = 0;
> +            SetWindowTextW(hwnd, windowTitleBuffer);
> +        }

That's essentially the same code you're using to set IDC_CONTENT text. 
Could you add a helper that does all of that? Also it will probably take 
fewer lines to use LoadStringW() to fill your buffer and set terminating 
null.
> +    /* Destroy unused common buttons. */
> +    TRACE("dwCommonButtons=%x\n", config->dwCommonButtons);
> +    if (!(config->dwCommonButtons & TDCBF_YES_BUTTON))
> +        DestroyWindow(GetDlgItem(hwnd, IDYES));

Did you check that it's actually destroyed and not just hidden?
> +            WCHAR buttonText[1024];
> +            int w, h;
> +            if (GetWindowTextW(hItem, buttonText, 1024))

Where does this limit come from?
> +    /* Query parent window/desktop size and center window. */
> +    monitor = MonitorFromWindow(config->hwndParent ? config->hwndParent : GetActiveWindow(),
> +                                MONITOR_DEFAULTTOPRIMARY);
> +    monitorInfo.cbSize = sizeof(monitorInfo);
> +    GetMonitorInfoW(monitor, &monitorInfo);
> +    windowLeft = (monitorInfo.rcWork.left + monitorInfo.rcWork.right
> +                  - (windowClientWidth + windowBorderWidth)) / 2;
> +    windowTop = (monitorInfo.rcWork.top + monitorInfo.rcWork.bottom
> +                 - (windowClientHeight + windowBorderHeight)) / 2;
> +    SetWindowPos(hwnd, 0, windowLeft, windowTop,
> +                 windowClientWidth + windowBorderWidth, windowClientHeight + windowBorderHeight,
> +                 SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOREDRAW);

This looks overcomplicated, isn't it enough to use desktop window if no 
parent was specified?
> +    DeleteObject(mainInstructionFont);
> +    Free(contentBuffer);
> +    Free(mainInstructionBuffer);

It's better to free right when you don't need it anymore, and please use 
simpler/shorter names.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box

[Joachim Priesner]

Hi Nikolay,

thanks for the awesomely quick feedback. I'll submit a v2 of the patch soon.
> > +    /* Destroy unused common buttons. */
> > +    TRACE("dwCommonButtons=%x\n", config->dwCommonButtons);
> > +    if (!(config->dwCommonButtons & TDCBF_YES_BUTTON))
> > +        DestroyWindow(GetDlgItem(hwnd, IDYES));
>
> Did you check that it's actually destroyed and not just hidden?

You mean on Windows? Yes, the buttons are destroyed (or not created at all) there, not just hidden.
> > +            WCHAR buttonText[1024];
> > +            int w, h;
> > +            if (GetWindowTextW(hItem, buttonText, 1024))
>
> Where does this limit come from?

The code comes from user32/msgbox.c. I think it's supposed to be a safe limit to hold the text of any of the default buttons (Yes/No/Cancel/...) in any language.
> > +    /* Query parent window/desktop size and center window. */
> > +    monitor = MonitorFromWindow(config->hwndParent ? config->hwndParent : GetActiveWindow(),
> > +                                MONITOR_DEFAULTTOPRIMARY);
> > +    monitorInfo.cbSize = sizeof(monitorInfo);
> > +    GetMonitorInfoW(monitor, &monitorInfo);
> > +    windowLeft = (monitorInfo.rcWork.left + monitorInfo.rcWork.right
> > +                  - (windowClientWidth + windowBorderWidth)) / 2;
> > +    windowTop = (monitorInfo.rcWork.top + monitorInfo.rcWork.bottom
> > +                 - (windowClientHeight + windowBorderHeight)) / 2;
> > +    SetWindowPos(hwnd, 0, windowLeft, windowTop,
> > +                 windowClientWidth + windowBorderWidth, windowClientHeight + windowBorderHeight,
> > +                 SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOREDRAW);
>
> This looks overcomplicated, isn't it enough to use desktop window if no 
> parent was specified?

I don't know. This comes from user32/msgbox.c as well, and seems to work there :) Do you still want me to change it?
> > +    DeleteObject(mainInstructionFont);
> > +    Free(contentBuffer);
> > +    Free(mainInstructionBuffer);
>
> It's better to free right when you don't need it anymore, and please use 
> simpler/shorter names.

contentBuffer and mainInstructionBuffer are actually gone now. As for the other names, could you give an example of an shortening of e.g. "mainInstructionFont" that's acceptable to you? (I deliberately chose some longer variable names in favor of readability -- I don't want to chose "miFont" for example, because I want to keep consistency with the member names of TASKDIALOGCONFIG. The very short variable names in user32/msgbox.c bugged me very much.)

Joachim