Re: comctl32/listview: fix icon spacing calculation [try 2]
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 http://testbot.winehq.org/JobDetails.pl?Key=23921 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: comctl32/listview: fix icon spacing calculation [try 2]
On 1/15/2013 00:53, Daniel Jelinski wrote: +if(lParam == -1) + return LISTVIEW_SetIconSpacing(infoPtr, -1, -1); +return LISTVIEW_SetIconSpacing(infoPtr, LOWORD(lParam), HIWORD(lParam)); Why do you need to handle this case specially? If it's -1 for 64bit comctl32 it should give you the same values with HIWORD/LOWORD as it does for 32bit.
Re: comctl32/listview: fix icon spacing calculation [try 2]
2013/1/14 Nikolay Sivov bungleh...@gmail.com: On 1/15/2013 00:53, Daniel Jelinski wrote: +if(lParam == -1) + return LISTVIEW_SetIconSpacing(infoPtr, -1, -1); +return LISTVIEW_SetIconSpacing(infoPtr, LOWORD(lParam), HIWORD(lParam)); Why do you need to handle this case specially? If it's -1 for 64bit comctl32 it should give you the same values with HIWORD/LOWORD as it does for 32bit. On 64bit the behavior with lParam=0x is different from that with lParam=-1. HIWORD/LOWORD values are the same, so without this special case they would behave the same way.
Re: comctl32/listview: fix icon spacing calculation [try 2]
On 1/15/2013 01:15, Daniel Jelinski wrote: 2013/1/14 Nikolay Sivov bungleh...@gmail.com: On 1/15/2013 00:53, Daniel Jelinski wrote: +if(lParam == -1) + return LISTVIEW_SetIconSpacing(infoPtr, -1, -1); +return LISTVIEW_SetIconSpacing(infoPtr, LOWORD(lParam), HIWORD(lParam)); Why do you need to handle this case specially? If it's -1 for 64bit comctl32 it should give you the same values with HIWORD/LOWORD as it does for 32bit. On 64bit the behavior with lParam=0x is different from that with lParam=-1. HIWORD/LOWORD values are the same, so without this special case they would behave the same way. So on 64bit you want to distinguish two cases: - ~0 value of lParam - you use it to reset to default values; - all other values including 0x that will result in the same call with both args being -1. So result is the same, right? Your test: +#ifdef _WIN64 +ret = SendMessage(hwnd, LVM_SETICONSPACING, 0, 0xBAADF00DDEADBEEFLL); +expect(0x, LOWORD(ret)); +expect(0x, HIWORD(ret)); +ret2 = SendMessage(hwnd, LVM_GETITEMSPACING, FALSE, 0); +ok((LONG)0xDEADBEEF == ret2, Expected DEADBEEF, got %p\n, (void*)ret2); +ret2 = SendMessage(hwnd, LVM_SETICONSPACING, 0, -1); +ok(0xDEADBEEF == ret2, Expected DEADBEEF, got %p\n, (void*)ret2); +#endif shows that higher DWORD is simply ignored, so value -1 and MAKELONG(-1, -1) should give same results.
Re: comctl32/listview: fix icon spacing calculation [try 2]
2013/1/14 Nikolay Sivov bungleh...@gmail.com: So on 64bit you want to distinguish two cases: - ~0 value of lParam - you use it to reset to default values; - all other values including 0x that will result in the same call with both args being -1. Unless I got something wrong, all other values including 0x will result in a call with both args being 65535. Since parameters are INTs now, 65535 != -1. The tests show that: SendMessage(hwnd, LVM_SETICONSPACING, 0, MAKELPARAM(-1,-1)); results in default spacing on 32bit and (65535,65535) on 64bit. MAKELPARAM(-1,-1) == (DWORD)-1 MAKELONG(-1,-1) == -1
Re: comctl32/listview: fix icon spacing calculation [try 2]
On 1/15/2013 01:43, Daniel Jelinski wrote: 2013/1/14 Nikolay Sivov bungleh...@gmail.com: So on 64bit you want to distinguish two cases: - ~0 value of lParam - you use it to reset to default values; - all other values including 0x that will result in the same call with both args being -1. Unless I got something wrong, all other values including 0x will result in a call with both args being 65535. Since parameters are INTs now, 65535 != -1. The tests show that: SendMessage(hwnd, LVM_SETICONSPACING, 0, MAKELPARAM(-1,-1)); results in default spacing on 32bit and (65535,65535) on 64bit. MAKELPARAM(-1,-1) == (DWORD)-1 MAKELONG(-1,-1) == -1 This message actually is supposed to be used with MAKELONG(), so it's not truncated in such way.
Re: comctl32/listview: fix icon spacing calculation [try 2]
2013/1/14 Nikolay Sivov bungleh...@gmail.com: This message actually is supposed to be used with MAKELONG(), so it's not truncated in such way. Probably (MSDN doesn't say a word about the preferred macro for this function, unless you count user comments), however if any app actually calls MAKELPARAM, at least it will get the same result as on Windows.
Re: comctl32/listview: fix icon spacing calculation [try 2]
On 1/15/2013 01:53, Daniel Jelinski wrote: 2013/1/14 Nikolay Sivov bungleh...@gmail.com: This message actually is supposed to be used with MAKELONG(), so it's not truncated in such way. Probably (MSDN doesn't say a word about the preferred macro for this function, unless you count user comments), however if any app actually calls MAKELPARAM, at least it will get the same result as on Windows. It's a way it's used in ListView_SetIconSpacing() (which could be broken in its own way of course). I think it's ok to ignore this 64 bit case for now, especially while we don't get daily test runs.
Re: comctl32/listview: fix icon spacing calculation [try 2]
2013/1/14 Nikolay Sivov bungleh...@gmail.com: It's a way it's used in ListView_SetIconSpacing() (which could be broken in its own way of course). I think it's ok to ignore this 64 bit case for now, especially while we don't get daily test runs. Are you saying that I should drop this? -return LISTVIEW_SetIconSpacing(infoPtr, (short)LOWORD(lParam), (short)HIWORD(lParam)); +if(lParam == -1) + return LISTVIEW_SetIconSpacing(infoPtr, -1, -1); +return LISTVIEW_SetIconSpacing(infoPtr, LOWORD(lParam), HIWORD(lParam)); This would net us 2 more todo_wine tests. Also, what's up with the daily test runs? Will they be fixed any time soon?
Re: comctl32/listview: fix icon spacing calculation
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 http://testbot.winehq.org/JobDetails.pl?Key=23897 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: comctl32/listview: fix icon spacing calculation
On 1/14/2013 00:00, Daniel Jelinski wrote:
-static DWORD LISTVIEW_SetIconSpacing(LISTVIEW_INFO *infoPtr, INT cx, INT cy)
+static DWORD LISTVIEW_SetIconSpacing(LISTVIEW_INFO *infoPtr, WORD cx, WORD cy)
I don't really like it as WORD, what it really is a int value with
special meaning reserved for -1.
For 64bits and lParam == -1 you'll still get two -1 values.
+/* LPARAM = -1 - restore default processing */
+ret = SendMessage(hwnd, LVM_SETICONSPACING, 0, MAKELPARAM(-1,-1));
+todo_wine
+expect(100, LOWORD(ret));
+expect(0x, HIWORD(ret));
+
+/* NOTE: -1 is not the same as MAKELPARAM(-1,-1) in 64bit listview */
+if(sizeof(LPARAM) == 8)
+{
+ret = SendMessage(hwnd, LVM_SETICONSPACING, 0, -1);
+todo_wine {
+expect(0x, LOWORD(ret));
+expect(0x, HIWORD(ret));
+}
+}
That means you should just use lParam == -1. Also this message is
supposed to use MAKELONG apparently, not sure if it makes a difference.
