[PATCH v3] comctl32: Implement handling of EM_SETCUEBANNER/EM_GETCUEBANNER messages.

[Sergio Gómez Del Real]

Signed-off-by: Sergio Gómez Del Real 
---
 dlls/comctl32/edit.c       | 62 ++++++++++++++++++++++++++++++++++++++
 dlls/comctl32/tests/edit.c | 62 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
index f0180adfbe..2796e6e53c 100644
--- a/dlls/comctl32/edit.c
+++ b/dlls/comctl32/edit.c
@@ -131,6 +131,7 @@ typedef struct
    				   should be sent to the first parent. */
    HWND hwndListBox;		/* handle of ComboBox's listbox or NULL */
    INT wheelDeltaRemainder;        /* scroll wheel delta left over after scrolling whole lines */
+       WCHAR *cue_banner_text;
    /*
     *	only for multi line controls
     */
@@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, HDC dc, INT line, BOOL rev)
    	x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - e, FALSE);
    } else
    	x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
+
+       if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && es->text_length == 0)
+       {
+	       SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
+	       TextOutW(dc, x, y, es->cue_banner_text, strlenW(es->cue_banner_text));
+       }
 }

@@ -4152,6 +4159,52 @@ static LRESULT EDIT_EM_GetThumb(EDITSTATE *es)
                         EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
 }

+/*********************************************************************
+ *
+ *	EM_SETCUEBANNER
+ *
+ */
+static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR *cue_text)
+{
+    SIZE_T str_size;
+
+    if (es->style & ES_MULTILINE || !cue_text)
+        return FALSE;
+
+    str_size = strlenW(cue_text);
+
+    if (es->cue_banner_text)
+        heap_free(es->cue_banner_text);
+    
+    es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
+    if (!es->cue_banner_text)
+        return FALSE;
+
+    memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
+    es->cue_banner_text[str_size] = 0;
+
+    return TRUE;
+}
+
+/*********************************************************************
+ *
+ *	EM_GETCUEBANNER
+ *
+ */
+static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD size)
+{
+    if (!es->cue_banner_text)
+        return FALSE;
+    else
+    {
+        if (size > strlenW(es->cue_banner_text))
+            size = strlenW(es->cue_banner_text) + 1;
+        memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
+        buf[size-1] = 0;
+        return TRUE;
+    }
+}
+

/********************************************************************
  *
@@ -4489,6 +4542,7 @@ static LRESULT EDIT_WM_NCDestroy(EDITSTATE *es)

SetWindowLongPtrW( es->hwndSelf, 0, 0 );
     heap_free(es->undo_text);
+    heap_free(es->cue_banner_text);
     heap_free(es);

return 0;
@@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND hwnd, UINT msg, WPARAM wParam, LPAR
         result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), (short)HIWORD(lParam));
         break;

+    case EM_SETCUEBANNER:
+        result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
+        break;
+
+    case EM_GETCUEBANNER:
+        result = EDIT_EM_GetCueBanner(es, (WCHAR *)wParam, (DWORD)lParam);
+        break;
+
     /* End of the EM_ messages which were in numerical order; what order
      * are these in?  vaguely alphabetical?
      */
diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c
index a5097d1cbb..290517ec43 100644
--- a/dlls/comctl32/tests/edit.c
+++ b/dlls/comctl32/tests/edit.c
@@ -3061,6 +3061,67 @@ static const struct message killfocus_combined_seq[] =
     { 0 }
 };

