[PATCH v2 resend 0/6] Fix LVS_OWNERDATA list view multi select.
From: Angelo Haller <angelo(a)szanni.org> The following patches fix sending of the LVN_ODSTATECHANGED notification for LVS_OWNERDATA list views. A few more refined tests have been added, some wine_todos removed, and bugs fixed. This is a v2 resend. Warning: I have had access to the Windows Research Kernel (WRK) 1.2 ~10 years ago. These changes are regarding comctrl32 & tests which are NOT part of the WRK. As outlined in https://wiki.winehq.org/Developer_FAQ this should therefore satisfy the requirement of ONLY submitting patches to components I have NOT had access to. Angelo Haller (6): comctl32/tests: Expand ownerdata listview tests. comctl32/listview: Move LVN_ODSTATECHANGED notification to function. comctl32/listview: Send LVN_ODSTATECHANGED only for virtual lists. comctl32/listview: Send LVN_ODSTATECHANGED notification. comctl32/listview: Send LVN_ODSTATECHANGED only for true ranges. comctl32/listview: Fix deselect on LVS_OWNERDATA. dlls/comctl32/listview.c | 55 +++++++++++++++++++++---------- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 2 files changed, 89 insertions(+), 25 deletions(-) Signed-off-by: Angelo Haller <angelo(a)szanni.org> -- 2.36.0
From: Angelo Haller <angelo(a)szanni.org> Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs. Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } }; -static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } }; @@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res); + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); + + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); } -- 2.36.0
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } };
-static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } };
@@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res);
+ flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); Hi Angelo,
Thanks for looking into this. There are a few things can be improved for this series. Please complete the keyboard sequence by sending WM_KEYUP for VK_DOWN and WM_KEYUP for VK_UP. Also please add tests for pressing VK_UP while holding Shift after setting pressing VK_DOWN while holding Shift. Same for holding both Shift and Control And add checks for NMLVODSTATECHANGE member values. For example, I don't think uOldState should always be 0.
+ + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
On 10/06/2022 03.12, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } };
-static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } };
@@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res);
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res);
+ flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); Hi Angelo,
Thanks for looking into this. There are a few things can be improved for this series.
Please complete the keyboard sequence by sending WM_KEYUP for VK_DOWN and WM_KEYUP for VK_UP.
I'll add the WM_KEYUP events for sure. Not sure how that slipped.
Also please add tests for pressing VK_UP while holding Shift after setting pressing VK_DOWN while holding Shift. Same for holding both Shift and Control And add checks for NMLVODSTATECHANGE member values. For example, I don't think uOldState should always be 0.
Happy to add more tests for VK_UP. With regards to the uOldState: I have not found an instance where it is not 0. Which kind of makes sense, at least according to the underlying logic. Windows seems to only use LVN_ODSTATECHANGED to set a new state (selection of multiple items). For un-setting (uOldState != 0) Windows will instead send an all items changed (LVN_ITEMCHANGED = -1) signal instead. That is at least what I have observed in my testing.
+ + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
On 6/11/22 02:33, Angelo Haller wrote:
On 10/06/2022 03.12, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } }; -static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } }; @@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res); + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); Hi Angelo,
Thanks for looking into this. There are a few things can be improved for this series.
Please complete the keyboard sequence by sending WM_KEYUP for VK_DOWN and WM_KEYUP for VK_UP.
I'll add the WM_KEYUP events for sure. Not sure how that slipped.
Also please add tests for pressing VK_UP while holding Shift after setting pressing VK_DOWN while holding Shift. Same for holding both Shift and Control And add checks for NMLVODSTATECHANGE member values. For example, I don't think uOldState should always be 0.
Happy to add more tests for VK_UP.
With regards to the uOldState: I have not found an instance where it is not 0. Which kind of makes sense, at least according to the underlying logic.
Windows seems to only use LVN_ODSTATECHANGED to set a new state (selection of multiple items). For un-setting (uOldState != 0) Windows will instead send an all items changed (LVN_ITEMCHANGED = -1) signal instead. That is at least what I have observed in my testing.
I see. Please add that tests as well.
+ + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
On 11/06/2022 02.57, Zhiyi Zhang wrote:
On 6/11/22 02:33, Angelo Haller wrote:
On 10/06/2022 03.12, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } }; -static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } }; @@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res); + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); Hi Angelo,
Thanks for looking into this. There are a few things can be improved for this series.
Please complete the keyboard sequence by sending WM_KEYUP for VK_DOWN and WM_KEYUP for VK_UP. I'll add the WM_KEYUP events for sure. Not sure how that slipped. Also please add tests for pressing VK_UP while holding Shift after setting pressing VK_DOWN while holding Shift. Same for holding both Shift and Control And add checks for NMLVODSTATECHANGE member values. For example, I don't think uOldState should always be 0. Happy to add more tests for VK_UP.
With regards to the uOldState: I have not found an instance where it is not 0. Which kind of makes sense, at least according to the underlying logic.
Windows seems to only use LVN_ODSTATECHANGED to set a new state (selection of multiple items). For un-setting (uOldState != 0) Windows will instead send an all items changed (LVN_ITEMCHANGED = -1) signal instead. That is at least what I have observed in my testing. I see. Please add that tests as well.
That test is in this exact patch. This is why I added ownerdata_multiselect_deselect_all_select_3_seq. It checks that on deslection of multiple items NO LVN_ODSTATECHANGED is send but instead LVN_ITEMCHANGED = -1. Or did i misunderstand?
+ + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
On 6/11/22 23:26, Angelo Haller wrote:
On 11/06/2022 02.57, Zhiyi Zhang wrote:
On 6/11/22 02:33, Angelo Haller wrote:
On 10/06/2022 03.12, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Add more test cases to ownderdata listviews. Check LVN_ITEMCHANGED IDs.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/tests/listview.c | 59 +++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 6ac7f53137d..78b3e3ae069 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -255,11 +255,33 @@ static const struct message ownerdata_deselect_all_parent_seq[] = { { 0 } }; -static const struct message ownerdata_multiselect_odstatechanged_seq[] = { - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, +static const struct message ownerdata_multiselect_select_0_to_1_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, - { WM_NOTIFY, sent|id, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 0, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_select_0_to_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id, 0, 0, LVN_ODSTATECHANGED }, + { WM_NOTIFY, sent|id|wparam, 1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_all_select_3_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { 0 } +}; + +static const struct message ownerdata_multiselect_deselect_3_select_2_seq[] = { + { WM_NOTIFY, sent|id|wparam, -1, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 3, 0, LVN_ITEMCHANGED }, + { WM_NOTIFY, sent|id|wparam, 2, 0, LVN_ITEMCHANGED }, { 0 } }; @@ -3575,8 +3597,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_1_seq, + "ownerdata multiselect: select multiple via SHIFT", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3592,8 +3614,8 @@ static void test_ownerdata_multiselect(void) expect(0, res); ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, - ownerdata_multiselect_odstatechanged_seq, - "ownerdata select multiple notification", TRUE); + ownerdata_multiselect_select_0_to_2_seq, + "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3604,6 +3626,27 @@ static void test_ownerdata_multiselect(void) res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(3, res); + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_DOWN, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_all_select_3_seq, + "ownerdata multiselect: deselect all, select item 3", TRUE); + + res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); + expect(1, res); Hi Angelo,
Thanks for looking into this. There are a few things can be improved for this series.
Please complete the keyboard sequence by sending WM_KEYUP for VK_DOWN and WM_KEYUP for VK_UP. I'll add the WM_KEYUP events for sure. Not sure how that slipped. Also please add tests for pressing VK_UP while holding Shift after setting pressing VK_DOWN while holding Shift. Same for holding both Shift and Control And add checks for NMLVODSTATECHANGE member values. For example, I don't think uOldState should always be 0. Happy to add more tests for VK_UP.
With regards to the uOldState: I have not found an instance where it is not 0. Which kind of makes sense, at least according to the underlying logic.
Windows seems to only use LVN_ODSTATECHANGED to set a new state (selection of multiple items). For un-setting (uOldState != 0) Windows will instead send an all items changed (LVN_ITEMCHANGED = -1) signal instead. That is at least what I have observed in my testing. I see. Please add that tests as well.
That test is in this exact patch. This is why I added ownerdata_multiselect_deselect_all_select_3_seq. It checks that on deslection of multiple items NO LVN_ODSTATECHANGED is send but instead LVN_ITEMCHANGED = -1.
Or did i misunderstand?
ownerdata_multiselect_deselect_all_select_3_seq is tested with Shift key released. It would be helpful to see the deselect message sequence while holding Shift key.
+ + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + res = SendMessageA(hwnd, WM_KEYDOWN, VK_UP, 0); + expect(0, res); + + ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, + ownerdata_multiselect_deselect_3_select_2_seq, + "ownerdata multiselect: deselect item 3, select item 2", TRUE); + DestroyWindow(hwnd); }
From: Angelo Haller <angelo(a)szanni.org> Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- v2: Remove header and change return to VOID. --- dlls/comctl32/listview.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 730bf4aaddd..72ade724313 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -437,6 +437,7 @@ static INT LISTVIEW_GetStringWidthT(const LISTVIEW_INFO *, LPCWSTR, BOOL); static BOOL LISTVIEW_KeySelection(LISTVIEW_INFO *, INT, BOOL); static UINT LISTVIEW_GetItemState(const LISTVIEW_INFO *, INT, UINT); static BOOL LISTVIEW_SetItemState(LISTVIEW_INFO *, INT, const LVITEMW *); +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *, INT, INT, const LVITEMW *); static LRESULT LISTVIEW_VScroll(LISTVIEW_INFO *, INT, INT); static LRESULT LISTVIEW_HScroll(LISTVIEW_INFO *, INT, INT); static BOOL LISTVIEW_EnsureVisible(LISTVIEW_INFO *, INT, BOOL); @@ -3557,16 +3558,11 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) INT nFirst = min(infoPtr->nSelectionMark, nItem); INT nLast = max(infoPtr->nSelectionMark, nItem); HWND hwndSelf = infoPtr->hwndSelf; - NMLVODSTATECHANGE nmlv; DWORD old_mask; LVITEMW item; INT i; - /* Temporarily disable change notification - * If the control is LVS_OWNERDATA, we need to send - * only one LVN_ODSTATECHANGED notification. - * See MSDN documentation for LVN_ITEMCHANGED. - */ + /* disable per item notifications on LVS_OWNERDATA style */ old_mask = infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->notify_mask &= ~NOTIFY_MASK_ITEM_CHANGE; @@ -3579,13 +3575,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) for (i = nFirst; i <= nLast; i++) LISTVIEW_SetItemState(infoPtr,i,&item); - ZeroMemory(&nmlv, sizeof(nmlv)); - nmlv.iFrom = nFirst; - nmlv.iTo = nLast; - nmlv.uOldState = 0; - nmlv.uNewState = item.state; + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); - notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); if (!IsWindow(hwndSelf)) return FALSE; infoPtr->notify_mask |= old_mask; @@ -8950,6 +8941,22 @@ static BOOL LISTVIEW_SetItemPosition(LISTVIEW_INFO *infoPtr, INT nItem, const PO return LISTVIEW_MoveIconTo(infoPtr, nItem, &Pt, FALSE); } +/* Make sure to also disable per item notifications via the notification mask. */ +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{ + NMLVODSTATECHANGE nmlv; + + if (!item) return; + + ZeroMemory(&nmlv, sizeof(nmlv)); + nmlv.iFrom = nFirst; + nmlv.iTo = nLast; + nmlv.uOldState = 0; + nmlv.uNewState = item->state; + + notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); +} + /*** * DESCRIPTION: * Sets the state of one or many items. -- 2.36.0
Let's use "comctl32/listview: Move sending LVN_ODSTATECHANGED notifications to a function." for the subject. On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- v2: Remove header and change return to VOID. --- dlls/comctl32/listview.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 730bf4aaddd..72ade724313 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -437,6 +437,7 @@ static INT LISTVIEW_GetStringWidthT(const LISTVIEW_INFO *, LPCWSTR, BOOL); static BOOL LISTVIEW_KeySelection(LISTVIEW_INFO *, INT, BOOL); static UINT LISTVIEW_GetItemState(const LISTVIEW_INFO *, INT, UINT); static BOOL LISTVIEW_SetItemState(LISTVIEW_INFO *, INT, const LVITEMW *); +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *, INT, INT, const LVITEMW *); static LRESULT LISTVIEW_VScroll(LISTVIEW_INFO *, INT, INT); static LRESULT LISTVIEW_HScroll(LISTVIEW_INFO *, INT, INT); static BOOL LISTVIEW_EnsureVisible(LISTVIEW_INFO *, INT, BOOL); @@ -3557,16 +3558,11 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) INT nFirst = min(infoPtr->nSelectionMark, nItem); INT nLast = max(infoPtr->nSelectionMark, nItem); HWND hwndSelf = infoPtr->hwndSelf; - NMLVODSTATECHANGE nmlv; DWORD old_mask; LVITEMW item; INT i;
- /* Temporarily disable change notification - * If the control is LVS_OWNERDATA, we need to send - * only one LVN_ODSTATECHANGED notification. - * See MSDN documentation for LVN_ITEMCHANGED. - */ + /* disable per item notifications on LVS_OWNERDATA style */
Nitpick. disable -> Disable. There are other places like this.
old_mask = infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->notify_mask &= ~NOTIFY_MASK_ITEM_CHANGE; @@ -3579,13 +3575,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) for (i = nFirst; i <= nLast; i++) LISTVIEW_SetItemState(infoPtr,i,&item);
- ZeroMemory(&nmlv, sizeof(nmlv)); - nmlv.iFrom = nFirst; - nmlv.iTo = nLast; - nmlv.uOldState = 0; - nmlv.uNewState = item.state; + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item);
- notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); if (!IsWindow(hwndSelf)) return FALSE; infoPtr->notify_mask |= old_mask; @@ -8950,6 +8941,22 @@ static BOOL LISTVIEW_SetItemPosition(LISTVIEW_INFO *infoPtr, INT nItem, const PO return LISTVIEW_MoveIconTo(infoPtr, nItem, &Pt, FALSE); }
+/* Make sure to also disable per item notifications via the notification mask. */ +static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT nLast, const LVITEMW *item) +{ + NMLVODSTATECHANGE nmlv; + + if (!item) return; + + ZeroMemory(&nmlv, sizeof(nmlv)); + nmlv.iFrom = nFirst; + nmlv.iTo = nLast; + nmlv.uOldState = 0; + nmlv.uNewState = item->state; + + notify_hdr(infoPtr, LVN_ODSTATECHANGED, (LPNMHDR)&nmlv); +} + /*** * DESCRIPTION: * Sets the state of one or many items.
From: Angelo Haller <angelo(a)szanni.org> The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set. Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv)); -- 2.36.0
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv;
+ if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return;
Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix.
if (!item) return;
ZeroMemory(&nmlv, sizeof(nmlv));
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv;
+ if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix.
Is this strictly necessary? The call site has special handling for: infoPtr->dwStyle & LVS_OWNERDATA We could guard the call to the function at the call site, if that is preferred. Apart from that I am not sure how to write a test for this. This bug is triggered by creating a non ownerdata list and selecting multiple entries holding shift+ctrl and clicking with the mouse. This will send an LVN_ODSTATECHANGED notification where it is not supposed to (as it is not an ownerdata list). I have not found any test examples with mouse emulation. Maybe I missed something. Or should I open a separate bug report for this line?
if (!item) return;
ZeroMemory(&nmlv, sizeof(nmlv));
On 6/11/22 07:56, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix.
Is this strictly necessary? The call site has special handling for: infoPtr->dwStyle & LVS_OWNERDATA
We could guard the call to the function at the call site, if that is preferred.
Apart from that I am not sure how to write a test for this. This bug is triggered by creating a non ownerdata list and selecting multiple entries holding shift+ctrl and clicking with the mouse. This will send an LVN_ODSTATECHANGED notification where it is not supposed to (as it is not an ownerdata list).
You can write a test that demonstrate the exact same thing. Create a non onwerdata listview control and test the message sequence doesn't contain LVN_ODSTATECHANGED, which should have a todo_wine because Wine is currently broken in this case. Then you fix it in the next patch and removes the todo_wine. As for the mouse emulation, could you do it using only the keyboard? Such as sending VK_DOWN while holding Shift and Ctrl?
I have not found any test examples with mouse emulation. Maybe I missed something.
Or should I open a separate bug report for this line?
if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv));
On 11/06/2022 03.02, Zhiyi Zhang wrote:
On 6/11/22 07:56, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix. Is this strictly necessary? The call site has special handling for: infoPtr->dwStyle & LVS_OWNERDATA
We could guard the call to the function at the call site, if that is preferred.
Apart from that I am not sure how to write a test for this. This bug is triggered by creating a non ownerdata list and selecting multiple entries holding shift+ctrl and clicking with the mouse. This will send an LVN_ODSTATECHANGED notification where it is not supposed to (as it is not an ownerdata list). You can write a test that demonstrate the exact same thing. Create a non onwerdata listview control and test the message sequence doesn't contain LVN_ODSTATECHANGED, which should have a todo_wine because Wine is currently broken in this case. Then you fix it in the next patch and removes the todo_wine.
As for the mouse emulation, could you do it using only the keyboard? Such as sending VK_DOWN while holding Shift and Ctrl?
This is exactly what I was trying to communicate. I can NOT trigger the erroneous sending of LVN_ODSTATECHANGED via the keyboard. The bug is in LISTVIEW_AddGroupSelection which gets called from LISTVIEW_LButtonDown. Patch 2/6 moves the offending code to a new function LISTVIEW_SetOwnerdataState(). This is why I asked about opening a bug report with an contrived application that crashes with wine but not with windows.
I have not found any test examples with mouse emulation. Maybe I missed something.
Or should I open a separate bug report for this line?
if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv));
On 11/06/2022 10.30, Angelo Haller wrote:
On 11/06/2022 03.02, Zhiyi Zhang wrote:
On 6/11/22 07:56, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix. Is this strictly necessary? The call site has special handling for: infoPtr->dwStyle & LVS_OWNERDATA
We could guard the call to the function at the call site, if that is preferred.
Apart from that I am not sure how to write a test for this. This bug is triggered by creating a non ownerdata list and selecting multiple entries holding shift+ctrl and clicking with the mouse. This will send an LVN_ODSTATECHANGED notification where it is not supposed to (as it is not an ownerdata list). You can write a test that demonstrate the exact same thing. Create a non onwerdata listview control and test the message sequence doesn't contain LVN_ODSTATECHANGED, which should have a todo_wine because Wine is currently broken in this case. Then you fix it in the next patch and removes the todo_wine.
As for the mouse emulation, could you do it using only the keyboard? Such as sending VK_DOWN while holding Shift and Ctrl?
This is exactly what I was trying to communicate. I can NOT trigger the erroneous sending of LVN_ODSTATECHANGED via the keyboard.
The bug is in LISTVIEW_AddGroupSelection which gets called from LISTVIEW_LButtonDown.
Patch 2/6 moves the offending code to a new function LISTVIEW_SetOwnerdataState().
This is why I asked about opening a bug report with an contrived application that crashes with wine but not with windows.
I just opened a bug report with a test application attached. Hope that helps :) https://bugs.winehq.org/show_bug.cgi?id=53123
I have not found any test examples with mouse emulation. Maybe I missed something.
Or should I open a separate bug report for this line?
if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv));
On 6/11/22 23:30, Angelo Haller wrote:
On 11/06/2022 03.02, Zhiyi Zhang wrote:
On 6/11/22 07:56, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
The LVN_ODSTATECHANGED notification should only be sent to lists that have LVS_OWNERDATA set.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 72ade724313..318df0a4093 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8946,6 +8946,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n { NMLVODSTATECHANGE nmlv; + if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; Make sense. It will be better if you can add a simple test before this patch and remove the todo_wines after the fix. Is this strictly necessary? The call site has special handling for: infoPtr->dwStyle & LVS_OWNERDATA
We could guard the call to the function at the call site, if that is preferred.
Apart from that I am not sure how to write a test for this. This bug is triggered by creating a non ownerdata list and selecting multiple entries holding shift+ctrl and clicking with the mouse. This will send an LVN_ODSTATECHANGED notification where it is not supposed to (as it is not an ownerdata list). You can write a test that demonstrate the exact same thing. Create a non onwerdata listview control and test the message sequence doesn't contain LVN_ODSTATECHANGED, which should have a todo_wine because Wine is currently broken in this case. Then you fix it in the next patch and removes the todo_wine.
As for the mouse emulation, could you do it using only the keyboard? Such as sending VK_DOWN while holding Shift and Ctrl?
This is exactly what I was trying to communicate. I can NOT trigger the erroneous sending of LVN_ODSTATECHANGED via the keyboard.
Ok, it's fine to leave the tests.
The bug is in LISTVIEW_AddGroupSelection which gets called from LISTVIEW_LButtonDown.
Patch 2/6 moves the offending code to a new function LISTVIEW_SetOwnerdataState().
This is why I asked about opening a bug report with an contrived application that crashes with wine but not with windows.
I have not found any test examples with mouse emulation. Maybe I missed something.
Or should I open a separate bug report for this line?
if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv));
From: Angelo Haller <angelo(a)szanni.org> Send LVN_ODSTATECHANGED notification on selection change for listviews when LVS_OWNERDATA is set. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52534 Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 318df0a4093..3c3f5af2a27 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3597,6 +3597,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) */ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) { + INT nFirst = -1; + INT nLast = -1; RANGES selection; DWORD old_mask; LVITEMW item; @@ -3648,21 +3650,25 @@ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) iterator_destroy(&i); } - /* disable per item notifications on LVS_OWNERDATA style - FIXME: single LVN_ODSTATECHANGED should be used */ + /* disable per item notifications on LVS_OWNERDATA style */ old_mask = infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->notify_mask &= ~NOTIFY_MASK_ITEM_CHANGE; LISTVIEW_DeselectAllSkipItems(infoPtr, selection); - iterator_rangesitems(&i, selection); - while(iterator_next(&i)) - LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + while(iterator_next(&i)) { + if (nFirst == -1) + nFirst = i.nItem; + nLast = i.nItem; + LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + } /* this will also destroy the selection */ iterator_destroy(&i); + LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); + infoPtr->notify_mask |= old_mask; LISTVIEW_SetItemFocus(infoPtr, nItem); } -- 2.36.0
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send LVN_ODSTATECHANGED notification on selection change for listviews when LVS_OWNERDATA is set.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52534 Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 318df0a4093..3c3f5af2a27 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3597,6 +3597,8 @@ static BOOL LISTVIEW_AddGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) */ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) { + INT nFirst = -1; + INT nLast = -1;
Let's merge it to a single line.
RANGES selection; DWORD old_mask; LVITEMW item; @@ -3648,21 +3650,25 @@ static void LISTVIEW_SetGroupSelection(LISTVIEW_INFO *infoPtr, INT nItem) iterator_destroy(&i); }
- /* disable per item notifications on LVS_OWNERDATA style - FIXME: single LVN_ODSTATECHANGED should be used */ + /* disable per item notifications on LVS_OWNERDATA style */ old_mask = infoPtr->notify_mask & NOTIFY_MASK_ITEM_CHANGE; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->notify_mask &= ~NOTIFY_MASK_ITEM_CHANGE;
LISTVIEW_DeselectAllSkipItems(infoPtr, selection);
- iterator_rangesitems(&i, selection); - while(iterator_next(&i)) - LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + while(iterator_next(&i)) { + if (nFirst == -1) + nFirst = i.nItem; + nLast = i.nItem; + LISTVIEW_SetItemState(infoPtr, i.nItem, &item); + } /* this will also destroy the selection */ iterator_destroy(&i);
+ LISTVIEW_SetOwnerDataState(infoPtr, nFirst, nLast, &item); + infoPtr->notify_mask |= old_mask; LISTVIEW_SetItemFocus(infoPtr, nItem); }
From: Angelo Haller <angelo(a)szanni.org> Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 3c3f5af2a27..bb394974906 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8953,6 +8953,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n NMLVODSTATECHANGE nmlv; if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; + if (nFirst == nLast) return; if (!item) return; ZeroMemory(&nmlv, sizeof(nmlv)); -- 2.36.0
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 3c3f5af2a27..bb394974906 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -8953,6 +8953,7 @@ static VOID LISTVIEW_SetOwnerDataState(LISTVIEW_INFO *infoPtr, INT nFirst, INT n NMLVODSTATECHANGE nmlv;
if (!(infoPtr->dwStyle & LVS_OWNERDATA)) return; + if (nFirst == nLast) return; if (!item) return;
ZeroMemory(&nmlv, sizeof(nmlv)); Add a test for this as well. You can merge it with the tests for 3/6.
From: Angelo Haller <angelo(a)szanni.org> Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change. Enable LVS_OWNERDATA multi select tests for wine. Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip) lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } /* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE); res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);; DestroyWindow(hwnd); } -- 2.36.0
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip)
lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests.
/* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE);
res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);;
Extra ; Thanks, Zhiyi
DestroyWindow(hwnd); }
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip)
lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests.
Maybe the comment needs to be better. Deselect all (-1) is the ONLY signal ever sent on ANY deselection, be it one or multiple items (for LVS_OWNERDATA). This is how win32 implements it, hence there is no way of making this a separate test. Maybe the line should read: /* Always send one deselect all (-1) notification for LVS_OWNERDATA style instead of * informing individual items of deselection */ And I am still looking into the other remarks. Some might need reordering as I won't be able to enable the tests without this last patch. Thanks for the feedback so far!
/* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE);
res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE);
res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void)
ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);;
Extra ;
Thanks, Zhiyi
DestroyWindow(hwnd); }
On 6/11/22 02:18, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip) lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests.
Maybe the comment needs to be better. Deselect all (-1) is the ONLY signal ever sent on ANY deselection, be it one or multiple items (for LVS_OWNERDATA). This is how win32 implements it, hence there is no way of making this a separate test.
Maybe the line should read:
/* Always send one deselect all (-1) notification for LVS_OWNERDATA style instead of * informing individual items of deselection */
And I am still looking into the other remarks. Some might need reordering as I won't be able to enable the tests without this last patch.
Thanks for the feedback so far!
I was thinking adding a test that selects items, and them maybe send a mouse click somewhere in the blank space of the listview control to deselect them all to show that only a -1 notification is sent. But it may be difficult to get the tests work reliably. If you can't make it work, I guess mixing it with other tests is okay.
/* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE); res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);; Extra ;
Thanks, Zhiyi
DestroyWindow(hwnd); }
On 11/06/2022 02.55, Zhiyi Zhang wrote:
On 6/11/22 02:18, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip) lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests. Maybe the comment needs to be better. Deselect all (-1) is the ONLY signal ever sent on ANY deselection, be it one or multiple items (for LVS_OWNERDATA). This is how win32 implements it, hence there is no way of making this a separate test.
Maybe the line should read:
/* Always send one deselect all (-1) notification for LVS_OWNERDATA style instead of * informing individual items of deselection */
And I am still looking into the other remarks. Some might need reordering as I won't be able to enable the tests without this last patch.
Thanks for the feedback so far! I was thinking adding a test that selects items, and them maybe send a mouse click somewhere in the blank space of the listview control to deselect them all to show that only a -1 notification is sent. But it may be difficult to get the tests work reliably. If you can't make it work, I guess mixing it with other tests is okay.
We do not need mouse clicks. See my reply to your comment in 1/6. The deselect of multiple items can be triggered by selecting multiple holding shift, releasing shift and moving the cursor again. This deselects multiple. The code testing for this is already in 1/6. My comment was with regards to enabling the tests individually. I might be able to do so by reordering and spitting some of the patches.
/* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE); res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);; Extra ;
Thanks, Zhiyi
DestroyWindow(hwnd); }
On 6/11/22 23:34, Angelo Haller wrote:
On 11/06/2022 02.55, Zhiyi Zhang wrote:
On 6/11/22 02:18, Angelo Haller wrote:
On 10/06/2022 03.13, Zhiyi Zhang wrote:
On 5/26/22 04:00, Angelo Haller wrote:
From: Angelo Haller <angelo(a)szanni.org>
Send one "deselect all items" notification on selection change for LVS_OWNERDATA listviews instead of notifying about each individual item change.
Enable LVS_OWNERDATA multi select tests for wine.
Signed-off-by: Angelo Haller <angelo(a)szanni.org> --- dlls/comctl32/listview.c | 6 ++++++ dlls/comctl32/tests/listview.c | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index bb394974906..a71e34b99d9 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -3406,6 +3406,12 @@ static BOOL LISTVIEW_DeselectAllSkipItems(LISTVIEW_INFO *infoPtr, RANGES toSkip) lvItem.state = 0; lvItem.stateMask = LVIS_SELECTED; + + /* notify deselect of all items (-1) on LVS_OWNERDATA style */ + if (infoPtr->dwStyle & LVS_OWNERDATA) { + LISTVIEW_SetItemState(infoPtr, -1, &lvItem); + return TRUE; + } Please add a separate test for this. Select and then deselect all items instead of mixing it with other tests. Maybe the comment needs to be better. Deselect all (-1) is the ONLY signal ever sent on ANY deselection, be it one or multiple items (for LVS_OWNERDATA). This is how win32 implements it, hence there is no way of making this a separate test.
Maybe the line should read:
/* Always send one deselect all (-1) notification for LVS_OWNERDATA style instead of * informing individual items of deselection */
And I am still looking into the other remarks. Some might need reordering as I won't be able to enable the tests without this last patch.
Thanks for the feedback so far! I was thinking adding a test that selects items, and them maybe send a mouse click somewhere in the blank space of the listview control to deselect them all to show that only a -1 notification is sent. But it may be difficult to get the tests work reliably. If you can't make it work, I guess mixing it with other tests is okay.
We do not need mouse clicks. See my reply to your comment in 1/6. The deselect of multiple items can be triggered by selecting multiple holding shift, releasing shift and moving the cursor again. This deselects multiple. The code testing for this is already in 1/6.
Using the keyboard VK_DOWN to deselect simultaneously selects another item. That's why I think it's mixing the results. You should be able to emulate a mouse click on the blank listview client area to deselect all items, without generating extra messages. This way we can see only the essential messages are sent and it's clearer.
My comment was with regards to enabling the tests individually. I might be able to do so by reordering and spitting some of the patches.
/* need to clone the DPA because callbacks can change it */ if (!(clone = ranges_clone(infoPtr->selectionRanges))) return FALSE; diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index 78b3e3ae069..066858ac7e8 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -3598,7 +3598,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_1_seq, - "ownerdata multiselect: select multiple via SHIFT", TRUE); + "ownerdata multiselect: select multiple via SHIFT", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3615,7 +3615,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_select_0_to_2_seq, - "ownerdata multiselect: select multiple via SHIFT+CONTROL", TRUE); + "ownerdata multiselect: select multiple via SHIFT+CONTROL", FALSE); res = SendMessageA(hwnd, WM_KEYUP, VK_DOWN, 0); expect(0, res); @@ -3633,7 +3633,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_all_select_3_seq, - "ownerdata multiselect: deselect all, select item 3", TRUE); + "ownerdata multiselect: deselect all, select item 3", FALSE); res = SendMessageA(hwnd, LVM_GETSELECTEDCOUNT, 0, 0); expect(1, res); @@ -3645,7 +3645,7 @@ static void test_ownerdata_multiselect(void) ok_sequence(sequences, PARENT_ODSTATECHANGED_SEQ_INDEX, ownerdata_multiselect_deselect_3_select_2_seq, - "ownerdata multiselect: deselect item 3, select item 2", TRUE); + "ownerdata multiselect: deselect item 3, select item 2", FALSE);; Extra ;
Thanks, Zhiyi
DestroyWindow(hwnd); }
participants (2)
-
Angelo Haller -
Zhiyi Zhang