[PATCH] comctl32/treeview: Make textWidth in struct _TREEITEM compatible with native one.
Hi Nikolay,
This fixes https://bugs.winehq.org/show_bug.cgi?id=35127
As I was working through bug 35127, I noticed that you had once changed the layout of struct _TREEITEM in commit 6ec621e835e03f715d5fee27c5de5b0d361814de.
Now I am sure that textWidth in struct _TREEITEM is a WORD and is at the offset 0x1a. You can check out the bug comments for details. I tried to find out what is at 0x18 but no luck, thus I am adding a unknown member to make textWidth at the right offset.
What do you think of the patch? Do you know what member is at struct _TREEITEM+0x18? I can use your opinion with this patch.
Thanks, Zhiyi Zhang
Signed-off-by: Zhiyi Zhang yi.gd.cn@gmail.com --- dlls/comctl32/tests/treeview.c | 58 ++++++++++++++++++++++++++++++++++++------ dlls/comctl32/treeview.c | 3 ++- 2 files changed, 52 insertions(+), 9 deletions(-)
Sorry for the inconvenience, I just found out there is something wrong with the tests thus the whole patch.
I will resend another one later. The patch should not be accepted.
On Sat, Jan 20, 2018 at 1:40 PM, Zhiyi Zhang yi.gd.cn@gmail.com wrote:
Hi Nikolay,
This fixes https://bugs.winehq.org/show_bug.cgi?id=35127
As I was working through bug 35127, I noticed that you had once changed the layout of struct _TREEITEM in commit 6ec621e835e03f715d5fee27c5de5b 0d361814de.
Now I am sure that textWidth in struct _TREEITEM is a WORD and is at the offset 0x1a. You can check out the bug comments for details. I tried to find out what is at 0x18 but no luck, thus I am adding a unknown member to make textWidth at the right offset.
What do you think of the patch? Do you know what member is at struct _TREEITEM+0x18? I can use your opinion with this patch.
Thanks, Zhiyi Zhang
Signed-off-by: Zhiyi Zhang yi.gd.cn@gmail.com
dlls/comctl32/tests/treeview.c | 58 ++++++++++++++++++++++++++++++ ++++++------ dlls/comctl32/treeview.c | 3 ++- 2 files changed, 52 insertions(+), 9 deletions(-)
On 01/20/2018 08:40 AM, Zhiyi Zhang wrote:
Hi Nikolay,
This fixes https://bugs.winehq.org/show_bug.cgi?id=35127
As I was working through bug 35127, I noticed that you had once changed the layout of struct _TREEITEM in commit 6ec621e835e03f715d5fee27c5de5b0d361814de.
Now I am sure that textWidth in struct _TREEITEM is a WORD and is at the offset 0x1a. You can check out the bug comments for details. I tried to find out what is at 0x18 but no luck, thus I am adding a unknown member to make textWidth at the right offset.
What do you think of the patch? Do you know what member is at struct _TREEITEM+0x18? I can use your opinion with this patch.
struct·_ITEM_DATA { -· HTREEITEM parent; /* for root value of parent field is unidetified */ +· HTREEITEM parent; /* for root value of parent field is unidentified */ · HTREEITEM nextsibling; · HTREEITEM firstchild; +· UINT unknownField0; /* untested */ +· UINT unknownField1; /* untested */ +· UINT unknownField2; /* untested */ +· WORD padding0; /* unknown field to make textWidth offset compatible with the native one */ +· WORD textWidth; /* horizontal text extent for pszText */ };
They are all unknown, so a single unknown[7] is better, assuming this will work on 64 bits.
+· /* padding0 should be unused on Wine */ +· if(!strcmp(winetest_platform, "wine")) +· { +· ok_(__FILE__, line)(data->padding0 == 0, "padding0 %04x, got %04x\n", 0, data->padding0); +· ok_(__FILE__, line)(data->textWidth == textWidth, "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· } +· /* TODO: textWidth computation on Windows is alway that of Wine plus 4, remove this test when it got corrected. +· Also, in Windows textWidth is calculated even if tree item is invisible whereas in Wine is postponed */ +· else +· { +· ok_(__FILE__, line)(textWidth == 0 || data->textWidth == 0 || data->textWidth == textWidth + 4, +· "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· }
This is very dirty. You should get rid of platform check; testing padding0 does not look useful; and last test line looks suspicious, if expected width is 0 nothing gets tested.
+· /* Set font to make textWidth calculation consistent */ +· hdc=GetDC(hMainWnd); +· SystemParametersInfoA(SPI_GETICONTITLELOGFONT, sizeof(lf), &lf, 0); +· SelectObject(hdc, CreateFontIndirectA(&lf));
This doesn't make it consistent, because you select one font, and control could be using another one, also you're using parent window for some reason. WM_GETFONT returns what you need.
Instead of calculating this width manually, it's better to use TVM_GETITEMRECT (with wparam == 1), if that gives expected width. If text portion rectangle width returned by this message matches internal field value on Windows, then we should use that. This will also hide +4 difference you see, unless it was caused by a wrong font.
typedef·struct _TREEITEM /* HTREEITEM is a _TREEINFO *. */ · UINT callbackMask; · UINT state; · UINT stateMask; +· WORD padding0; /* unknown field to make textWidth offset compatible with the native one */ +· WORD textWidth; /* horizontal text extent for pszText */
Instead this should match structure used in tests, i.e. it should start with same fields and not have incompatible data mixed with what we try to match.
+· /* padding0 should be unused on Wine */ +· if(!strcmp(winetest_platform, "wine")) +· { +· ok_(__FILE__, line)(data->padding0 == 0, "padding0 %04x, got %04x\n", 0, data->padding0); +· ok_(__FILE__, line)(data->textWidth == textWidth, "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· } +· /* TODO: textWidth computation on Windows is alway that of Wine plus 4, remove this test when it got corrected. +· Also, in Windows textWidth is calculated even if tree item is invisible whereas in Wine is postponed */ +· else +· { +· ok_(__FILE__, line)(textWidth == 0 || data->textWidth == 0 || data->textWidth == textWidth + 4, +· "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· }
+ This is very dirty. You should get rid of platform check; testing padding0 does not look useful; and last test line looks suspicious, if expected width is 0 nothing gets tested.
You're right, that what I later found out what's wrong with the tests. Thanks for the review.
On Sun, Jan 21, 2018 at 12:45 AM, Nikolay Sivov nsivov@codeweavers.com wrote:
On 01/20/2018 08:40 AM, Zhiyi Zhang wrote:
Hi Nikolay,
This fixes https://bugs.winehq.org/show_bug.cgi?id=35127
As I was working through bug 35127, I noticed that you had once changed the layout of struct _TREEITEM in commit 6ec621e835e03f715d5fee27c5de5b 0d361814de.
Now I am sure that textWidth in struct _TREEITEM is a WORD and is at the offset 0x1a. You can check out the bug comments for details. I tried to find out what is at 0x18 but no luck, thus I am adding a unknown member to make textWidth at the right offset.
What do you think of the patch? Do you know what member is at struct _TREEITEM+0x18? I can use your opinion with this patch.
struct·_ITEM_DATA { -· HTREEITEM parent; /* for root value of parent field is unidetified */ +· HTREEITEM parent; /* for root value of parent field is unidentified */ · HTREEITEM nextsibling; · HTREEITEM firstchild; +· UINT unknownField0; /* untested */ +· UINT unknownField1; /* untested */ +· UINT unknownField2; /* untested */ +· WORD padding0; /* unknown field to make textWidth offset compatible with the native one */ +· WORD textWidth; /* horizontal text extent for pszText */ };
They are all unknown, so a single unknown[7] is better, assuming this will work on 64 bits.
+· /* padding0 should be unused on Wine */
+· if(!strcmp(winetest_platform, "wine")) +· { +· ok_(__FILE__, line)(data->padding0 == 0, "padding0 %04x, got %04x\n", 0, data->padding0); +· ok_(__FILE__, line)(data->textWidth == textWidth, "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· } +· /* TODO: textWidth computation on Windows is alway that of Wine plus 4, remove this test when it got corrected. +· Also, in Windows textWidth is calculated even if tree item is invisible whereas in Wine is postponed */ +· else +· { +· ok_(__FILE__, line)(textWidth == 0 || data->textWidth == 0 || data->textWidth == textWidth + 4, +· "textWidth %04x, got %04x\n", textWidth, data->textWidth); +· }
This is very dirty. You should get rid of platform check; testing padding0 does not look useful; and last test line looks suspicious, if expected width is 0 nothing gets tested.
+· /* Set font to make textWidth calculation consistent */ +· hdc=GetDC(hMainWnd); +· SystemParametersInfoA(SPI_GETICONTITLELOGFONT, sizeof(lf), &lf, 0); +· SelectObject(hdc, CreateFontIndirectA(&lf));
This doesn't make it consistent, because you select one font, and control could be using another one, also you're using parent window for some reason. WM_GETFONT returns what you need.
Instead of calculating this width manually, it's better to use TVM_GETITEMRECT (with wparam == 1), if that gives expected width. If text portion rectangle width returned by this message matches internal field value on Windows, then we should use that. This will also hide +4 difference you see, unless it was caused by a wrong font.
typedef·struct _TREEITEM /* HTREEITEM is a _TREEINFO *. */
· UINT callbackMask; · UINT state; · UINT stateMask; +· WORD padding0; /* unknown field to make textWidth offset compatible with the native one */ +· WORD textWidth; /* horizontal text extent for pszText */
Instead this should match structure used in tests, i.e. it should start with same fields and not have incompatible data mixed with what we try to match.
-
Eric Zhang -
Nikolay Sivov -
Zhiyi Zhang