+static void test_cue_banner(void)
+{
+    HWND hwnd_edit;
+    BOOL ret;
+    static WCHAR testW[] = {'T','e','s','t',0};
+    static WCHAR testcmp1W[] = {'T','e','s','t',0};
+    static WCHAR testcmp2W[] = {'T','e','s',0};
+    static WCHAR getcuetestW[5];
+    static WCHAR emptyW[] = {0};
+
+    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | ES_AUTOVSCROLL, 0);
+
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
+    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
+
+    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
+    ok(ret == FALSE, "EM_SETCUEBANNER should have returned FALSE.\n");
+
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
+    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
+
+    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
+    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
+
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, (WPARAM)getcuetestW, 5);
+    if (ret == FALSE)
+    {
+        win_skip("skipping for Win XP and 2003 Server.\n");
+        DestroyWindow(hwnd_edit);
+        return;
+    }
+    ok(ret == TRUE, "EM_GETCUEBANNER should have returned TRUE.\n");
+    ok(lstrcmpW(getcuetestW, testW) == 0, "EM_GETCUEBANNER returned string %s.\n", wine_dbgstr_w(getcuetestW));
+
+    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)emptyW);
+    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
+
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, (WPARAM)getcuetestW, 5);
+    ok(ret == TRUE, "EM_GETCUEBANNER should have returned TRUE.\n");
+    ok(lstrcmpW(getcuetestW, emptyW) == 0, "EM_GETCUEBANNER returned string %s.\n", wine_dbgstr_w(getcuetestW));
+
+    /* EM_GETCUEBANNER's buffer size includes null char */
+    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
+    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
+    memset(testW, 0, lstrlenW(testcmp1W)*sizeof(WCHAR));
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, (WPARAM)testW, (LPARAM)lstrlenW(testcmp1W)+1);
+    ok(ret == TRUE, "EM_GETCUEBANNER should have returned TRUE.\n");
+    ok(lstrcmpW(testW, testcmp1W) == 0, "EM_GETCUEBANNER returned string %s.\n", wine_dbgstr_w(testW));
+    memset(testW, 0, lstrlenW(testcmp1W)*sizeof(WCHAR));
+    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, (WPARAM)testW, (LPARAM)lstrlenW(testcmp1W));
+    ok(lstrcmpW(testW, testcmp2W) == 0, "EM_GETCUEBANNER returned string %s.\n", wine_dbgstr_w(testW));
+    DestroyWindow(hwnd_edit);
+
+    /* setting cue banner fails for multi-line edit controls */
+    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | ES_AUTOVSCROLL | ES_MULTILINE, 0);
+    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
+    ok(ret == FALSE, "EM_SETCUEBANNER.\n");
+
+    DestroyWindow(hwnd_edit);
+}
+
 static void test_change_focus(void)
 {
     HWND hwnd, parent_wnd;
@@ -3138,6 +3199,7 @@ START_TEST(edit)
     test_EM_GETLINE();
     test_wordbreak_proc();
     test_change_focus();
+    test_cue_banner();

UnregisterWindowClasses();

[Nikolay Sivov]

On 10/22/2018 09:24 PM, Sergio Gómez Del Real wrote:
> Signed-off-by: Sergio Gómez Del Real
>
> dlls/comctl32/edit.c       | 62 ++++++++++++++++++++++++++++++++++++++
>   dlls/comctl32/tests/edit.c | 62 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 124 insertions(+)
>
> diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> index f0180adfbe..2796e6e53c 100644
> --- a/dlls/comctl32/edit.c
> +++ b/dlls/comctl32/edit.c
> @@ -131,6 +131,7 @@ typedef struct
>    				   should be sent to the first parent. */
>    HWND hwndListBox;		/* handle of ComboBox's listbox or NULL */
>    INT wheelDeltaRemainder;        /* scroll wheel delta left over after scrolling whole lines */
> -
>   WCHAR *cue_banner_text;
> /*
> - only for multi line controls
> */
>
> @@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, HDC dc, INT line, BOOL rev)
>    	x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - e, FALSE);
>    } else
>    	x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
> -
> -
>   if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && es->text_length == 0)
> -
>   {
> -
>       SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
> -
>       TextOutW(dc, x, y, es->cue_banner_text, strlenW(es->cue_banner_text));
> -
>   }
> }

Now looking again, it seems it would be better to move this to WM_PAINT, 
because it does not have to be called for every line. Does this 
complicate (x,y) calculation a lot?
> @@ -4152,6 +4159,52 @@ static LRESULT EDIT_EM_GetThumb(EDITSTATE *es)
>                           EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
>   }
>
> +/*********************************************************************
> -
> -
> -
> - EM_SETCUEBANNER
> -
> -
> - */
>
> +static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR *cue_text)
> +{
> - SIZE_T str_size;
> -
> - if (es->style & ES_MULTILINE || !cue_text)
> -
>    return FALSE;
> -
> - str_size = strlenW(cue_text);
> -
> - if (es->cue_banner_text)
> -
>    heap_free(es->cue_banner_text);
> -
> - es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
> - if (!es->cue_banner_text)
> -
>    return FALSE;
> -
> - memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
> - es->cue_banner_text[str_size] = 0;
> -
> - return TRUE;
>
> +}

I think you can reuse heap_strdupW() from propsheet.c for that, first 
moving it to a header, and changing Alloc() -> heap_alloc(). No need to 
null check before releasing old text by the way.
> -
>
> +/*********************************************************************
> -
> -
> -
> - EM_GETCUEBANNER
> -
> -
> - */
>
> +static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD size)
> +{
> - if (!es->cue_banner_text)
> -
>    return FALSE;
> - else
> - {
> -
>    if (size > strlenW(es->cue_banner_text))
> -
>        size = strlenW(es->cue_banner_text) + 1;
> -
>    memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
> -
>    buf[size-1] = 0;
> -
>    return TRUE;
> - }
>
> +}
> -

