[PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alex Henrie
Fixes test failures on Arabic and Hebrew Windows. For some reason, on
these locales Windows uses a different formula for allocating the
template buffer, allocating a little more than 2 times the resource
size on Hebrew and a little less on Arabic. Because we don't know how
exactly the buffer size is determined on these locales, just accept a
size near 2 times the resource size.

Signed-off-by: Alex Henrie <[hidden email]>
---
 dlls/comctl32/tests/propsheet.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c
index a3f910d2d5..68dce519bb 100644
--- a/dlls/comctl32/tests/propsheet.c
+++ b/dlls/comctl32/tests/propsheet.c
@@ -58,16 +58,21 @@ static int CALLBACK sheet_callback(HWND hwnd, UINT msg, LPARAM lparam)
     case PSCB_PRECREATE:
       {
         HMODULE module = GetModuleHandleA("comctl32.dll");
+        HMODULE kernel32 = GetModuleHandleA("kernel32.dll");
+        LANGID (WINAPI *pGetThreadUILanguage)(void) = (void *)GetProcAddress(kernel32, "GetThreadUILanguage");
         DWORD size, buffer_size;
         HRSRC hrsrc;
+        int reading_layout;
 
         hrsrc = FindResourceA(module, MAKEINTRESOURCEA(1006 /* IDD_PROPSHEET */),
                 (LPSTR)RT_DIALOG);
         size = SizeofResource(module, hrsrc);
         ok(size != 0, "Failed to get size of propsheet dialog resource\n");
         buffer_size = HeapSize(GetProcessHeap(), 0, (void *)lparam);
-        ok(buffer_size == 2 * size, "Unexpected template buffer size %u, resource size %u\n",
-                buffer_size, size);
+        GetLocaleInfoA(pGetThreadUILanguage ? pGetThreadUILanguage() : GetUserDefaultUILanguage(),
+                LOCALE_IREADINGLAYOUT | LOCALE_RETURN_NUMBER, (void *)&reading_layout, sizeof(reading_layout));
+        ok(reading_layout == 2 /* RTL */ ? abs(buffer_size - 2 * size) <= 32 : buffer_size == 2 * size,
+                "Unexpected template buffer size %u, resource size %u\n", buffer_size, size);
         break;
       }
     case PSCB_INITIALIZED:
--
2.15.0



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Nikolay Sivov
On 13.11.2017 8:13, Alex Henrie wrote:

> Fixes test failures on Arabic and Hebrew Windows. For some reason, on
> these locales Windows uses a different formula for allocating the
> template buffer, allocating a little more than 2 times the resource
> size on Hebrew and a little less on Arabic. Because we don't know how
> exactly the buffer size is determined on these locales, just accept a
> size near 2 times the resource size.
>
> Signed-off-by: Alex Henrie <[hidden email]>
> ---
>  dlls/comctl32/tests/propsheet.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/comctl32/tests/propsheet.c b/dlls/comctl32/tests/propsheet.c
> index a3f910d2d5..68dce519bb 100644
> --- a/dlls/comctl32/tests/propsheet.c
> +++ b/dlls/comctl32/tests/propsheet.c
> @@ -58,16 +58,21 @@ static int CALLBACK sheet_callback(HWND hwnd, UINT msg, LPARAM lparam)
>      case PSCB_PRECREATE:
>        {
>          HMODULE module = GetModuleHandleA("comctl32.dll");
> +        HMODULE kernel32 = GetModuleHandleA("kernel32.dll");
> +        LANGID (WINAPI *pGetThreadUILanguage)(void) = (void *)GetProcAddress(kernel32, "GetThreadUILanguage");
>          DWORD size, buffer_size;
>          HRSRC hrsrc;
> +        int reading_layout;
>  
>          hrsrc = FindResourceA(module, MAKEINTRESOURCEA(1006 /* IDD_PROPSHEET */),
>                  (LPSTR)RT_DIALOG);
>          size = SizeofResource(module, hrsrc);
>          ok(size != 0, "Failed to get size of propsheet dialog resource\n");
>          buffer_size = HeapSize(GetProcessHeap(), 0, (void *)lparam);
> -        ok(buffer_size == 2 * size, "Unexpected template buffer size %u, resource size %u\n",
> -                buffer_size, size);
> +        GetLocaleInfoA(pGetThreadUILanguage ? pGetThreadUILanguage() : GetUserDefaultUILanguage(),
> +                LOCALE_IREADINGLAYOUT | LOCALE_RETURN_NUMBER, (void *)&reading_layout, sizeof(reading_layout));
> +        ok(reading_layout == 2 /* RTL */ ? abs(buffer_size - 2 * size) <= 32 : buffer_size == 2 * size,
> +                "Unexpected template buffer size %u, resource size %u\n", buffer_size, size);
>          break;
>        }
>      case PSCB_INITIALIZED:
>

Is it possible we're picking wrong resource with FindResource() here?
Maybe we should be using explicit language instead of a neutral one, so

---
propsheet.c:69: Test failed: Unexpected template buffer size 508,
resource size 270
---

could mean we were supposed to pick up a different resource of size 254.

P.S. GetLocaleInfo() takes LCID not LANGID, and you can use special
constants like LOCALE_USER_DEFAULT too.



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alex Henrie
2017-11-14 0:40 GMT-07:00 Nikolay Sivov <[hidden email]>:
> Is it possible we're picking wrong resource with FindResource() here?
> Maybe we should be using explicit language instead of a neutral one, so
>
> ---
> propsheet.c:69: Test failed: Unexpected template buffer size 508,
> resource size 270
> ---
>
> could mean we were supposed to pick up a different resource of size 254.

I tried FindResourceExA on Hebrew Windows 10 with all of the
following, and got a resource of size 250 every time:

MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL)
MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT)
MAKELANGID(LANG_NEUTRAL, SUBLANG_SYS_DEFAULT)
MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_DEFAULT)
MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_UNSPECIFIED)
MAKELANGID(LANG_NEUTRAL, SUBLANG_UI_CUSTOM_DEFAULT)
MAKELANGID(LANG_HEBREW, SUBLANG_HEBREW_ISRAEL)

