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

[Jim Mussared]

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).

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=44794
Signed-off-by: Jim Mussared 
---
 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);
 }

-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);
     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

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=42156

Your paranoid android.

=== debian9 (build log) ===

error: corrupt patch at line 34
Task: Patch failed to apply

[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
>
> 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

[Nikolay Sivov]

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
>
> 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