Could you use lstrcpynW() for this? Also it seems inaccurate for null 
text case - for ES_MULTILINE it shouldn't touch output buffer, and for 
single line case it write empty string, that's according to my quick 
testing, we'll need some tests for that. Also is it supposed to crash on 
null destination?
> @@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND hwnd, UINT msg, WPARAM wParam, LPAR
>           result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), (short)HIWORD(lParam));
>           break;
> - case EM_SETCUEBANNER:
> -
>    result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
> -
>    break;

So does wParam work or not?
> +static void test_cue_banner(void)
> +{
> - HWND hwnd_edit;
> - BOOL ret;
> - static WCHAR testW[] = {'T','e','s','t',0};
> - static WCHAR testcmp1W[] = {'T','e','s','t',0};
> - static WCHAR testcmp2W[] = {'T','e','s',0};
> - static WCHAR getcuetestW[5];
> - static WCHAR emptyW[] = {0};
> -
> - hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | ES_AUTOVSCROLL, 0);
> -
> - ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> - ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> -
> - ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
> - ok(ret == FALSE, "EM_SETCUEBANNER should have returned FALSE.\n");
> -
> - ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> - ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> -
> - ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
> - ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
> -
> - ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, (WPARAM)getcuetestW, 5);
> - if (ret == FALSE)
> - {
> -
>    win_skip("skipping for Win XP and 2003 Server.\n");
> -
>    DestroyWindow(hwnd_edit);
> -
>    return;
> - }

Move this up please. You can test that returned buffer is not null 
terminated as indication of unimplemented message on XP. No reason to 
run other tests before this skip if it's not going to work.

[Sergio Gómez Del Real]

On 23/10/18 1:47 a. m., Nikolay Sivov wrote:
> On 10/22/2018 09:24 PM, Sergio Gómez Del Real wrote:
> > Signed-off-by: Sergio Gómez Del Real
> >
> > dlls/comctl32/edit.c       | 62 ++++++++++++++++++++++++++++++++++++++
> >   dlls/comctl32/tests/edit.c | 62 ++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 124 insertions(+)
> >
> > diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> > index f0180adfbe..2796e6e53c 100644
> > --- a/dlls/comctl32/edit.c
> > +++ b/dlls/comctl32/edit.c
> > @@ -131,6 +131,7 @@ typedef struct
> >                          should be sent to the first parent. */
> >       HWND hwndListBox;        /* handle of ComboBox's listbox or 
> > NULL */
> >       INT wheelDeltaRemainder;        /* scroll wheel delta left over 
> > after scrolling whole lines */
> > +       WCHAR *cue_banner_text;
> >       /*
> >        *    only for multi line controls
> >        */
> > @@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, HDC 
> > dc, INT line, BOOL rev)
> >           x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - 
> > e, FALSE);
> >       } else
> >           x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
> > -
> >
> > +       if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && 
> > es->text_length == 0)
> > +       {
> > +           SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
> > +           TextOutW(dc, x, y, es->cue_banner_text, 
> > strlenW(es->cue_banner_text));
> > +       }
> >   }
>
> Now looking again, it seems it would be better to move this to 
> WM_PAINT, because it does not have to be called for every line. Does 
> this complicate (x,y) calculation a lot?

I'll look into this. Hopefully retrieving (x,y) won't be complicated.
> > @@ -4152,6 +4159,52 @@ static LRESULT EDIT_EM_GetThumb(EDITSTATE 
> > *es)
> >                           EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
> >   }
> > +/*********************************************************************
> > -
> > -
> > - *    EM_SETCUEBANNER
> > -
> > -
> > - */
> >
> > +static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR *cue_text)
> > +{
> > +    SIZE_T str_size;
> > -
> >
> > +    if (es->style & ES_MULTILINE || !cue_text)
> > +        return FALSE;
> > -
> >
> > +    str_size = strlenW(cue_text);
> > -
> >
> > +    if (es->cue_banner_text)
> > +        heap_free(es->cue_banner_text);
> > -
> >
> > +    es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
> > +    if (!es->cue_banner_text)
> > +        return FALSE;
> > -
> >
> > +    memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
> > +    es->cue_banner_text[str_size] = 0;
> > -
> >
> > +    return TRUE;
> > +}
>
> I think you can reuse heap_strdupW() from propsheet.c for that, first 
> moving it to a header, and changing Alloc() -> heap_alloc(). No need 
> to null check before releasing old text by the way.

