Re: [v2 PATCH] user32/msgbox: Support WM_COPY Message

[Nikolay Sivov]

On 08.11.2016 13:44, Alistair Leslie-Hughes wrote:
> Fixes: https://bugs.winehq.org/show_bug.cgi?id=17205
>
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
> ---
>  dlls/user32/msgbox.c       |  86 +++++++++++++++++++--
>  dlls/user32/tests/dialog.c | 187 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 267 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/user32/msgbox.c b/dlls/user32/msgbox.c
> index 2ba98c9..4735b8a 100644
> --- a/dlls/user32/msgbox.c
> +++ b/dlls/user32/msgbox.c
> @@ -44,6 +44,11 @@ struct ThreadWindows
>      HWND *handles;
>  };
>
> +/* Index the order the buttons need to appear to an ID* constant */
> +static const int buttonOrder[10] = { IDYES, IDNO, IDOK, IDABORT, IDRETRY,
> +                                 IDCANCEL, IDIGNORE, IDTRYAGAIN,
> +                                 IDCONTINUE, IDHELP };
> +
>  static BOOL CALLBACK MSGBOX_EnumProc(HWND hwnd, LPARAM lParam)
>  {
>      struct ThreadWindows *threadWindows = (struct ThreadWindows *)lParam;
> @@ -77,11 +82,6 @@ static void MSGBOX_OnInit(HWND hwnd, LPMSGBOXPARAMSW lpmb)
>      WCHAR *buffer = NULL;
>      const WCHAR *ptr;
>
> -    /* Index the order the buttons need to appear to an ID* constant */
> -    static const int buttonOrder[10] = { IDYES, IDNO, IDOK, IDABORT, IDRETRY,
> -                                         IDCANCEL, IDIGNORE, IDTRYAGAIN,
> -                                         IDCONTINUE, IDHELP };
> -
>      nclm.cbSize = sizeof(nclm);
>      SystemParametersInfoW (SPI_GETNONCLIENTMETRICS, 0, &nclm, 0);
>
> @@ -319,6 +319,77 @@ static void MSGBOX_OnInit(HWND hwnd, LPMSGBOXPARAMSW lpmb)
>      HeapFree( GetProcessHeap(), 0, buffer );
>  }
>
> +static void MSGBOX_CopyToClipboard( HWND hwnd )
> +{
> +    int i;
> +    static const WCHAR line[] = {'-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-',
> +                           '-','-','-','-','-','-','-','-','\r','\n', 0};
> +    static const WCHAR crlf[] = {'\r','\n', 0};
> +    static const WCHAR spaces[] = {' ',' ',' ', 0};
> +    int lenTitle = GetWindowTextLengthW(hwnd) + 1;
> +    int lenMsg = GetWindowTextLengthW(GetDlgItem(hwnd, MSGBOX_IDTEXT)) + 1;
> +
> +    /*
> +    ---------------------------
> +    Dialog Title
> +    ---------------------------
> +    Dialog Message
> +    ---------------------------
> +    Button(s) Text. OK
> +    ---------------------------
> +    */
> +    int len = ((sizeof(crlf) * 3) + (sizeof(line) * 4) + lenTitle + lenMsg) * sizeof(WCHAR);

This will allocate more than you need.
> +    WCHAR *text = HeapAlloc( GetProcessHeap(), 0,  len);

Maybe return right here if allocation failed? Also please use consistent
formatting regarding spaces.
> +    if(text)
> +    {
> +        lstrcpyW(text, line);
> +        if (GetWindowTextW(hwnd, text + lstrlenW(text), lenTitle))
> +        {
> +            HGLOBAL hMem;
> +            WCHAR *data;
> +
> +            lstrcatW(text, crlf);
> +            lstrcatW(text, line);
> +            GetWindowTextW(GetDlgItem(hwnd, MSGBOX_IDTEXT), text + lstrlenW(text), lenMsg);

There is GetDlgItemText() for that.
> +            lstrcatW(text, crlf);
> +            lstrcatW(text, line);
> +
> +            for (i = 0; i < (sizeof(buttonOrder) / sizeof(buttonOrder[0])); i++)
> +            {
> +                HWND hItem = GetDlgItem(hwnd, buttonOrder[i]);
> +                if (GetWindowLongW(hItem, GWL_STYLE) & WS_VISIBLE)
> +                {

Does IsWindowVisible() work here?
> +                    WCHAR buffer[1024] = {0};

This looks excessive for a button text string.
> case WM_COMMAND:
>      switch (LOWORD(wParam))
> diff --git a/dlls/user32/tests/dialog.c b/dlls/user32/tests/dialog.c
> index a37c678..0bb7dca 100644
> --- a/dlls/user32/tests/dialog.c
> +++ b/dlls/user32/tests/dialog.c
> @@ -40,6 +40,7 @@
>  #include "winbase.h"
>  #include "wingdi.h"
>  #include "winuser.h"
> +#include "winnls.h"
>
> #define MAXHWNDS 1024
>  static HWND hwnd [MAXHWNDS];
> @@ -1392,6 +1393,187 @@ static void test_MessageBoxFontTest(void)
>      DestroyWindow(hDlg);
>  }
>
> +static const char msgbox_title[] = "%5!z9ZXw*ia;57n/FGl.bCH,Su\"mfKN;foCqAU\'j6AmoJgAc_D:Z0A\'E6PF_O/w";

What's the idea behind such title? If you're trying to create a unique
window title, I don't think it has to be that unique.
> +static WCHAR expectedOK[] =
> +{
> +'-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','\r','\n',
> +'%','5','!','z','9','Z','X','w','*','i','a',';','5','7','n','/','F','G','l','.','b','C','H',',','S','u','"','m','f',
> +'K','N',';','f','o','C','q','A','U','\'','j','6','A','m','o','J','g','A','c','_','D',':','Z','0','A','\'','E','6','P',
> +'F','_','O','/','w','\r','\n',
> +'-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','\r','\n',
> +'M','e','s','s','a','g','e','\r','\n',
> +'-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','\r','\n',
> +'O','K',' ',' ',' ','\r','\n',
> +'-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','-','\r','\n', 0
> +};

Instead of expected strings you can as well duplicate your
implementation logic and generate expected result. This will help
testing with localized dialogs.
> +
> +DWORD WINAPI WorkerThread(void *param)

Please please use more specific name.
> +            {
> +                succeeded = lstrcmpW(expected, text) == 0;
> +                if(!succeeded)
> +                {
> +                    ok(0, "%s\n", wine_dbgstr_w(text));
> +                    ok(0, "%s\n", wine_dbgstr_w(expected));
> +                }
> +            }

Could this simply be ok(!lstrcmpW(), "..."); ?
> +
> +static void test_MessageBox_WM_COPY_Test(void)
> +{
> +    DWORD tid = 0;
> +
> +    non_english = (PRIMARYLANGID(GetUserDefaultLangID()) != LANG_ENGLISH);
> +    trace("non_english %d\n", non_english);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedOK, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_OK);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedOkCancel, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_OKCANCEL);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedAbortRetryIgnore, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_ABORTRETRYIGNORE);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedYesNo, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_YESNO);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedYesNoCancel, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_YESNOCANCEL);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedRetryCancel, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_RETRYCANCEL);
> +
> +    CreateThread(NULL, 0, WorkerThread, &expectedCancelTryContinue, 0, &tid);
> +    MessageBoxA(NULL, "Message", msgbox_title, MB_CANCELTRYCONTINUE);
> +}

This could be a loop over possible button values.