[PATCH] comctl32: Improve hit testing and invalidation for sub items.

[Zhiyi Zhang]

On 2018/9/18 15:35, 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 add tests for prove your point and you might want to split the patch. Thanks
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=44794
> Signed-off-by: Jim Mussared <jim.mussared at 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;
Formating issue.
>      TRACE(" invalidating rect=%s\n", wine_dbgstr_rect(rect));
>      InvalidateRect(infoPtr->hwndSelf, rect, TRUE);
>  }
>
> -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);
> -}
You're changing the function. Please don't remove it and add it back somewhere else.
It's making it difficult to compare.
> -
>  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;
Formating issue
>      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);
>      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);
> +    }
> +}
> +
>  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. */
> --
> 2.19.0
>
>