Ok.
> > -
> >
> > +/*********************************************************************
> > -
> > -
> > - *    EM_GETCUEBANNER
> > -
> > -
> > - */
> >
> > +static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD size)
> > +{
> > +    if (!es->cue_banner_text)
> > +        return FALSE;
> > +    else
> > +    {
> > +        if (size > strlenW(es->cue_banner_text))
> > +            size = strlenW(es->cue_banner_text) + 1;
> > +        memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
> > +        buf[size-1] = 0;
> > +        return TRUE;
> > +    }
> > +}
> > -
>
> Could you use lstrcpynW() for this? Also it seems inaccurate for null 
> text case - for ES_MULTILINE it shouldn't touch output buffer, and for 
> single line case it write empty string, that's according to my quick 
> testing, we'll need some tests for that. Also is it supposed to crash 
> on null destination?

I'm not sure I understand. For "null text case" this will always return 
FALSE. If the control has ES_MULTILINE, then es->cue_banner_text will be 
null, so it won't touch output buffer. I included tests to Set/Get the 
empty string.

It shouldn't crash; I'll fix that.
> > @@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND 
> > hwnd, UINT msg, WPARAM wParam, LPAR
> >           result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), 
> > (short)HIWORD(lParam));
> >           break;
> >   +    case EM_SETCUEBANNER:
> > +        result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
> > +        break;
>
> So does wParam work or not?

It doesn't; it is always ignored on Windows. I included it in the 
previous version just in case, but I think it is best to remove it.
> > +static void test_cue_banner(void)
> > +{
> > +    HWND hwnd_edit;
> > +    BOOL ret;
> > +    static WCHAR testW[] = {'T','e','s','t',0};
> > +    static WCHAR testcmp1W[] = {'T','e','s','t',0};
> > +    static WCHAR testcmp2W[] = {'T','e','s',0};
> > +    static WCHAR getcuetestW[5];
> > +    static WCHAR emptyW[] = {0};
> > -
> >
> > +    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | ES_AUTOVSCROLL, 
> > 0);
> > -
> >
> > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > -
> >
> > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
> > +    ok(ret == FALSE, "EM_SETCUEBANNER should have returned FALSE.\n");
> > -
> >
> > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > -
> >
> > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
> > +    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
> > -
> >
> > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 
> > (WPARAM)getcuetestW, 5);
> > +    if (ret == FALSE)
> > +    {
> > +        win_skip("skipping for Win XP and 2003 Server.\n");
> > +        DestroyWindow(hwnd_edit);
> > +        return;
> > +    }
>
> Move this up please. You can test that returned buffer is not null 
> terminated as indication of unimplemented message on XP. No reason to 
> run other tests before this skip if it's not going to work.

Ok.

[Nikolay Sivov]

