[PATCH] comctl32/listview: Don't send LVN_ENDLABELEDIT twice under some circumstances
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comctl32/listview.c | 10 +++++++++ dlls/comctl32/tests/listview.c | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 200bf93be55..0193c038ebb 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -326,6 +326,7 @@ typedef struct tagLISTVIEW_INFO /* misc */ DWORD iVersion; /* CCM_[G,S]ETVERSION */ + BOOL inEndEditLabel; /* To prevent sending LVN_ENDLABELEDIT during LVN_ENDLABELEDIT */ } LISTVIEW_INFO; /* @@ -5869,6 +5870,9 @@ static BOOL LISTVIEW_EndEditLabelT(LISTVIEW_INFO *infoPtr, BOOL storeText, BOOL WCHAR *pszText = NULL; BOOL res; + if (infoPtr->inEndEditLabel) + return FALSE; + if (storeText) { DWORD len = isW ? GetWindowTextLengthW(infoPtr->hwndEdit) : GetWindowTextLengthA(infoPtr->hwndEdit); @@ -5914,9 +5918,14 @@ static BOOL LISTVIEW_EndEditLabelT(LISTVIEW_INFO *infoPtr, BOOL storeText, BOOL dispInfo.item.pszText = same ? NULL : pszText; dispInfo.item.cchTextMax = textlenT(dispInfo.item.pszText, isW); + /* Sending LVN_ENDLABELEDITW might trigger EN_KILLFOCUS which would call this function again */ + infoPtr->inEndEditLabel = TRUE; + /* Do we need to update the Item Text */ res = notify_dispinfoT(infoPtr, LVN_ENDLABELEDITW, &dispInfo, isW); + infoPtr->inEndEditLabel = FALSE; + infoPtr->nEditLabelItem = -1; infoPtr->hwndEdit = 0; @@ -9497,6 +9506,7 @@ static LRESULT LISTVIEW_NCCreate(HWND hwnd, WPARAM wParam, const CREATESTRUCTW * infoPtr->itemEdit.fEnabled = FALSE; infoPtr->iVersion = COMCTL32_VERSION; infoPtr->colRectsDirty = FALSE; + infoPtr->inEndEditLabel = FALSE; /* get default font (icon title) */ SystemParametersInfoW(SPI_GETICONTITLELOGFONT, 0, &logFont, 0); diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index e9b715ee412..3e9e9697a4f 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -75,6 +75,10 @@ static BOOL g_disp_A_to_W; static NMLVDISPINFOA g_editbox_disp_info; /* when this is set focus will be tested on LVN_DELETEITEM */ static BOOL g_focus_test_LVN_DELETEITEM; +/* Whether to send WM_KILLFOCUS to the edit control during LVN_ENDLABELEDIT */ +static BOOL do_LVN_ENDLABELEDIT_killfocus = FALSE; +/* Number of LVN_ENDLABELEDIT notifications received */ +static BOOL LVN_ENDLABELEDITA_count = 0; static HWND subclass_editbox(HWND hwndListview); @@ -510,6 +514,11 @@ static LRESULT WINAPI parent_wnd_proc(HWND hwnd, UINT message, WPARAM wParam, LP ok(IsWindow(edit), "expected valid edit control handle\n"); ok((GetWindowLongA(edit, GWL_STYLE) & ES_MULTILINE) == 0, "edit is multiline\n"); + LVN_ENDLABELEDITA_count++; + + if (do_LVN_ENDLABELEDIT_killfocus) + SendMessageA(edit, WM_KILLFOCUS, 0, 0); + return TRUE; } case LVN_BEGINSCROLL: @@ -6322,6 +6331,37 @@ static void test_LVSCW_AUTOSIZE(void) DestroyWindow(hwnd); } +static void test_LVN_ENDLABELEDITW(void) +{ + HWND hwnd, hwndedit; + LVITEMW item = {0}; + WCHAR text[] = {'l','a','l','a',0}; + DWORD ret; + + hwnd = create_listview_control(LVS_REPORT | LVS_EDITLABELS); + + insert_column(hwnd, 0); + + item.mask = LVIF_TEXT; + item.pszText = text; + ListView_InsertItemW(hwnd, &item); + + SetFocus(hwnd); + hwndedit = (HWND)SendMessageW(hwnd, LVM_EDITLABELW, 0, 0); + + ret = SendMessageA(hwndedit, WM_SETTEXT, 0, (LPARAM)"test"); + expect(TRUE, ret); + + LVN_ENDLABELEDITA_count = 0; + do_LVN_ENDLABELEDIT_killfocus = TRUE; + ret = SendMessageA(hwndedit, WM_KEYDOWN, VK_RETURN, 0); + do_LVN_ENDLABELEDIT_killfocus = FALSE; + ok(LVN_ENDLABELEDITA_count == 1, + "messagebox during LVN_ENDLABELEDIT gave wrong number of LVN_ENDLABELEDITA: %d\n", LVN_ENDLABELEDITA_count); + + DestroyWindow(hwnd); +} + START_TEST(listview) { ULONG_PTR ctx_cookie; @@ -6425,6 +6465,7 @@ START_TEST(listview) test_oneclickactivate(); test_state_image(); test_LVSCW_AUTOSIZE(); + test_LVN_ENDLABELEDITW(); unload_v6_module(ctx_cookie, hCtx); -- 2.18.0
On 08/24/2018 11:41 PM, Fabian Maurer wrote:
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comctl32/listview.c | 10 +++++++++ dlls/comctl32/tests/listview.c | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c index 200bf93be55..0193c038ebb 100644 --- a/dlls/comctl32/listview.c +++ b/dlls/comctl32/listview.c @@ -326,6 +326,7 @@ typedef struct tagLISTVIEW_INFO
/* misc */ DWORD iVersion; /* CCM_[G,S]ETVERSION */ + BOOL inEndEditLabel; /* To prevent sending LVN_ENDLABELEDIT during LVN_ENDLABELEDIT */ } LISTVIEW_INFO;
If we really end up doing this (see test comments), it'd be better to use a single field to mask change notifications and label one, see bDoChangeNotify.
/* @@ -5869,6 +5870,9 @@ static BOOL LISTVIEW_EndEditLabelT(LISTVIEW_INFO *infoPtr, BOOL storeText, BOOL WCHAR *pszText = NULL; BOOL res;
+ if (infoPtr->inEndEditLabel) + return FALSE; + if (storeText) { DWORD len = isW ? GetWindowTextLengthW(infoPtr->hwndEdit) : GetWindowTextLengthA(infoPtr->hwndEdit); @@ -5914,9 +5918,14 @@ static BOOL LISTVIEW_EndEditLabelT(LISTVIEW_INFO *infoPtr, BOOL storeText, BOOL dispInfo.item.pszText = same ? NULL : pszText; dispInfo.item.cchTextMax = textlenT(dispInfo.item.pszText, isW);
+ /* Sending LVN_ENDLABELEDITW might trigger EN_KILLFOCUS which would call this function again */ + infoPtr->inEndEditLabel = TRUE; + /* Do we need to update the Item Text */ res = notify_dispinfoT(infoPtr, LVN_ENDLABELEDITW, &dispInfo, isW);
+ infoPtr->inEndEditLabel = FALSE; + infoPtr->nEditLabelItem = -1; infoPtr->hwndEdit = 0;
@@ -9497,6 +9506,7 @@ static LRESULT LISTVIEW_NCCreate(HWND hwnd, WPARAM wParam, const CREATESTRUCTW * infoPtr->itemEdit.fEnabled = FALSE; infoPtr->iVersion = COMCTL32_VERSION; infoPtr->colRectsDirty = FALSE; + infoPtr->inEndEditLabel = FALSE;
/* get default font (icon title) */ SystemParametersInfoW(SPI_GETICONTITLELOGFONT, 0, &logFont, 0); diff --git a/dlls/comctl32/tests/listview.c b/dlls/comctl32/tests/listview.c index e9b715ee412..3e9e9697a4f 100644 --- a/dlls/comctl32/tests/listview.c +++ b/dlls/comctl32/tests/listview.c @@ -75,6 +75,10 @@ static BOOL g_disp_A_to_W; static NMLVDISPINFOA g_editbox_disp_info; /* when this is set focus will be tested on LVN_DELETEITEM */ static BOOL g_focus_test_LVN_DELETEITEM; +/* Whether to send WM_KILLFOCUS to the edit control during LVN_ENDLABELEDIT */ +static BOOL do_LVN_ENDLABELEDIT_killfocus = FALSE; +/* Number of LVN_ENDLABELEDIT notifications received */ +static BOOL LVN_ENDLABELEDITA_count = 0;
static HWND subclass_editbox(HWND hwndListview);
@@ -510,6 +514,11 @@ static LRESULT WINAPI parent_wnd_proc(HWND hwnd, UINT message, WPARAM wParam, LP ok(IsWindow(edit), "expected valid edit control handle\n"); ok((GetWindowLongA(edit, GWL_STYLE) & ES_MULTILINE) == 0, "edit is multiline\n");
+ LVN_ENDLABELEDITA_count++; + + if (do_LVN_ENDLABELEDIT_killfocus) + SendMessageA(edit, WM_KILLFOCUS, 0, 0); +
The question is if the focus is still on edit at this point. I remember we already had some problems with it, because applications can easily depend on exact sequence of events here, e.g. LVN_ENDLABELEDIT handler can interact with edit box directly, or EN_* messages could be intercepted.
return TRUE; } case LVN_BEGINSCROLL: @@ -6322,6 +6331,37 @@ static void test_LVSCW_AUTOSIZE(void) DestroyWindow(hwnd); }
+static void test_LVN_ENDLABELEDITW(void) +{ + HWND hwnd, hwndedit; + LVITEMW item = {0}; + WCHAR text[] = {'l','a','l','a',0}; + DWORD ret; + + hwnd = create_listview_control(LVS_REPORT | LVS_EDITLABELS); + + insert_column(hwnd, 0); + + item.mask = LVIF_TEXT; + item.pszText = text; + ListView_InsertItemW(hwnd, &item); + + SetFocus(hwnd); + hwndedit = (HWND)SendMessageW(hwnd, LVM_EDITLABELW, 0, 0); + + ret = SendMessageA(hwndedit, WM_SETTEXT, 0, (LPARAM)"test"); + expect(TRUE, ret); + + LVN_ENDLABELEDITA_count = 0; + do_LVN_ENDLABELEDIT_killfocus = TRUE; + ret = SendMessageA(hwndedit, WM_KEYDOWN, VK_RETURN, 0); + do_LVN_ENDLABELEDIT_killfocus = FALSE; + ok(LVN_ENDLABELEDITA_count == 1, + "messagebox during LVN_ENDLABELEDIT gave wrong number of LVN_ENDLABELEDITA: %d\n", LVN_ENDLABELEDITA_count); +
This should use a message sequence test to avoid another global variable. Does this fix some application or you just noticed that? We should fix that regardless, I'm just curious.
Hello Nikolay, thanks for your comments.
If we really end up doing this (see test comments), it'd be better to use a single field to mask change notifications and label one, see bDoChangeNotify.
Sure, that's reasonable.
The question is if the focus is still on edit at this point. I remember we already had some problems with it, because applications can easily depend on exact sequence of events here, e.g. LVN_ENDLABELEDIT handler can interact with edit box directly, or EN_* messages could be intercepted.
This should use a message sequence test to avoid another global variable.
Sure, this will take care of the "exact sequence" issue too.
Does this fix some application or you just noticed that? We should fix that regardless, I'm just curious.
It affects wine regedit. Just open it, got to "HKCU/Environment" and try renaming TEMP to TEMp. You'll get the error message twice, due to this bug. It's not a big issue really, but I'd like it fixed. Regards, Fabian Maurer
Looking at the code again, it does seem quite inconvenient to mask bDoChangeNotify.
bOldChange = infoPtr->bDoChangeNotify; if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->bDoChangeNotify = FALSE; ... infoPtr->bDoChangeNotify = bOldChange;
I'm not sure, how could I do this in a nice way with a bitmask? Regards, Fabian Maurer
On 08/25/2018 11:19 PM, Fabian Maurer wrote:
Looking at the code again, it does seem quite inconvenient to mask bDoChangeNotify.
bOldChange = infoPtr->bDoChangeNotify;
if (infoPtr->dwStyle & LVS_OWNERDATA) infoPtr->bDoChangeNotify = FALSE;
...
infoPtr->bDoChangeNotify = bOldChange;
I'm not sure, how could I do this in a nice way with a bitmask?
Exactly how it's done now, bOldChange = mask & NOTIFY_ITEMCHANGE; mask &= ~NOTIFY_ITEMCHANGE; mask |= bOldChange;
Regards,
Fabian Maurer
participants (2)
-
Fabian Maurer -
Nikolay Sivov