Interestingly, 508 is exactly 2 times the US English resource size.
Maybe the RTL locales base their buffer size on the US English
resource? I checked and other languages such as German have different
resource and buffer sizes from English.

I also checked Kurdish, Persian, Punjabi, Urdu, and Uyghur Windows 10,
and resource sizes were the same for both the localized resource and
the US English resource. So, I couldn't tell whether they compute
buffer size based on the English resource or based on their own
resource.

> P.S. GetLocaleInfo() takes LCID not LANGID, and you can use special
> constants like LOCALE_USER_DEFAULT too.

OK, LOCALE_USER_DEFAULT should be fine. Out of curiosity, what is the
correct way to specify a LCID? Do I have to say something like
MAKELCID(MAKELANGID(LANG_ENGLISH, SUBLANG_ENGLISH_US), 0)?

More importantly, how do you think these tests should be written?
Should I use the US English resource size to determine the correct
buffer size on RTL locales, or should I just continue accepting sizes
within 32 bytes of expected? Also, should the different code path be
for all RTL locales, or just for Arabic and Hebrew specifically?

-Alex


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Nikolay Sivov-2
On 11/15/2017 09:05 AM, Alex Henrie wrote:

> 2017-11-14 0:40 GMT-07:00 Nikolay Sivov <[hidden email]>:
>> Is it possible we're picking wrong resource with FindResource() here?
>> Maybe we should be using explicit language instead of a neutral one, so
>>
>> ---
>> propsheet.c:69: Test failed: Unexpected template buffer size 508,
>> resource size 270
>> ---
>>
>> could mean we were supposed to pick up a different resource of size 254.
> I tried FindResourceExA on Hebrew Windows 10 with all of the
> following, and got a resource of size 250 every time:
>
> MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL)
> MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT)
> MAKELANGID(LANG_NEUTRAL, SUBLANG_SYS_DEFAULT)
> MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_DEFAULT)
> MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_UNSPECIFIED)
> MAKELANGID(LANG_NEUTRAL, SUBLANG_UI_CUSTOM_DEFAULT)
> MAKELANGID(LANG_HEBREW, SUBLANG_HEBREW_ISRAEL)
>
> Interestingly, 508 is exactly 2 times the US English resource size.
> Maybe the RTL locales base their buffer size on the US English
> resource? I checked and other languages such as German have different
> resource and buffer sizes from English.
>
> I also checked Kurdish, Persian, Punjabi, Urdu, and Uyghur Windows 10,
> and resource sizes were the same for both the localized resource and
> the US English resource. So, I couldn't tell whether they compute
> buffer size based on the English resource or based on their own
> resource.
That's confusing, how can they all be the same if we have a failing
test? What's picked up as neutral resource, that's a question I guess.
Also it sounds strange that localized strings don't affect template
size, but maybe strings are stored separately. If regardless of locale
it's always using US English size, that's what we should use too, still
it's interesting where size 270 comes from.

