[PATCH resend 1/8] comctl32/listbox: Make SetCount fail if LBS_NODATA is not set
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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)) -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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)) -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 */ -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 */ -- 2.19.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 ); } -- 2.19.1
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
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 <testbot(a)winehq.org> 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
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... -- Francois Gouget <fgouget(a)codeweavers.com>
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.
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. -- Francois Gouget <fgouget(a)codeweavers.com>
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.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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 ); } -- 2.19.1
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) ===
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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(); -- 2.19.1
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
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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(); -- 2.19.1
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) ===
On 11/05/2018 02:17 PM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
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))
On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov <nsivov(a)codeweavers.com> 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.
On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov <nsivov(a)codeweavers.com> 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.
On Tue, Nov 6, 2018 at 10:57 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov <nsivov(a)codeweavers.com> 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.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Tue, Nov 6, 2018 at 10:57 AM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 11/5/18 5:40 PM, Gabriel Ivăncescu wrote:
On Mon, Nov 5, 2018 at 2:38 PM Nikolay Sivov <nsivov(a)codeweavers.com> 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. -- Alexandre Julliard julliard(a)winehq.org
On Tue, Nov 6, 2018 at 1:48 PM Alexandre Julliard <julliard(a)winehq.org> 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 julliard(a)winehq.org
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.
participants (5)
-
Alexandre Julliard -
Francois Gouget -
Gabriel Ivăncescu -
Marvin -
Nikolay Sivov