On 10/23/2018 07:07 PM, Sergio Gómez Del Real wrote:
> On 23/10/18 1:47 a. m., Nikolay Sivov wrote:
> > On 10/22/2018 09:24 PM, Sergio Gómez Del Real wrote:
> > > Signed-off-by: Sergio Gómez Del Real
> > >
> > > dlls/comctl32/edit.c       | 62 
> > > ++++++++++++++++++++++++++++++++++++++
> > >   dlls/comctl32/tests/edit.c | 62 
> > > ++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 124 insertions(+)
> > >
> > > diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> > > index f0180adfbe..2796e6e53c 100644
> > > --- a/dlls/comctl32/edit.c
> > > +++ b/dlls/comctl32/edit.c
> > > @@ -131,6 +131,7 @@ typedef struct
> > >                          should be sent to the first parent. */
> > >       HWND hwndListBox;        /* handle of ComboBox's listbox or 
> > > NULL */
> > >       INT wheelDeltaRemainder;        /* scroll wheel delta left 
> > > over after scrolling whole lines */
> > > +       WCHAR *cue_banner_text;
> > >       /*
> > >        *    only for multi line controls
> > >        */
> > > @@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, HDC 
> > > dc, INT line, BOOL rev)
> > >           x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - 
> > > e, FALSE);
> > >       } else
> > >           x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
> > > -
> > >
> > > +       if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && 
> > > es->text_length == 0)
> > > +       {
> > > +           SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
> > > +           TextOutW(dc, x, y, es->cue_banner_text, 
> > > strlenW(es->cue_banner_text));
> > > +       }
> > >   }
> >
> > Now looking again, it seems it would be better to move this to 
> > WM_PAINT, because it does not have to be called for every line. Does 
> > this complicate (x,y) calculation a lot?
>
> I'll look into this. Hopefully retrieving (x,y) won't be complicated.
> > > @@ -4152,6 +4159,52 @@ static LRESULT EDIT_EM_GetThumb(EDITSTATE 
> > > *es)
> > >                           EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
> > >   }
> > > +/*********************************************************************
> > > -
> > > -
> > > - *    EM_SETCUEBANNER
> > > -
> > > -
> > > - */
> > >
> > > +static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR *cue_text)
> > > +{
> > > +    SIZE_T str_size;
> > > -
> > >
> > > +    if (es->style & ES_MULTILINE || !cue_text)
> > > +        return FALSE;
> > > -
> > >
> > > +    str_size = strlenW(cue_text);
> > > -
> > >
> > > +    if (es->cue_banner_text)
> > > +        heap_free(es->cue_banner_text);
> > > -
> > >
> > > +    es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
> > > +    if (!es->cue_banner_text)
> > > +        return FALSE;
> > > -
> > >
> > > +    memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
> > > +    es->cue_banner_text[str_size] = 0;
> > > -
> > >
> > > +    return TRUE;
> > > +}
> >
> > I think you can reuse heap_strdupW() from propsheet.c for that, first 
> > moving it to a header, and changing Alloc() -> heap_alloc(). No need 
> > to null check before releasing old text by the way.
>
> Ok.
> > > -
> > >
> > > +/*********************************************************************
> > > -
> > > -
> > > - *    EM_GETCUEBANNER
> > > -
> > > -
> > > - */
> > >
> > > +static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD 
> > > size)
> > > +{
> > > +    if (!es->cue_banner_text)
> > > +        return FALSE;
> > > +    else
> > > +    {
> > > +        if (size > strlenW(es->cue_banner_text))
> > > +            size = strlenW(es->cue_banner_text) + 1;
> > > +        memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
> > > +        buf[size-1] = 0;
> > > +        return TRUE;
> > > +    }
> > > +}
> > > -
> >
> > Could you use lstrcpynW() for this? Also it seems inaccurate for null 
> > text case - for ES_MULTILINE it shouldn't touch output buffer, and 
> > for single line case it write empty string, that's according to my 
> > quick testing, we'll need some tests for that. Also is it supposed to 
> > crash on null destination?
>
> I'm not sure I understand. For "null text case" this will always 
> return FALSE. If the control has ES_MULTILINE, then 
> es->cue_banner_text will be null, so it won't touch output buffer. I 
> included tests to Set/Get the empty string.

I mean buff != NULL && size != 0 && cue_banner_text == NULL, what 
happens to buff.
> It shouldn't crash; I'll fix that.
> > > @@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND 
> > > hwnd, UINT msg, WPARAM wParam, LPAR
> > >           result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), 
> > > (short)HIWORD(lParam));
> > >           break;
> > >   +    case EM_SETCUEBANNER:
> > > +        result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
> > > +        break;
> >
> > So does wParam work or not?
>
> It doesn't; it is always ignored on Windows. I included it in the 
> previous version just in case, but I think it is best to remove it.
> > > +static void test_cue_banner(void)
> > > +{
> > > +    HWND hwnd_edit;
> > > +    BOOL ret;
> > > +    static WCHAR testW[] = {'T','e','s','t',0};
> > > +    static WCHAR testcmp1W[] = {'T','e','s','t',0};
> > > +    static WCHAR testcmp2W[] = {'T','e','s',0};
> > > +    static WCHAR getcuetestW[5];
> > > +    static WCHAR emptyW[] = {0};
> > > -
> > >
> > > +    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | 
> > > ES_AUTOVSCROLL, 0);
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_SETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
> > > +    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 
> > > (WPARAM)getcuetestW, 5);
> > > +    if (ret == FALSE)
> > > +    {
> > > +        win_skip("skipping for Win XP and 2003 Server.\n");
> > > +        DestroyWindow(hwnd_edit);
> > > +        return;
> > > +    }
> >
> > Move this up please. You can test that returned buffer is not null 
> > terminated as indication of unimplemented message on XP. No reason to 
> > run other tests before this skip if it's not going to work.
>
> Ok.

[Sergio Gómez Del Real]