>
>> P.S. GetLocaleInfo() takes LCID not LANGID, and you can use special
>> constants like LOCALE_USER_DEFAULT too.
> OK, LOCALE_USER_DEFAULT should be fine. Out of curiosity, what is the
> correct way to specify a LCID? Do I have to say something like
> MAKELCID(MAKELANGID(LANG_ENGLISH, SUBLANG_ENGLISH_US), 0)?
>
> More importantly, how do you think these tests should be written?
> Should I use the US English resource size to determine the correct
> buffer size on RTL locales, or should I just continue accepting sizes
> within 32 bytes of expected? Also, should the different code path be
> for all RTL locales, or just for Arabic and Hebrew specifically?
Ideally we should test against actual exact size, using single path, and
not testing locale properties.
>
> -Alex
>
>



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alex Henrie
2017-11-14 23:21 GMT-07:00 Nikolay Sivov <[hidden email]>:

> On 11/15/2017 09:05 AM, Alex Henrie wrote:
>
>> 2017-11-14 0:40 GMT-07:00 Nikolay Sivov <[hidden email]>:
>>>
>>> Is it possible we're picking wrong resource with FindResource() here?
>>> Maybe we should be using explicit language instead of a neutral one, so
>>>
>>> ---
>>> propsheet.c:69: Test failed: Unexpected template buffer size 508,
>>> resource size 270
>>> ---
>>>
>>> could mean we were supposed to pick up a different resource of size 254.
>>
>> I tried FindResourceExA on Hebrew Windows 10 with all of the
>> following, and got a resource of size 250 every time:
>>
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL)
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT)
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_SYS_DEFAULT)
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_DEFAULT)
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_CUSTOM_UNSPECIFIED)
>> MAKELANGID(LANG_NEUTRAL, SUBLANG_UI_CUSTOM_DEFAULT)
>> MAKELANGID(LANG_HEBREW, SUBLANG_HEBREW_ISRAEL)
>>
>> Interestingly, 508 is exactly 2 times the US English resource size.
>> Maybe the RTL locales base their buffer size on the US English
>> resource? I checked and other languages such as German have different
>> resource and buffer sizes from English.
>>
>> I also checked Kurdish, Persian, Punjabi, Urdu, and Uyghur Windows 10,
>> and resource sizes were the same for both the localized resource and
>> the US English resource. So, I couldn't tell whether they compute
>> buffer size based on the English resource or based on their own
>> resource.
>
> That's confusing, how can they all be the same if we have a failing test?
> What's picked up as neutral resource, that's a question I guess. Also it
> sounds strange that localized strings don't affect template size, but maybe
> strings are stored separately. If regardless of locale it's always using US
> English size, that's what we should use too, still it's interesting where
> size 270 comes from.

I used Hebrew Windows for testing because it's simpler than Arabic:
Windows only recognizes one Hebrew dialect.

English: Resource size 254, buffer size 508
Arabic: Resource size 270, buffer size 508
Hebrew: Resource size 250, buffer size 508
German: Resource size 266, buffer size 532

