[PATCH 0/2] MR6898: comctl32: Update combobox dropdown size.

[Orin Varley (＠ovarley1)]

When the combobox height is to be set by the application (as the CBS_NOINTEGRALHEIGHT style is on), and when a large size is specified but only a small number of items in the list, the height of the combobox should be set by the number of items rather than the size specified to avoid empty lines.

The first commit fixes this and the second is a test for this behavior.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57360

Closes #7

[PATCH 1/2] comctl32: Update combobox dropdown size.

[Orin Varley]

From: Orin Varley

When the combobox height is to be set by the application (as the CBS_NOINTEGRALHEIGHT style is on),
and when a large size is specified but only a small number of items in the list, the height of the
combobox should be set by the number of items rather than the size specified to avoid empty lines.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57360
---
 dlls/comctl32/combo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/comctl32/combo.c b/dlls/comctl32/combo.c
index cb0e9745b2a..09579952f22 100644
--- a/dlls/comctl32/combo.c
+++ b/dlls/comctl32/combo.c
@@ -971,7 +971,7 @@ static void CBDropDown( LPHEADCOMBO lphc )

if (lphc->dwStyle & CBS_NOINTEGRALHEIGHT)
         {
-            nDroppedHeight -= 1;
+            nDroppedHeight = min(nItems * nIHeight + COMBO_YBORDERSIZE(), nDroppedHeight - 1);
         }
         else
         {

[PATCH 2/2] comctl32: Add a test for a small number of items but big size to the combobox dropdown size tests.

[Orin Varley]

From: Orin Varley

To catch bugs like the one bellow.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57360
---
 dlls/comctl32/tests/combo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/comctl32/tests/combo.c b/dlls/comctl32/tests/combo.c
index 1040b863b13..515348feac9 100644
--- a/dlls/comctl32/tests/combo.c
+++ b/dlls/comctl32/tests/combo.c
@@ -1277,6 +1277,7 @@ static void test_combo_dropdown_size(DWORD style)
         {33, 50, -1},
         {35, 100, 40},
         {15, 50, 3},
+        {7, 650, 40},
     };

for (test = 0; test < ARRAY_SIZE(info_height); test++)

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/combo.c:
>      if (lphc->dwStyle & CBS_NOINTEGRALHEIGHT)
>      {
> -
>        nDroppedHeight -= 1;
> -
>        nDroppedHeight = min(nItems * nIHeight + COMBO_YBORDERSIZE(), nDroppedHeight - 1);

Shouldn't you consider if the items are visible like in the `else` branch?

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/combo.c:
>      if (lphc->dwStyle & CBS_NOINTEGRALHEIGHT)

The commit subject message is a bit vague. What about "comctl32: Make CBS_NOINTEGRALHEIGHT only set minimum comboxbox height."

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/combo.c:
>      {33, 50, -1},
>      {35, 100, 40},

Rename the commit subject message to "comctl32/tests: ... "

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/combo.c:
>      {33, 50, -1},
>      {35, 100, 40},
>      {15, 50, 3},
> -
>    {7, 650, 40},

So we're aiming for fewer items, so why not 1 but 7?

And let's add the tests first, mark the test failures with a todo_wine and then remove it after the fix.

[Orin Varley (＠ovarley1)]

On Tue Nov 26 10:19:24 2024 +0000, Zhiyi Zhang wrote:
> So we're aiming for fewer items, so why not 1 but 7?
> And let's add the tests first, mark the test failures with a todo_wine
> and then remove it after the fix.

We can use 1, I was just using 7 as the application I was looking at used 7 but it doesn't matter really as long as it's small enough.

I'll add those changes in now then. Do you want me to do two separate MRs then?

[Orin Varley (＠ovarley1)]

On Tue Nov 26 10:19:24 2024 +0000, Zhiyi Zhang wrote:
> The commit subject message is a bit vague. What about "comctl32: Make
> CBS_NOINTEGRALHEIGHT only set minimum comboxbox height."

Yep, good call

[Orin Varley (＠ovarley1)]

On Tue Nov 26 10:19:24 2024 +0000, Zhiyi Zhang wrote:
> Shouldn't you consider if the items are visible like in the `else` branch?

Good point. From what I saw on Windows, it didn't seem to do that but I will spend some time checking this again as it is a bit strange. I also should have put another test in where limit < num_items but they're both still smaller than the height so I will add that in.