[PATCH] comctl32: Improve hit testing and invalidation for sub items.
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 <jim.mussared(a)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); } -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
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
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(a)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
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(a)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
participants (4)
-
Jim Mussared -
Marvin -
Nikolay Sivov -
Zhiyi Zhang