Re: [PATCH] comctl32: Add partial support for LVM_SETBKIMAGE.

[Zhiyi Zhang]

Let's use "comctl32/listview: Add partial support for LVM_SETBKIMAGE." for the subject.

On 4/22/22 21:29, Dmitry Timoshkov wrote:
> Signed-off-by: Dmitry Timoshkov
>
> dlls/comctl32/listview.c | 65 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
> index 730bf4aaddd..3ca76c2ef05 100644
> --- a/dlls/comctl32/listview.c
> +++ b/dlls/comctl32/listview.c
> @@ -91,7 +91,7 @@
> -
> - Messages:
> - -- LVM_ENABLEGROUPVIEW
> -
> - -- LVM_GETBKIMAGE, LVM_SETBKIMAGE
> -
> - -- LVM_GETBKIMAGE
> - -- LVM_GETGROUPINFO, LVM_SETGROUPINFO
> - -- LVM_GETGROUPMETRICS, LVM_SETGROUPMETRICS
> - -- LVM_GETINSERTMARK, LVM_SETINSERTMARK
>
> @@ -297,6 +297,7 @@ typedef struct tagLISTVIEW_INFO
>    COLORREF clrBk;
>    COLORREF clrText;
>    COLORREF clrTextBk;
> -
>
> HBITMAP hBkBitmap;
>
> /* font */
> HFONT hDefaultFont;
>
> @@ -4567,11 +4568,30 @@ static INT LISTVIEW_GetTopIndex(const LISTVIEW_INFO *infoPtr)
>   */
>  static inline BOOL LISTVIEW_FillBkgnd(const LISTVIEW_INFO *infoPtr, HDC hdc, const RECT *lprcBox)
>  {

Let's remove inline.
> - if (!infoPtr->hBkBrush) return FALSE;
> - if (infoPtr->hBkBitmap)
> - {
> -
>    HDC hdc_src = CreateCompatibleDC(hdc);
> -
> -
>    TRACE("(hdc=%p, lprcBox=%s, hBkBitmap=%p)\n", hdc, wine_dbgstr_rect(lprcBox), infoPtr->hBkBitmap);
> -
> -
>    SelectObject(hdc_src, infoPtr->hBkBitmap);
> -
>    BitBlt(hdc, lprcBox->left, lprcBox->top,
> -
>           lprcBox->right - lprcBox->left,
> -
>           lprcBox->bottom - lprcBox->top,
> -
>           hdc_src, lprcBox->left, lprcBox->top, SRCCOPY);

A bit of inconsistent styling. Let's wrap the parameters for BitBlt() at 100 characters.
> -
> -
>    DeleteDC(hdc_src);
>
> - TRACE("(hdc=%p, lprcBox=%s, hBkBrush=%p)\n", hdc, wine_dbgstr_rect(lprcBox), infoPtr->hBkBrush);
> -
>    return TRUE;
> - }

This is not an either or relation. The background brush is used to fill background if present. Then the background image is painted
on top of that if present.
> - else if (infoPtr->hBkBrush)
> - {
> -
>    TRACE("(hdc=%p, lprcBox=%s, hBkBrush=%p)\n", hdc, wine_dbgstr_rect(lprcBox), infoPtr->hBkBrush);
>
> - return FillRect(hdc, lprcBox, infoPtr->hBkBrush);
> -
>    return FillRect(hdc, lprcBox, infoPtr->hBkBrush);
> - }
> -
> - return FALSE;
>
> }
>
> /* Draw main item or subitem */
> @@ -8028,7 +8048,37 @@ static BOOL LISTVIEW_SetBkColor(LISTVIEW_INFO *infoPtr, COLORREF color)
>      return TRUE;
>  }
>
> -/* LISTVIEW_SetBkImage */
> +static BOOL LISTVIEW_SetBkImage(LISTVIEW_INFO *infoPtr, const LVBKIMAGEW *image, BOOL isW)
> +{
> - TRACE("%08lx,%p,%p,%u,%d,%d\n", image->ulFlags, image->hbm, image->pszImage,
> -
>    image->cchImageMax, image->xOffsetPercent, image->yOffsetPercent);

Add a space after comma.
> -
> - if (image->ulFlags & ~LVBKIF_SOURCE_MASK)
> -
>    FIXME("not supported flags %08lx\n", image->ulFlags & ~LVBKIF_SOURCE_MASK);
> -
> - if (image->xOffsetPercent || image->yOffsetPercent)
> -
>    FIXME("not supported offset %d,%d\n", image->xOffsetPercent, image->yOffsetPercent);
> -
> - switch (image->ulFlags & LVBKIF_SOURCE_MASK)
> - {
> - case LVBKIF_SOURCE_NONE:
> -
>    if (infoPtr->hBkBitmap) DeleteObject(infoPtr->hBkBitmap);
> -
>    infoPtr->hBkBitmap = 0;
> -
>    return TRUE;

This is supposed to return FALSE.
> -
> - case LVBKIF_SOURCE_HBITMAP:
> -
>    if (infoPtr->hBkBitmap) DeleteObject(infoPtr->hBkBitmap);
> -
>    infoPtr->hBkBitmap = image->hbm;
> -
>    InvalidateRect(infoPtr->hwndSelf, NULL, TRUE);

I don't think the InvalidateRect() is needed.

Also, I found LVM_SETBKIMAGE is acting a bit weird on Windows. For example, if a LVM_SETBKIMAGE message trying to set a bitmap
is sent the second time. The second message returns FALSE and the background image gets reset to none(from LVM_GETBKIMAGE). I don't know if we should
implement such a behavior but I would be more comfortable with some tests.
> -
>    return TRUE;
> -
> - case LVBKIF_SOURCE_URL:
> -
>    FIXME("LVBKIF_SOURCE_URL: %s\n", isW ? debugstr_w(image->pszImage) : debugstr_a((LPCSTR)image->pszImage));
> -
>    break;
> - }
> -
> - return FALSE;
>
> +}
>
> /*** Helper for {Insert,Set}ColumnT *only* */
>  static void column_fill_hditem(const LISTVIEW_INFO *infoPtr, HDITEMW *lphdi, INT nColumn,
> @@ -10406,6 +10456,7 @@ static LRESULT LISTVIEW_NCDestroy(LISTVIEW_INFO *infoPtr)
>    infoPtr->hFont = 0;
>    if (infoPtr->hDefaultFont) DeleteObject(infoPtr->hDefaultFont);
>    if (infoPtr->clrBk != CLR_NONE) DeleteObject(infoPtr->hBkBrush);
> -
>
> if (infoPtr->hBkBitmap) DeleteObject(infoPtr->hBkBitmap);
>
> SetWindowLongPtrW(infoPtr->hwndSelf, 0, 0);
>
> @@ -11554,7 +11605,9 @@ LISTVIEW_WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
>    case LVM_SETBKCOLOR:
>      return LISTVIEW_SetBkColor(infoPtr, (COLORREF)lParam);
> - /* case LVM_SETBKIMAGE: */
> -
>
> case LVM_SETBKIMAGEA:
> -
>
> case LVM_SETBKIMAGEW:
> -
>
> return LISTVIEW_SetBkImage(infoPtr, (LVBKIMAGEW *)lParam, uMsg == LVM_SETBKIMAGEW);
>
> case LVM_SETCALLBACKMASK:
> infoPtr->uCallbackMask = (UINT)wParam;