[PATCH] comctl32/tests: Add test for v6 combobox dropdown size

[Fabian Maurer]

Signed-off-by: Fabian Maurer 
---
 dlls/comctl32/tests/combo.c | 135 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/dlls/comctl32/tests/combo.c b/dlls/comctl32/tests/combo.c
index 895d9429b4..74cbe7851e 100644
--- a/dlls/comctl32/tests/combo.c
+++ b/dlls/comctl32/tests/combo.c
@@ -33,6 +33,8 @@
 #define EDITBOX_ID         0
 #define COMBO_ID           1995

+#define COMBO_YBORDERSIZE() 2
+
 #define expect(expected, got) ok(got == expected, "Expected %d, got %d\n", expected, got)

#define expect_rect(r, _left, _top, _right, _bottom) ok(r.left == _left && r.top == _top && \
@@ -1151,6 +1153,137 @@ static void test_combo_WS_VSCROLL(void)
     DestroyWindow(hCombo);
 }

+
+static void test_listbox_size(DWORD style)
+{
+    HWND hCombo, hList;
+    COMBOBOXINFO cbInfo;
+    UINT x, y;
+    BOOL ret;
+    int i, test;
+    const char wine_test[] = "Wine Test";
+
+    static const struct list_size_info
+    {
+        int num_items;
+        int height_combo;
+        int limit;
+        BOOL todo_regular;
+        BOOL todo_nointegral;
+    } info_height[] = {
+        {2, 24, -1, TRUE, TRUE},
+        {2, 41, -1, TRUE, TRUE},
+        {2, 300, -1, TRUE},
+        {6, 50, -1, TRUE, TRUE},
+        {6, 1, -1, TRUE, TRUE},
+        {6, 800, -1, TRUE},
+        {80, 10, -1, TRUE, TRUE},
+        {100, 900, -1, TRUE, TRUE},
+        {2, 100, -1, TRUE},
+
+        {2, 12, 30, FALSE, TRUE},
+        {100, 12, 30, TRUE, TRUE},
+        {2, 12, 1, TRUE, TRUE},
+        {100, 12, 1, TRUE, TRUE},
+        {2, 12, 100, FALSE, TRUE},
+        {100, 12, 100, TRUE, TRUE},
+        {15, 12, 4, TRUE, TRUE},
+        {15, 12, 15, TRUE, TRUE},
+
+        {10, 24, -1, TRUE, TRUE},
+        {10, 41, -1, TRUE, TRUE},
+        {10, 42, -1, TRUE, TRUE},
+        {10, 50, -1, TRUE, TRUE},
+    };
+
+    for(test = 0; test < sizeof(info_height) / sizeof(info_height[0]); test++)
+    {
+        const struct list_size_info *info_test = &info_height[test];
+        int height_item; /* Height of a list item */
+        int height_list; /* Height of the list we got */
+        int expected_height_list;
+        RECT rect_list_client;
+        int min_visible_expected;
+
+        hCombo = CreateWindowA("ComboBox", "Combo", WS_VISIBLE | WS_CHILD | style, 5, 5, 100,
+                info_test->height_combo, hMainWnd, (HMENU)COMBO_ID, NULL, 0);
+
+        min_visible_expected = SendMessageA(hCombo, CB_GETMINVISIBLE, 0, 0);
+        todo_wine
+        ok(min_visible_expected == 30, "Expected maximum of visible items to be 30, got %d\n", min_visible_expected);
+
+        cbInfo.cbSize = sizeof(COMBOBOXINFO);
+        SetLastError(0xdeadbeef);
+        ret = GetComboBoxInfo(hCombo, &cbInfo);
+        ok(ret, "Failed to get COMBOBOXINFO structure; LastError: %u\n", GetLastError());
+
+        hList = cbInfo.hwndList;
+        for (i = 0; i < info_test->num_items; i++)
+            SendMessageA(hCombo, CB_ADDSTRING, 0, (LPARAM) wine_test);
+
+        if(info_test->limit != -1)
+        {
+            int min_visible_actual;
+            min_visible_expected = info_test->limit;
+
+            SendMessageA(hCombo, CB_SETMINVISIBLE, min_visible_expected, 0);
+            min_visible_actual = SendMessageA(hCombo, CB_GETMINVISIBLE, 0, 0);
+            todo_wine
+            ok(min_visible_expected == min_visible_actual, "Expected maximum of visible items to be %d, got %d\n",
+                    min_visible_expected, min_visible_actual);
+        }
+
+        /* Click on the button to drop down the list */
+        x = cbInfo.rcButton.left + (cbInfo.rcButton.right-cbInfo.rcButton.left)/2;
+        y = cbInfo.rcButton.top + (cbInfo.rcButton.bottom-cbInfo.rcButton.top)/2;
+        ret = SendMessageA(hCombo, WM_LBUTTONDOWN, 0, MAKELPARAM(x, y));
+        ok(ret, "WM_LBUTTONDOWN was not processed. LastError=%d\n",
+           GetLastError());
+        ok(SendMessageA(hCombo, CB_GETDROPPEDSTATE, 0, 0),
+           "The dropdown list should have appeared after clicking the button.\n");
+
+        GetClientRect(hList, &rect_list_client);
+        height_list = rect_list_client.bottom - rect_list_client.top;
+        height_item = (int)SendMessageA(hList, LB_GETITEMHEIGHT, 0, 0);
+
+        if(style & CBS_NOINTEGRALHEIGHT)
+        {
+            RECT rect_list_complete;
+            int list_height_nonclient;
+            int list_height_calculated;
+
+            GetWindowRect(hList, &rect_list_complete);
+
+            list_height_nonclient = (rect_list_complete.bottom - rect_list_complete.top)
+                                    - (rect_list_client.bottom - rect_list_client.top);
+
+            /* Calculate the expected client size of the listbox popup from the size of the combobox. */
+            list_height_calculated = info_test->height_combo
+                    - (cbInfo.rcItem.bottom + COMBO_YBORDERSIZE())
+                    - list_height_nonclient
+                    - 1;
+
+            expected_height_list = min(list_height_calculated, height_item * info_test->num_items);
+            if( expected_height_list < 0)
+                expected_height_list = 0;
+
+            todo_wine_if(info_test->todo_nointegral)
+            ok(expected_height_list == height_list,
+               "Test %d, expected list height to be %d, got %d\n", test, expected_height_list, height_list);
+        }
+        else
+        {
+            expected_height_list = min(info_test->num_items, min_visible_expected) * height_item;
+
+            todo_wine_if(info_test->todo_regular)
+            ok(expected_height_list == height_list,
+               "Test %d, expected list height to be %d, got %d\n", test, expected_height_list, height_list);
+        }
+
+        DestroyWindow(hCombo);
+    }
+}
+
 START_TEST(combo)
 {
     ULONG_PTR ctx_cookie;
@@ -1192,6 +1325,8 @@ START_TEST(combo)
     test_combo_listbox_styles(CBS_SIMPLE);
     test_combo_listbox_styles(CBS_DROPDOWN);
     test_combo_listbox_styles(CBS_DROPDOWNLIST);
+    test_listbox_size(CBS_DROPDOWN);
+    test_listbox_size(CBS_DROPDOWN | CBS_NOINTEGRALHEIGHT);

cleanup();
     unload_v6_module(ctx_cookie, hCtx);

[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=35655

Your paranoid android.

=== wxppro (32 bit combo) ===
combo.c:1279: Test failed: Test 7, expected list height to be 480, got 864

=== w2003std (32 bit combo) ===
combo.c:1279: Test failed: Test 7, expected list height to be 480, got 864

[Fabian Maurer]

There seems to be some inconsistency between 
windows versions, the test fails for xp and 2003. 
Should I just check for windows version and adjust 
the tests accordingly?

Regards,
Fabian Maurer

[Nikolay Sivov]

On 1/31/2018 2:19 AM, Fabian Maurer wrote:
> There seems to be some inconsistency between windows versions, the test
> fails for xp and 2003. Should I just check for windows version and
> adjust the tests accordingly?

If it's not essential to demonstrate new behavior of version 6
implementation, you can as well drop it.

Some comments regarding actual patch.
> +#define COMBO_YBORDERSIZE() 2
> -

Why is it defined like that if it's a constant? Also if it's going to be
used in this new test only you can use actual constant variable in new
test function.
> +static void test_listbox_size(DWORD style)

I think it's better to call it test_combo_* to make it clear what's
being tested.
> - } info_height[] = {

...
> -
>    {80, 10, -1, TRUE, TRUE},
> -
>    {100, 900, -1, TRUE, TRUE},

...
> -
>    {100, 12, 30, TRUE, TRUE},

I'm a bit concerned about somewhat long loops this data will cause. Is
it really necessary to have that many items?
> -
>    hCombo = CreateWindowA("ComboBox", "Combo", WS_VISIBLE | WS_CHILD | style, 5, 5, 100,
> -
>            info_test->height_combo, hMainWnd, (HMENU)COMBO_ID, NULL, 0);

Since it's v6 only test, this could be WC_COMBOBOXA.
> - for(test = 0; test < sizeof(info_height) / sizeof(info_height[0]); test++)

...
> -
>    if(info_test->limit != -1)

...
> -
>        if( expected_height_list < 0)

Please reformat this to use consistent spacing - 1 space after keyword.

[Fabian Maurer]

> > There seems to be some inconsistency between windows versions, the test
> > fails for xp and 2003. Should I just check for windows version and
> > adjust the tests accordingly?
>
> If it's not essential to demonstrate new behavior of version 6
> implementation, you can as well drop it.

It's not, I'll get rid of it when reducing the amount of tests.
> > +#define COMBO_YBORDERSIZE() 2
>
> Why is it defined like that if it's a constant? Also if it's going to be
> used in this new test only you can use actual constant variable in new
> test function.

It was defined in original combo like that, that's why I used that. But sure, I'll make it into a 
local constant.
> I'm a bit concerned about somewhat long loops this data will cause. Is
> it really necessary to have that many items?

Probably not, no. I just wanted to make sure, I'll trim that down.
> Please reformat this to use consistent spacing - 1 space after keyword.

Ah, sorry I always forget about that... I'll clean it up right away.

Thanks for the quick reply, I'll send an updated version soon.

Regards,
Fabian Maurer

[Fabian Maurer]

Also, sorry for the double mail,
but I just noticed that the testbot result shows "OK" in 
the patches list. But why, when the testbot (https://
testbot.winehq.org/JobDetails.pl?Key=35655) says 
there's test failures?

Regards,
Fabian Maurer

[Michael Stefaniuc]

On 01/31/2018 12:23 AM, Fabian Maurer wrote:
> Also, sorry for the double mail,
>
> but I just noticed that the testbot result shows "OK" in the patches
> list. But why, when the testbot
> (https://testbot.winehq.org/JobDetails.pl?Key=35655) says there's test
> failures?

The patch watcher filters out "known" test failures. Maybe it got
confused there.

bye
    michael