On 23/10/18 11:35 a. m., Nikolay Sivov wrote:
> On 10/23/2018 07:07 PM, Sergio Gómez Del Real wrote:
> > On 23/10/18 1:47 a. m., Nikolay Sivov wrote:
> > > On 10/22/2018 09:24 PM, Sergio Gómez Del Real wrote:
> > > > Signed-off-by: Sergio Gómez Del Real
> > > >
> > > > dlls/comctl32/edit.c       | 62 
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >   dlls/comctl32/tests/edit.c | 62 
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >   2 files changed, 124 insertions(+)
> > > >
> > > > diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> > > > index f0180adfbe..2796e6e53c 100644
> > > > --- a/dlls/comctl32/edit.c
> > > > +++ b/dlls/comctl32/edit.c
> > > > @@ -131,6 +131,7 @@ typedef struct
> > > >                          should be sent to the first parent. */
> > > >       HWND hwndListBox;        /* handle of ComboBox's listbox or 
> > > > NULL */
> > > >       INT wheelDeltaRemainder;        /* scroll wheel delta left 
> > > > over after scrolling whole lines */
> > > > +       WCHAR *cue_banner_text;
> > > >       /*
> > > >        *    only for multi line controls
> > > >        */
> > > > @@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, 
> > > > HDC dc, INT line, BOOL rev)
> > > >           x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - 
> > > > e, FALSE);
> > > >       } else
> > > >           x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
> > > > -
> > > >
> > > > +       if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && 
> > > > es->text_length == 0)
> > > > +       {
> > > > +           SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
> > > > +           TextOutW(dc, x, y, es->cue_banner_text, 
> > > > strlenW(es->cue_banner_text));
> > > > +       }
> > > >   }
> > >
> > > Now looking again, it seems it would be better to move this to 
> > > WM_PAINT, because it does not have to be called for every line. Does 
> > > this complicate (x,y) calculation a lot?
> >
> > I'll look into this. Hopefully retrieving (x,y) won't be complicated.
> > > > @@ -4152,6 +4159,52 @@ static LRESULT 
> > > > EDIT_EM_GetThumb(EDITSTATE *es)
> > > >                           EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
> > > >   }
> > > > +/*********************************************************************
> > > > -
> > > > -
> > > > - *    EM_SETCUEBANNER
> > > > -
> > > > -
> > > > - */
> > > >
> > > > +static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR 
> > > > *cue_text)
> > > > +{
> > > > +    SIZE_T str_size;
> > > > -
> > > >
> > > > +    if (es->style & ES_MULTILINE || !cue_text)
> > > > +        return FALSE;
> > > > -
> > > >
> > > > +    str_size = strlenW(cue_text);
> > > > -
> > > >
> > > > +    if (es->cue_banner_text)
> > > > +        heap_free(es->cue_banner_text);
> > > > -
> > > >
> > > > +    es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
> > > > +    if (!es->cue_banner_text)
> > > > +        return FALSE;
> > > > -
> > > >
> > > > +    memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
> > > > +    es->cue_banner_text[str_size] = 0;
> > > > -
> > > >
> > > > +    return TRUE;
> > > > +}
> > >
> > > I think you can reuse heap_strdupW() from propsheet.c for that, 
> > > first moving it to a header, and changing Alloc() -> heap_alloc(). 
> > > No need to null check before releasing old text by the way.
> >
> > Ok.
> > > > -
> > > >
> > > > +/*********************************************************************
> > > > -
> > > > -
> > > > - *    EM_GETCUEBANNER
> > > > -
> > > > -
> > > > - */
> > > >
> > > > +static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD 
> > > > size)
> > > > +{
> > > > +    if (!es->cue_banner_text)
> > > > +        return FALSE;
> > > > +    else
> > > > +    {
> > > > +        if (size > strlenW(es->cue_banner_text))
> > > > +            size = strlenW(es->cue_banner_text) + 1;
> > > > +        memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
> > > > +        buf[size-1] = 0;
> > > > +        return TRUE;
> > > > +    }
> > > > +}
> > > > -
> > >
> > > Could you use lstrcpynW() for this? Also it seems inaccurate for 
> > > null text case - for ES_MULTILINE it shouldn't touch output buffer, 
> > > and for single line case it write empty string, that's according to 
> > > my quick testing, we'll need some tests for that. Also is it 
> > > supposed to crash on null destination?
> >
> > I'm not sure I understand. For "null text case" this will always 
> > return FALSE. If the control has ES_MULTILINE, then 
> > es->cue_banner_text will be null, so it won't touch output buffer. I 
> > included tests to Set/Get the empty string.
>
> I mean buff != NULL && size != 0 && cue_banner_text == NULL, what 
> happens to buff.