>>> P.S. GetLocaleInfo() takes LCID not LANGID, and you can use special
>>> constants like LOCALE_USER_DEFAULT too.
>>
>> OK, LOCALE_USER_DEFAULT should be fine. Out of curiosity, what is the
>> correct way to specify a LCID? Do I have to say something like
>> MAKELCID(MAKELANGID(LANG_ENGLISH, SUBLANG_ENGLISH_US), 0)?
>>
>> More importantly, how do you think these tests should be written?
>> Should I use the US English resource size to determine the correct
>> buffer size on RTL locales, or should I just continue accepting sizes
>> within 32 bytes of expected? Also, should the different code path be
>> for all RTL locales, or just for Arabic and Hebrew specifically?
>
> Ideally we should test against actual exact size, using single path, and not
> testing locale properties.

Considering the numbers I gave above, how can we have only a single path?

-Alex


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alexandre Julliard
Alex Henrie <[hidden email]> writes:

> I used Hebrew Windows for testing because it's simpler than Arabic:
> Windows only recognizes one Hebrew dialect.
>
> English: Resource size 254, buffer size 508
> Arabic: Resource size 270, buffer size 508
> Hebrew: Resource size 250, buffer size 508
> German: Resource size 266, buffer size 532

Is it actually using the Hebrew resource though? Maybe it needs the
propsheet pages to be RTL or something like that.

--
Alexandre Julliard
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alex Henrie
2017-11-15 11:46 GMT-07:00 Alexandre Julliard <[hidden email]>:

> Alex Henrie <[hidden email]> writes:
>
>> I used Hebrew Windows for testing because it's simpler than Arabic:
>> Windows only recognizes one Hebrew dialect.
>>
>> English: Resource size 254, buffer size 508
>> Arabic: Resource size 270, buffer size 508
>> Hebrew: Resource size 250, buffer size 508
>> German: Resource size 266, buffer size 532
>
> Is it actually using the Hebrew resource though? Maybe it needs the
> propsheet pages to be RTL or something like that.

Interesting, I think you're right: If I call
SetProcessDefaultLayout(LAYOUT_RTL) at the beginning of the tests then
the buffer size tests pass. This suggests three possible solutions:

1. Call SetProcessDefaultLayout(LAYOUT_RTL) before any of the tests
run, and change test_buttons to expect the buttons to be in reverse
order if on an RTL locale.

2. Call SetProcessDefaultLayout(LAYOUT_RTL) before each buffer size
test, and restore the original setting after each of those tests
finishes.

3. Check for an RTL locale in sheet_callback and expect a US English
resource if the locale is RTL but the test program is LTR.

Which of these solutions is preferred?

-Alex


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] comctl32/tests: Fix propsheet template buffer size test on RTL locales

Alexandre Julliard
Alex Henrie <[hidden email]> writes:

> 2017-11-15 11:46 GMT-07:00 Alexandre Julliard <[hidden email]>:
>> Alex Henrie <[hidden email]> writes:
>>
>>> I used Hebrew Windows for testing because it's simpler than Arabic:
>>> Windows only recognizes one Hebrew dialect.
>>>
>>> English: Resource size 254, buffer size 508
>>> Arabic: Resource size 270, buffer size 508
>>> Hebrew: Resource size 250, buffer size 508
>>> German: Resource size 266, buffer size 532
>>
>> Is it actually using the Hebrew resource though? Maybe it needs the
>> propsheet pages to be RTL or something like that.
>
> Interesting, I think you're right: If I call
> SetProcessDefaultLayout(LAYOUT_RTL) at the beginning of the tests then
> the buffer size tests pass. This suggests three possible solutions:
>
> 1. Call SetProcessDefaultLayout(LAYOUT_RTL) before any of the tests
> run, and change test_buttons to expect the buttons to be in reverse
> order if on an RTL locale.

This one is probably the best, it's the one nearest to normal
application usage.

--
Alexandre Julliard
[hidden email]