From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH] comctl32: Improve hit testing and invalidation for sub items.
Message-Id: <3c2d5da2-022a-01dd-0c77-d014897ce484@codeweavers.com>
Date: Mon, 24 Sep 2018 11:15:11 +0300
In-Reply-To: <CAMGfSbgtyhW=uZpB-gyEC2ziahH3uTeSycAniPg_OAv7NYQ90Q@mail.gmail.com>
References: <CAMGfSbgtyhW=uZpB-gyEC2ziahH3uTeSycAniPg_OAv7NYQ90Q@mail.gmail.com>

On 09/18/2018 10:35 AM, Jim Mussared wrote:

> These fixes are for "Vectric Cut2D" which uses listview to show
> document layers. See referenced bug for more details. The layer list
> uses clickable subitems for:
>   - toggling visibility of the layer
>   - color swatch (click to edit color)
>   - menu (click to show the right-click menu)
>   (all of these are activated on mouse up)
>
> At the moment, the sub items are not clickable in Wine. Additionally,
> they do not update when the layer changes state.
>
> This patch fixes three things:
>   - When hit testing, the sub items's coordinates are relative to the
>   item. So the mouse coordinate (opt) will be outside the bounds of the
>   item (rcBounds), and fail the hit test.
>   Skip this check when not doing a selection hit test (which applies to
>   mouse up).
>   - InvalidateItem only invalidates the region of the primary item. Now
>   also invalidate all sub-items.
>   - InvalidateSubItem calculated the position of the subitem
>   incorrectly (it's offset by the left edge of the main item).

Please split it into three separate patches then.

>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=44794
> Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
> ---
>   dlls/comctl32/listview.c | 41 ++++++++++++++++++++++++----------------
>   1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
> index 777b40f2fe..1440581f71 100644
> --- a/dlls/comctl32/listview.c
> +++ b/dlls/comctl32/listview.c
> @@ -1725,38 +1725,46 @@ static inline BOOL is_redrawing(const
> LISTVIEW_INFO *infoPtr)
>
>   static inline void LISTVIEW_InvalidateRect(const LISTVIEW_INFO
> *infoPtr, const RECT* rect)
>   {
> -    if(!is_redrawing(infoPtr)) return;
> +    if(!is_redrawing(infoPtr)) return;
>       TRACE(" invalidating rect=%s\n", wine_dbgstr_rect(rect));
>       InvalidateRect(infoPtr->hwndSelf, rect, TRUE);
>   }

Please don't include formatting or whitespace only changes.

>
> -static inline void LISTVIEW_InvalidateItem(const LISTVIEW_INFO
> *infoPtr, INT nItem)
> -{
> -    RECT rcBox;
> -
> -    if (!is_redrawing(infoPtr) || nItem < 0 || nItem >= infoPtr->nItemCount)
> -        return;
> -
> -    LISTVIEW_GetItemBox(infoPtr, nItem, &rcBox);
> -    LISTVIEW_InvalidateRect(infoPtr, &rcBox);
> -}
> -
>   static inline void LISTVIEW_InvalidateSubItem(const LISTVIEW_INFO
> *infoPtr, INT nItem, INT nSubItem)
>   {
>       POINT Origin, Position;
>       RECT rcBox;
> -
> -    if(!is_redrawing(infoPtr)) return;
> +
> +    if(!is_redrawing(infoPtr)) return;
>       assert (infoPtr->uView == LV_VIEW_DETAILS);
>       LISTVIEW_GetOrigin(infoPtr, &Origin);
>       LISTVIEW_GetItemOrigin(infoPtr, nItem, &Position);
>       LISTVIEW_GetHeaderRect(infoPtr, nSubItem, &rcBox);
>       rcBox.top = 0;
>       rcBox.bottom = infoPtr->nItemHeight;
> -    OffsetRect(&rcBox, Origin.x + Position.x, Origin.y + Position.y);
> +    OffsetRect(&rcBox, Origin.x, Origin.y + Position.y);

This one is meant for reordered columns case?

>       LISTVIEW_InvalidateRect(infoPtr, &rcBox);
>   }
>
> +static inline void LISTVIEW_InvalidateItem(const LISTVIEW_INFO
> *infoPtr, INT nItem)
> +{
> +    RECT rcBox;
> +
> +    if (!is_redrawing(infoPtr) || nItem < 0 || nItem >= infoPtr->nItemCount)
> +        return;
> +
> +    LISTVIEW_GetItemBox(infoPtr, nItem, &rcBox);
> +    LISTVIEW_InvalidateRect(infoPtr, &rcBox);
> +
> +    /* Additionally invalidate all other sub-items.
> +       The first sub-item is handled by GetItemBox above. */
> +    if (infoPtr->uView == LV_VIEW_DETAILS) {
> +        INT nSubItem;
> +       for (nSubItem = 1; nSubItem <
> DPA_GetPtrCount(infoPtr->hdpaColumns); nSubItem++)
> +            LISTVIEW_InvalidateSubItem(infoPtr, nItem, nSubItem);
> +    }
> +}

I don't see why this would be necessary, item box should include whole 
row already.

> +
>   static inline void LISTVIEW_InvalidateList(const LISTVIEW_INFO *infoPtr)
>   {
>       LISTVIEW_InvalidateRect(infoPtr, NULL);
> @@ -7741,8 +7749,9 @@ static INT LISTVIEW_HitTest(const LISTVIEW_INFO
> *infoPtr, LPLVHITTESTINFO lpht,
>           UnionRect(&rcBounds, &rcIcon, &rcLabel);
>           UnionRect(&rcBounds, &rcBounds, &rcState);
>       }
> +
>       TRACE("rcBounds=%s\n", wine_dbgstr_rect(&rcBounds));
> -    if (!PtInRect(&rcBounds, opt)) return -1;
> +    if (select && !PtInRect(&rcBounds, opt)) return -1;
>
>       /* That's a special case - row rectangle is used as item rectangle and
>          returned flags contain all item parts. */

This looks wrong. Could you describe listview configuration and hittest 
messages being sent, that don't work?

> --
> 2.19.0
>
>