[PATCH] comctl32/listview: Don't send LVN_ENDLABELEDIT twice under some circumstances

[Nikolay Sivov]

On 08/24/2018 11:41 PM, Fabian Maurer wrote:

> Signed-off-by: Fabian Maurer <dark.shadow4 at 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.