[PATCH resend 1/8] comctl32/listbox: Make SetCount fail if LBS_NODATA is not set

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

Can anyone please look at this series? Nikolay? They are needed to finally
implement LBS_NODATA (which I've already written but I'm sure it will need
some changes, however I need these series first anyway).

dlls/comctl32/listbox.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 2137ef8..0a7c341 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -1752,7 +1752,7 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
 {
     LRESULT ret;

-    if (HAS_STRINGS(descr))
+    if (!(descr->style & LBS_NODATA))
     {
         SetLastError(ERROR_SETCOUNT_ON_BAD_LB);
         return LB_ERR;
@@ -2518,6 +2518,9 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )
     if (descr->style & LBS_OWNERDRAWVARIABLE) descr->style |= LBS_NOINTEGRALHEIGHT;
     descr->item_height = LISTBOX_SetFont( descr, 0 );

+    if ((descr->style & (LBS_OWNERDRAWFIXED | LBS_HASSTRINGS | LBS_SORT)) != LBS_OWNERDRAWFIXED)
+        descr->style &= ~LBS_NODATA;
+
     if (descr->style & LBS_OWNERDRAWFIXED)
     {
    if( descr->lphc && (descr->lphc->dwStyle & CBS_DROPDOWN))

[PATCH resend 2/8] user32/listbox: Make SetCount fail if LBS_NODATA is not set

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/user32/listbox.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c
index c8bd148..529a47b 100644
--- a/dlls/user32/listbox.c
+++ b/dlls/user32/listbox.c
@@ -1757,7 +1757,7 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
 {
     LRESULT ret;

-    if (HAS_STRINGS(descr))
+    if (!(descr->style & LBS_NODATA))
     {
         SetLastError(ERROR_SETCOUNT_ON_BAD_LB);
         return LB_ERR;
@@ -2523,6 +2523,9 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )
     if (descr->style & LBS_OWNERDRAWVARIABLE) descr->style |= LBS_NOINTEGRALHEIGHT;
     descr->item_height = LISTBOX_SetFont( descr, 0 );

+    if ((descr->style & (LBS_OWNERDRAWFIXED | LBS_HASSTRINGS | LBS_SORT)) != LBS_OWNERDRAWFIXED)
+        descr->style &= ~LBS_NODATA;
+
     if (descr->style & LBS_OWNERDRAWFIXED)
     {
    if( descr->lphc && (descr->lphc->dwStyle & CBS_DROPDOWN))

[PATCH resend 3/8] comctl32/listbox: Clear LBS_OWNERDRAWVARIABLE if LBS_OWNERDRAWFIXED is set

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

Spotted by tests and it simply makes no sense to have both.

dlls/comctl32/listbox.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
index 0a7c341..5e33466 100644
--- a/dlls/comctl32/listbox.c
+++ b/dlls/comctl32/listbox.c
@@ -2523,6 +2523,9 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )

if (descr->style & LBS_OWNERDRAWFIXED)
     {
+        /* Windows accepts both, but FIXED overrides VARIABLE */
+        descr->style &= ~LBS_OWNERDRAWVARIABLE;
+
    if( descr->lphc && (descr->lphc->dwStyle & CBS_DROPDOWN))
    {
        /* WinWord gets VERY unhappy if we send WM_MEASUREITEM from here */

[PATCH resend 4/8] user32/listbox: Clear LBS_OWNERDRAWVARIABLE if LBS_OWNERDRAWFIXED is set

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/user32/listbox.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dlls/user32/listbox.c b/dlls/user32/listbox.c
index 529a47b..02bb923 100644
--- a/dlls/user32/listbox.c
+++ b/dlls/user32/listbox.c
@@ -2528,6 +2528,9 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )

if (descr->style & LBS_OWNERDRAWFIXED)
     {
+        /* Windows accepts both, but FIXED overrides VARIABLE */
+        descr->style &= ~LBS_OWNERDRAWVARIABLE;
+
    if( descr->lphc && (descr->lphc->dwStyle & CBS_DROPDOWN))
    {
        /* WinWord gets VERY unhappy if we send WM_MEASUREITEM from here */

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/comctl32/tests/listbox.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index c9d13a8..16efa60 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -1752,6 +1752,13 @@ static void test_set_count( void )
     GetUpdateRect( listbox, &r, TRUE );
     ok( !IsRectEmpty( &r ), "got empty rect\n");

+    DestroyWindow( listbox );
+
+    listbox = create_listbox( LBS_OWNERDRAWFIXED | WS_CHILD | WS_VISIBLE, parent );
+
+    ret = SendMessageA( listbox, LB_SETCOUNT, 100, 0 );
+    ok( ret == LB_ERR, "expected %d, got %d\n", LB_ERR, ret );
+
     DestroyWindow( listbox );
     DestroyWindow( parent );
 }

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=43911

Your paranoid android.

=== wvistau64 (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_zh_CN (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_fr (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_he (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64 (64 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Gabriel Ivăncescu]

These errors are pre-existing and don't have anything to do with these patches.

On Mon, Nov 5, 2018 at 1:44 PM Marvin  wrote:
> Hi,
>
> While running your changed tests on Windows, I think I found new failures.
> Being a bot and all I'm not very good at pattern recognition, so I might be
> wrong, but could you please double-check?
>
> Full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=43911
>
> Your paranoid android.
>
> === wvistau64 (32 bit Windows report) ===
>
> comctl32:
> listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18
>
> === wvistau64_zh_CN (32 bit Windows report) ===
>
> comctl32:
> listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18
>
> === wvistau64_fr (32 bit Windows report) ===
>
> comctl32:
> listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18
>
> === wvistau64_he (32 bit Windows report) ===
>
> comctl32:
> listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18
>
> === wvistau64 (64 bit Windows report) ===
>
> comctl32:
> listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Francois Gouget]

On Mon, 5 Nov 2018, Gabriel Ivăncescu wrote:
> These errors are pre-existing and don't have anything to do with these patches.

[...]
> > Full results can be found at:
> > https://testbot.winehq.org/JobDetails.pl?Key=43911

[...]
> > === wvistau64 (32 bit Windows report) ===
> >
> > comctl32:
> > listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

And yet, looking at the wvistau64 results, for example, the error is 
flagged as new (that's why it's orange on the website). Indeed I don't 
see this error in any of the WineTest reports from the past 5 WineTest 
reports for this VM:

https://testbot.winehq.org/JobDetails.pl?Key=43843#k103
https://testbot.winehq.org/JobDetails.pl?Key=43800#k103
https://testbot.winehq.org/JobDetails.pl?Key=43750#k103
https://testbot.winehq.org/JobDetails.pl?Key=43691#k103
https://testbot.winehq.org/JobDetails.pl?Key=43643#k103

I also don't any such failure in the WineTest results:
https://test.winehq.org/data/tests/comctl32:listbox.html

Even the 2 Windows 10 failures are different.

So I'm a bit puzzled by your statement...

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Nikolay Sivov]

On 11/05/2018 07:45 PM, Francois Gouget wrote:
> On Mon, 5 Nov 2018, Gabriel Ivăncescu wrote:
> > These errors are pre-existing and don't have anything to do with these patches.
>
> [...]
> > > Full results can be found at:
> > > https://testbot.winehq.org/JobDetails.pl?Key=43911
>
> [...]
> > > === wvistau64 (32 bit Windows report) ===
> > >
> > > comctl32:
> > > listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18
>
> And yet, looking at the wvistau64 results, for example, the error is
> flagged as new (that's why it's orange on the website). Indeed I don't
> see this error in any of the WineTest reports from the past 5 WineTest
> reports for this VM:
>
> https://testbot.winehq.org/JobDetails.pl?Key=43843#k103
> https://testbot.winehq.org/JobDetails.pl?Key=43800#k103
> https://testbot.winehq.org/JobDetails.pl?Key=43750#k103
> https://testbot.winehq.org/JobDetails.pl?Key=43691#k103
> https://testbot.winehq.org/JobDetails.pl?Key=43643#k103
>
> I also don't any such failure in the WineTest results:
> https://test.winehq.org/data/tests/comctl32:listbox.html
>
> Even the 2 Windows 10 failures are different.
>
> So I'm a bit puzzled by your statement...

It's true that it's not a new failure, I've seen testbot report it many 
times for sent patches. Don't know about full test results.

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Francois Gouget]

On Mon, 5 Nov 2018, Nikolay Sivov wrote:
[...]
> It's true that it's not a new failure, I've seen testbot report it many 
> times for sent patches. Don't know about full test results.

I found why the test fails on wvistau64. From the description of the 
configuration for that VM:

Running tests from E: drive.

The tests are run from E:\ which is an empty drive so it has no 
subfolders and no '..' parent directory. This is why LB_DIR fails when 
asked to only return directories.

So comctl32:listbox requires to be run in a path with either a '..' 
parent directory or some subdirectory (despite stating the opposite in a 
comment). Such a requirement is wrong.

And the reason it does not fail during the full test suite must be that 
some previous test leaves directories behind.

So comctl32:listbox is broken.
And some other test is broken too.

[PATCH resend 5/8] comctl32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Nikolay Sivov]

On 11/6/18 7:43 PM, Francois Gouget wrote:
> On Mon, 5 Nov 2018, Nikolay Sivov wrote:
> [...]
> > It's true that it's not a new failure, I've seen testbot report it many
> > times for sent patches. Don't know about full test results.
>
> I found why the test fails on wvistau64. From the description of the
> configuration for that VM:
> Running tests from E: drive.
>
> The tests are run from E:\ which is an empty drive so it has no
> subfolders and no '..' parent directory. This is why LB_DIR fails when
> asked to only return directories.
>
> So comctl32:listbox requires to be run in a path with either a '..'
> parent directory or some subdirectory (despite stating the opposite in a
> comment). Such a requirement is wrong.
>
> And the reason it does not fail during the full test suite must be that
> some previous test leaves directories behind.
>
> So comctl32:listbox is broken.
> And some other test is broken too.

Makes sense. I'll send something to make it better.
>

[PATCH resend 6/8] user32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/user32/tests/listbox.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/dlls/user32/tests/listbox.c b/dlls/user32/tests/listbox.c
index dfa8de7..4d9e70f 100644
--- a/dlls/user32/tests/listbox.c
+++ b/dlls/user32/tests/listbox.c
@@ -1710,6 +1710,13 @@ static void test_set_count( void )
     GetUpdateRect( listbox, &r, TRUE );
     ok( !IsRectEmpty( &r ), "got empty rect\n");

+    DestroyWindow( listbox );
+
+    listbox = create_listbox( LBS_OWNERDRAWFIXED | WS_CHILD | WS_VISIBLE, parent );
+
+    ret = SendMessageA( listbox, LB_SETCOUNT, 100, 0 );
+    ok( ret == LB_ERR, "expected %d, got %d\n", LB_ERR, ret );
+
     DestroyWindow( listbox );
     DestroyWindow( parent );
 }

[PATCH resend 6/8] user32/tests: Test for LB_SETCOUNT without LBS_NODATA

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=43912

Your paranoid android.

=== wvistau64 (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_zh_CN (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_fr (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_he (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64 (64 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== debian9 (build log) ===

=== debian9 (build log) ===

[PATCH resend 7/8] comctl32/tests: Test incompatible styles for LBS_NODATA

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/comctl32/tests/listbox.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/dlls/comctl32/tests/listbox.c b/dlls/comctl32/tests/listbox.c
index 16efa60..ad341c5 100644
--- a/dlls/comctl32/tests/listbox.c
+++ b/dlls/comctl32/tests/listbox.c
@@ -1763,6 +1763,26 @@ static void test_set_count( void )
     DestroyWindow( parent );
 }

+static void test_nodata_invalid_styles( void )
+{
+    static const DWORD style[] =
+    {
+        0, LBS_OWNERDRAWVARIABLE, LBS_SORT, LBS_HASSTRINGS,
+        LBS_OWNERDRAWFIXED | LBS_SORT, LBS_OWNERDRAWFIXED | LBS_HASSTRINGS
+    };
+    HWND parent, listbox;
+    UINT i;
+
+    parent = create_parent();
+    for (i = 0; i < ARRAY_SIZE(style); i++)
+    {
+        listbox = create_listbox(style[i] | LBS_NODATA | WS_CHILD | WS_VISIBLE, parent);
+        ok(SendMessageA(listbox, LB_SETCOUNT, 100, 0) == LB_ERR, "LBS_NODATA enabled with incompatible styles 0x%X\n", style[i]);
+        DestroyWindow(listbox);
+    }
+    DestroyWindow(parent);
+}
+
 static int lb_getlistboxinfo;

static LRESULT WINAPI listbox_subclass_proc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
@@ -2204,6 +2224,7 @@ START_TEST(listbox)
     test_listbox_LB_DIR();
     test_listbox_dlgdir();
     test_set_count();
+    test_nodata_invalid_styles();
     test_GetListBoxInfo();
     test_missing_lbuttonup();
     test_extents();

[PATCH resend 7/8] comctl32/tests: Test incompatible styles for LBS_NODATA

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=43913

Your paranoid android.

=== wvistau64 (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_zh_CN (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_fr (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_he (32 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64 (64 bit Windows report) ===

comctl32:
listbox.c:1129: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

[PATCH resend 8/8] user32/tests: Test incompatible styles for LBS_NODATA

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 dlls/user32/tests/listbox.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/dlls/user32/tests/listbox.c b/dlls/user32/tests/listbox.c
index 4d9e70f..5259f29 100644
--- a/dlls/user32/tests/listbox.c
+++ b/dlls/user32/tests/listbox.c
@@ -1721,6 +1721,26 @@ static void test_set_count( void )
     DestroyWindow( parent );
 }

+static void test_nodata_invalid_styles( void )
+{
+    static const DWORD style[] =
+    {
+        0, LBS_OWNERDRAWVARIABLE, LBS_SORT, LBS_HASSTRINGS,
+        LBS_OWNERDRAWFIXED | LBS_SORT, LBS_OWNERDRAWFIXED | LBS_HASSTRINGS
+    };
+    HWND parent, listbox;
+    UINT i;
+
+    parent = create_parent();
+    for (i = 0; i < ARRAY_SIZE(style); i++)
+    {
+        listbox = create_listbox(style[i] | LBS_NODATA | WS_CHILD | WS_VISIBLE, parent);
+        ok(SendMessageA(listbox, LB_SETCOUNT, 100, 0) == LB_ERR, "LBS_NODATA enabled with incompatible styles 0x%X\n", style[i]);
+        DestroyWindow(listbox);
+    }
+    DestroyWindow(parent);
+}
+
 static DWORD (WINAPI *pGetListBoxInfo)(HWND);
 static int lb_getlistboxinfo;

@@ -2067,6 +2087,7 @@ START_TEST(listbox)
   test_listbox_LB_DIR();
   test_listbox_dlgdir();
   test_set_count();
+  test_nodata_invalid_styles();
   test_GetListBoxInfo();
   test_missing_lbuttonup();
   test_extents();

[PATCH resend 8/8] user32/tests: Test incompatible styles for LBS_NODATA

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=43914

Your paranoid android.

=== wvistau64 (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_zh_CN (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_fr (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64_he (32 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== wvistau64 (64 bit Windows report) ===

user32:
listbox.c:844: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7
listbox.c:1079: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18

=== debian9 (32 bit Wine report) ===

user32:
msg.c:8263: Test failed: WaitForSingleObject failed 102
msg.c:8269: Test failed: destroy child on thread exit: 0: the msg 0x0082 was expected, but got msg 0x000f instead
msg.c:8269: Test failed: destroy child on thread exit: 1: the msg 0x000f was expected, but got msg 0x0014 instead
msg.c:8269: Test failed: destroy child on thread exit: 2: the msg sequence is not complete: expected 0014 - actual 0000

=== debian9 (build log) ===

=== debian9 (build log) ===

[Nikolay Sivov]

On 11/05/2018 02:17 PM, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu
>
> Can anyone please look at this series? Nikolay? They are needed to finally
> implement LBS_NODATA (which I've already written but I'm sure it will need
> some changes, however I need these series first anyway).

What we need first is more tests covering this series and actual 
implementation that you have written.
> dlls/comctl32/listbox.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> index 2137ef8..0a7c341 100644
> --- a/dlls/comctl32/listbox.c
> +++ b/dlls/comctl32/listbox.c
> @@ -1752,7 +1752,7 @@ static LRESULT LISTBOX_SetCount( LB_DESCR *descr, INT count )
>   {
>       LRESULT ret;
> - if (HAS_STRINGS(descr))
> - if (!(descr->style & LBS_NODATA))
>  {
>      SetLastError(ERROR_SETCOUNT_ON_BAD_LB);
>      return LB_ERR;
>
> @@ -2518,6 +2518,9 @@ static BOOL LISTBOX_Create( HWND hwnd, LPHEADCOMBO lphc )
>       if (descr->style & LBS_OWNERDRAWVARIABLE) descr->style |= LBS_NOINTEGRALHEIGHT;
>       descr->item_height = LISTBOX_SetFont( descr, 0 );
> - if ((descr->style & (LBS_OWNERDRAWFIXED | LBS_HASSTRINGS | LBS_SORT)) != LBS_OWNERDRAWFIXED)
> -
>    descr->style &= ~LBS_NODATA;
> -

It's easier to see and test if it's correct when it fixes existing tests.

P.S. please let's focus on a single module first, user32 or comctl32, 
your choice. This will reduce a number of patches you have to send, and 
we still can duplicate for other module later.
>   if (descr->style & LBS_OWNERDRAWFIXED)
>   {
>
> if( descr->lphc && (descr->lphc->dwStyle & CBS_DROPDOWN))

[Gabriel Ivăncescu]

On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov  wrote:
> What we need first is more tests covering this series and actual
> implementation that you have written.

But there will be more patches that I split before I implement
LBS_NODATA. And in fact the LBS_NODATA patch is pretty large by itself
(mostly due to multi-column listboxes). Do you want me to send them
all in one go? (it's about 9 patches or so, the last one is pretty
large, couldn't split it up).

These patches, for now, don't break anything though, only correct some
behavior. LBS_NODATA will still work (but very bad performance).
That's why I split them. Are you sure you don't want them as it is?
(then I can send the next batch)
> It's easier to see and test if it's correct when it fixes existing tests.

What do you mean? I don't think there are any existing tests for
LBS_NODATA. Of course I add them as I "fix" it in the patches.

Apart from these behaviors, LBS_NODATA doesn't really break anything
since its only purpose is for performance on large lists.
> P.S. please let's focus on a single module first, user32 or comctl32,
> your choice. This will reduce a number of patches you have to send, and
> we still can duplicate for other module later.

Ok.

[Gabriel Ivăncescu]

Sorry, I meant "multi-selection" listboxes, not multi-column.

[Nikolay Sivov]

On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
> On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov  wrote:
> > What we need first is more tests covering this series and actual
> > implementation that you have written.
>
> But there will be more patches that I split before I implement
> LBS_NODATA. And in fact the LBS_NODATA patch is pretty large by itself
> (mostly due to multi-column listboxes). Do you want me to send them
> all in one go? (it's about 9 patches or so, the last one is pretty
> large, couldn't split it up).
>
> These patches, for now, don't break anything though, only correct some
> behavior. LBS_NODATA will still work (but very bad performance).
> That's why I split them. Are you sure you don't want them as it is?
> (then I can send the next batch)

No, I don't want them in one go, all I asked is to work to get tests 
committed first, and then proceed with the rest.
> > It's easier to see and test if it's correct when it fixes existing tests.
>
> What do you mean? I don't think there are any existing tests for
> LBS_NODATA. Of course I add them as I "fix" it in the patches.

There's no tests, that's a problem exactly. What I'm asking is to send 
tests first, mark failing parts accordingly, and then move to improving 
implementation.
> Apart from these behaviors, LBS_NODATA doesn't really break anything
> since its only purpose is for performance on large lists.
> > P.S. please let's focus on a single module first, user32 or comctl32,
> > your choice. This will reduce a number of patches you have to send, and
> > we still can duplicate for other module later.
>
> Ok.

[Gabriel Ivăncescu]

On Tue, Nov 6, 2018 at 10:57 AM Nikolay Sivov  wrote:
> On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
> > On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov  wrote:
> > > What we need first is more tests covering this series and actual
> > > implementation that you have written.
> >
> > But there will be more patches that I split before I implement
> > LBS_NODATA. And in fact the LBS_NODATA patch is pretty large by itself
> > (mostly due to multi-column listboxes). Do you want me to send them
> > all in one go? (it's about 9 patches or so, the last one is pretty
> > large, couldn't split it up).
> >
> > These patches, for now, don't break anything though, only correct some
> > behavior. LBS_NODATA will still work (but very bad performance).
> > That's why I split them. Are you sure you don't want them as it is?
> > (then I can send the next batch)
>
> No, I don't want them in one go, all I asked is to work to get tests
> committed first, and then proceed with the rest.

Would it not be acceptable to have simple patches followed by tests
that are relevant to those patches? Last time, I was told to do it
this way so I wouldn't have to use todo_wine when it ends up being
replaced afterwards.

So I'm conflicted as to what's the actual practice of sending tests
and how it is supposed to be done.

For now I'll send the patch series again but with all relevant tests
after the given patches -- hopefully that's OK with you. So e.g. first
patch fixes something, then it's followed by tests that prove it's
right. Next patch fixes another thing, then it's followed by relevant
tests for it, etc. This way I avoid having to use todo_wine at all and
still prove correct behavior.

The patches are pretty simple before the actual implementation of LBS_NODATA.

[Alexandre Julliard]

Gabriel Ivăncescu  writes:
> On Tue, Nov 6, 2018 at 10:57 AM Nikolay Sivov  wrote:
> > On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
> > > On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov  wrote:
> > > > What we need first is more tests covering this series and actual
> > > > implementation that you have written.
> > >
> > > But there will be more patches that I split before I implement
> > > LBS_NODATA. And in fact the LBS_NODATA patch is pretty large by itself
> > > (mostly due to multi-column listboxes). Do you want me to send them
> > > all in one go? (it's about 9 patches or so, the last one is pretty
> > > large, couldn't split it up).
> > >
> > > These patches, for now, don't break anything though, only correct some
> > > behavior. LBS_NODATA will still work (but very bad performance).
> > > That's why I split them. Are you sure you don't want them as it is?
> > > (then I can send the next batch)
> >
> > No, I don't want them in one go, all I asked is to work to get tests
> > committed first, and then proceed with the rest.
>
> Would it not be acceptable to have simple patches followed by tests
> that are relevant to those patches? Last time, I was told to do it
> this way so I wouldn't have to use todo_wine when it ends up being
> replaced afterwards.
>
> So I'm conflicted as to what's the actual practice of sending tests
> and how it is supposed to be done.

Both are acceptable, but if it's easy to add todo_wine, it's better to
send the test first. This way it's clear which test failures the patch
is fixing. In your case it seems that adding todos would be trivial, so
that's what you should do.

[Gabriel Ivăncescu]

On Tue, Nov 6, 2018 at 1:48 PM Alexandre Julliard  wrote:
> Both are acceptable, but if it's easy to add todo_wine, it's better to
> send the test first. This way it's clear which test failures the patch
> is fixing. In your case it seems that adding todos would be trivial, so
> that's what you should do.
>
> --
> Alexandre Julliard
> [e-mail]

Well in this case the patches are small and the tests follow them and
prove the behavior of the respective patch. (I already sent them,
hopefully it's not such a big deal)

Just for future reference: If I were to use todo on larger tests (that
have their own function), I would use the todo_wine before calling the
function? Placing them on each 'ok' statement will require a lot of
todos otherwise.