Your are right; it is writing empty string in the case you are 
considering. I'll update tests for this case.
> > It shouldn't crash; I'll fix that.
> > > > @@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND 
> > > > hwnd, UINT msg, WPARAM wParam, LPAR
> > > >           result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), 
> > > > (short)HIWORD(lParam));
> > > >           break;
> > > >   +    case EM_SETCUEBANNER:
> > > > +        result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
> > > > +        break;
> > >
> > > So does wParam work or not?
> >
> > It doesn't; it is always ignored on Windows. I included it in the 
> > previous version just in case, but I think it is best to remove it.
> > > > +static void test_cue_banner(void)
> > > > +{
> > > > +    HWND hwnd_edit;
> > > > +    BOOL ret;
> > > > +    static WCHAR testW[] = {'T','e','s','t',0};
> > > > +    static WCHAR testcmp1W[] = {'T','e','s','t',0};
> > > > +    static WCHAR testcmp2W[] = {'T','e','s',0};
> > > > +    static WCHAR getcuetestW[5];
> > > > +    static WCHAR emptyW[] = {0};
> > > > -
> > > >
> > > > +    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | 
> > > > ES_AUTOVSCROLL, 0);
> > > > -
> > > >
> > > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned 
> > > > FALSE.\n");
> > > > -
> > > >
> > > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
> > > > +    ok(ret == FALSE, "EM_SETCUEBANNER should have returned 
> > > > FALSE.\n");
> > > > -
> > > >
> > > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned 
> > > > FALSE.\n");
> > > > -
> > > >
> > > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
> > > > +    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
> > > > -
> > > >
> > > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 
> > > > (WPARAM)getcuetestW, 5);
> > > > +    if (ret == FALSE)
> > > > +    {
> > > > +        win_skip("skipping for Win XP and 2003 Server.\n");
> > > > +        DestroyWindow(hwnd_edit);
> > > > +        return;
> > > > +    }
> > >
> > > Move this up please. You can test that returned buffer is not null 
> > > terminated as indication of unimplemented message on XP. No reason 
> > > to run other tests before this skip if it's not going to work.
> >
> > Ok.

[Sergio Gómez Del Real]

On 23/10/18 11:07 a. m., Sergio Gómez Del Real wrote:
> On 23/10/18 1:47 a. m., Nikolay Sivov wrote:
> > On 10/22/2018 09:24 PM, Sergio Gómez Del Real wrote:
> > > Signed-off-by: Sergio Gómez Del Real
> > >
> > > dlls/comctl32/edit.c       | 62 
> > > ++++++++++++++++++++++++++++++++++++++
> > >   dlls/comctl32/tests/edit.c | 62 
> > > ++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 124 insertions(+)
> > >
> > > diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> > > index f0180adfbe..2796e6e53c 100644
> > > --- a/dlls/comctl32/edit.c
> > > +++ b/dlls/comctl32/edit.c
> > > @@ -131,6 +131,7 @@ typedef struct
> > >                          should be sent to the first parent. */
> > >       HWND hwndListBox;        /* handle of ComboBox's listbox or 
> > > NULL */
> > >       INT wheelDeltaRemainder;        /* scroll wheel delta left 
> > > over after scrolling whole lines */
> > > +       WCHAR *cue_banner_text;
> > >       /*
> > >        *    only for multi line controls
> > >        */
> > > @@ -2181,6 +2182,12 @@ static void EDIT_PaintLine(EDITSTATE *es, HDC 
> > > dc, INT line, BOOL rev)
> > >           x += EDIT_PaintText(es, dc, x, y, line, e - li, li + ll - 
> > > e, FALSE);
> > >       } else
> > >           x += EDIT_PaintText(es, dc, x, y, line, 0, ll, FALSE);
> > > -
> > >
> > > +       if (!(es->flags & EF_FOCUSED) && es->cue_banner_text && 
> > > es->text_length == 0)
> > > +       {
> > > +           SetTextColor(dc, GetSysColor(COLOR_GRAYTEXT));
> > > +           TextOutW(dc, x, y, es->cue_banner_text, 
> > > strlenW(es->cue_banner_text));
> > > +       }
> > >   }
> >
> > Now looking again, it seems it would be better to move this to 
> > WM_PAINT, because it does not have to be called for every line. Does 
> > this complicate (x,y) calculation a lot?
>
> I'll look into this. Hopefully retrieving (x,y) won't be complicated.

However, since CUE BANNER only makes sense for single-line controls, 
it'd be called only once (when painting the one and only line). For 
multi-lines it will never be called. Could we just take advantage of 
already having (x,y) and leave the code where it is?
> > > @@ -4152,6 +4159,52 @@ static LRESULT EDIT_EM_GetThumb(EDITSTATE 
> > > *es)
> > >                           EDIT_WM_HScroll(es, EM_GETTHUMB, 0));
> > >   }
> > > +/*********************************************************************
> > > -
> > > -
> > > - *    EM_SETCUEBANNER
> > > -
> > > -
> > > - */
> > >
> > > +static BOOL EDIT_EM_SetCueBanner(EDITSTATE *es, const WCHAR *cue_text)
> > > +{
> > > +    SIZE_T str_size;
> > > -
> > >
> > > +    if (es->style & ES_MULTILINE || !cue_text)
> > > +        return FALSE;
> > > -
> > >
> > > +    str_size = strlenW(cue_text);
> > > -
> > >
> > > +    if (es->cue_banner_text)
> > > +        heap_free(es->cue_banner_text);
> > > -
> > >
> > > +    es->cue_banner_text = heap_alloc((str_size + 1) * sizeof(WCHAR));
> > > +    if (!es->cue_banner_text)
> > > +        return FALSE;
> > > -
> > >
> > > +    memcpy(es->cue_banner_text, cue_text, str_size * sizeof(WCHAR));
> > > +    es->cue_banner_text[str_size] = 0;
> > > -
> > >
> > > +    return TRUE;
> > > +}
> >
> > I think you can reuse heap_strdupW() from propsheet.c for that, first 
> > moving it to a header, and changing Alloc() -> heap_alloc(). No need 
> > to null check before releasing old text by the way.
>
> Ok.
> > > -
> > >
> > > +/*********************************************************************
> > > -
> > > -
> > > - *    EM_GETCUEBANNER
> > > -
> > > -
> > > - */
> > >
> > > +static BOOL EDIT_EM_GetCueBanner(EDITSTATE *es, WCHAR *buf, DWORD 
> > > size)
> > > +{
> > > +    if (!es->cue_banner_text)
> > > +        return FALSE;
> > > +    else
> > > +    {
> > > +        if (size > strlenW(es->cue_banner_text))
> > > +            size = strlenW(es->cue_banner_text) + 1;
> > > +        memcpy(buf, es->cue_banner_text, (size - 1) * sizeof(WCHAR));
> > > +        buf[size-1] = 0;
> > > +        return TRUE;
> > > +    }
> > > +}
> > > -
> >
> > Could you use lstrcpynW() for this? Also it seems inaccurate for null 
> > text case - for ES_MULTILINE it shouldn't touch output buffer, and 
> > for single line case it write empty string, that's according to my 
> > quick testing, we'll need some tests for that. Also is it supposed to 
> > crash on null destination?
>
> I'm not sure I understand. For "null text case" this will always 
> return FALSE. If the control has ES_MULTILINE, then 
> es->cue_banner_text will be null, so it won't touch output buffer. I 
> included tests to Set/Get the empty string.
>
> It shouldn't crash; I'll fix that.
> > > @@ -4703,6 +4757,14 @@ static LRESULT CALLBACK EDIT_WindowProc(HWND 
> > > hwnd, UINT msg, WPARAM wParam, LPAR
> > >           result = EDIT_EM_CharFromPos(es, (short)LOWORD(lParam), 
> > > (short)HIWORD(lParam));
> > >           break;
> > >   +    case EM_SETCUEBANNER:
> > > +        result = EDIT_EM_SetCueBanner(es, (const WCHAR *)lParam);
> > > +        break;
> >
> > So does wParam work or not?
>
> It doesn't; it is always ignored on Windows. I included it in the 
> previous version just in case, but I think it is best to remove it.
> > > +static void test_cue_banner(void)
> > > +{
> > > +    HWND hwnd_edit;
> > > +    BOOL ret;
> > > +    static WCHAR testW[] = {'T','e','s','t',0};
> > > +    static WCHAR testcmp1W[] = {'T','e','s','t',0};
> > > +    static WCHAR testcmp2W[] = {'T','e','s',0};
> > > +    static WCHAR getcuetestW[5];
> > > +    static WCHAR emptyW[] = {0};
> > > -
> > >
> > > +    hwnd_edit = create_editcontrolW(ES_AUTOHSCROLL | 
> > > ES_AUTOVSCROLL, 0);
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_SETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 0, 0);
> > > +    ok(ret == FALSE, "EM_GETCUEBANNER should have returned FALSE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_SETCUEBANNER, 0, (LPARAM)testW);
> > > +    ok(ret == TRUE, "EM_SETCUEBANNER should have returned TRUE.\n");
> > > -
> > >
> > > +    ret = SendMessageW(hwnd_edit, EM_GETCUEBANNER, 
> > > (WPARAM)getcuetestW, 5);
> > > +    if (ret == FALSE)
> > > +    {
> > > +        win_skip("skipping for Win XP and 2003 Server.\n");
> > > +        DestroyWindow(hwnd_edit);
> > > +        return;
> > > +    }
> >
> > Move this up please. You can test that returned buffer is not null 
> > terminated as indication of unimplemented message on XP. No reason to 
> > run other tests before this skip if it's not going to work.
>
> Ok.