Re: wine/dlls/comctl32 listview.c

[Rein Klazes]

On Tue, 29 Apr 2003 19:49:44 -0500, you wrote:
> Modified files:
>    dlls/comctl32  : listview.c
>
> Log message:
>    Dimitrie O. Paun 
>    Generate custom draw notifications for ownerdraw listviews (based on a
>    patch by Maxime Bellengé).
>    Reorganize the custom draw code for better reuse.

Dimitrie,

This causes a regression in Newsbin. Normally when retrieving new
headers they are displayed with a green background (showing that they
are "new"). With the patch, I notice two things:

- the first line is lacking the green the green background completely
(so it is white);
- following lines only the first column is painted correctly. The text
in the other columns lacks the green background.

Does this give you an idea what might be wrong?

Rein.

wine/dlls/comctl32 listview.c

[Dimitrie O. Paun]

On Tue, 6 May 2003, Rein Klazes wrote:
> Does this give you an idea what might be wrong?

Thanks for the bug report, I'll look into it later on tonight,
right now I'm at work.

wine/dlls/comctl32 listview.c

[Maxime Bellengé]

This patch made the search result listview of emule work. The one I 
sent to Dimitrie was based on that msdn page :
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/pla...

and on the source code of emule.

I am wondering if this patch broke something or triggered a new bug ?
However I am sure that this code was needed because the
CDDS_ITEMPREPAINT message was never sent.

Dimitrie, what's your viewpoint ?

Max

On Tue, 2003-05-06 at 19:51, Rein Klazes wrote:
> On Tue, 29 Apr 2003 19:49:44 -0500, you wrote:
> > Modified files:
> >    dlls/comctl32  : listview.c
> >
> > Log message:
> >    Dimitrie O. Paun 
> >    Generate custom draw notifications for ownerdraw listviews (based on a
> >    patch by Maxime Bellengé).
> >    Reorganize the custom draw code for better reuse.
>
> Dimitrie,
>
> This causes a regression in Newsbin. Normally when retrieving new
> headers they are displayed with a green background (showing that they
> are "new"). With the patch, I notice two things:
> - the first line is lacking the green the green background completely
>
> (so it is white);
> - following lines only the first column is painted correctly. The text
>
> in the other columns lacks the green background.
>
> Does this give you an idea what might be wrong?
>
> Rein.

wine/dlls/comctl32 listview.c

[Dimitrie O. Paun]

On May 6, 2003 01:51 pm, Rein Klazes wrote:
> This causes a regression in Newsbin. Normally when retrieving new
> headers they are displayed with a green background (showing that they
> are "new"). With the patch, I notice two things:
> - the first line is lacking the green the green background completely
>
> (so it is white);
> - following lines only the first column is painted correctly. The text
>
> in the other columns lacks the green background.

Rein,

Here is a completely untested patch (I didn't even have time to compile :),
but it should work. I think I found the problem for the first item (we
weren't setting CDDS_ITEM for item 0), and Maxime found (I think) the problem
for the second item (we weren't setting CDDS_SUBITEM for the columns).
What I don't understand about CDDS_SUBITEM is why is this a regression,
as we never set this CDDS_SUBITEM before...

Anyway, here it is, please let me know if it helps:

Index: dlls/comctl32/listview.c
===================================================================
RCS file: /var/cvs/wine/dlls/comctl32/listview.c,v
retrieving revision 1.348
diff -u -r1.348 listview.c
--- dlls/comctl32/listview.c    2 May 2003 20:14:33 -0000       1.348
+++ dlls/comctl32/listview.c    8 May 2003 13:01:49 -0000
@@ -858,7 +858,7 @@
     lpnmlvcd->clrTextBk = infoPtr->clrTextBk;
     lpnmlvcd->clrText   = infoPtr->clrText;
     if (!lplvItem) return;
-    lpnmlvcd->nmcd.dwItemSpec = lplvItem->iItem;
+    lpnmlvcd->nmcd.dwItemSpec = lplvItem->iItem + 1;
     lpnmlvcd->iSubItem = lplvItem->iSubItem;
     if (lplvItem->state & LVIS_SELECTED) lpnmlvcd->nmcd.uItemState |= CDIS_SELECTED;
     if (lplvItem->state & LVIS_FOCUSED) lpnmlvcd->nmcd.uItemState |= CDIS_FOCUS;
@@ -868,9 +868,16 @@

static inline DWORD notify_customdraw (LISTVIEW_INFO *infoPtr, DWORD dwDrawStage, NMLVCUSTOMDRAW *lpnmlvcd)
 {
+    BOOL isForItem = (lpnmlvcd->nmcd.dwItemSpec != 0);
+    DWORD result;
+
     lpnmlvcd->nmcd.dwDrawStage = dwDrawStage;
-    if (lpnmlvcd->nmcd.dwItemSpec) lpnmlvcd->nmcd.dwDrawStage |= CDDS_ITEM;
-    return notify_hdr(infoPtr, NM_CUSTOMDRAW, &lpnmlvcd->nmcd.hdr);
+    if (isForItem) lpnmlvcd->nmcd.dwDrawStage |= CDDS_ITEM;
+    if (lpnmlvcd->iSubItem) lpnmlvcd->nmcd.dwDrawStage |= CDDS_SUBITEM;
+    if (isForItem) lpnmlvcd->nmcd.dwItemSpec--;
+    result = notify_hdr(infoPtr, NM_CUSTOMDRAW, &lpnmlvcd->nmcd.hdr);
+    if (isForItem) lpnmlvcd->nmcd.dwItemSpec++;
+    return result;
 }

static DWORD notify_prepaint (LISTVIEW_INFO *infoPtr, HDC hdc, NMLVCUSTOMDRAW *lpnmlvcd)

wine/dlls/comctl32 listview.c

[Rein Klazes]

On Thu, 8 May 2003 09:07:49 -0400, you wrote:
> Here is a completely untested patch (I didn't even have time to compile :),
> but it should work. I think I found the problem for the first item (we
> weren't setting CDDS_ITEM for item 0), and Maxime found (I think) the problem
> for the second item (we weren't setting CDDS_SUBITEM for the columns).
> What I don't understand about CDDS_SUBITEM is why is this a regression,
> as we never set this CDDS_SUBITEM before...
>
> Anyway, here it is, please let me know if it helps:

Dimitrie,

The first line is corrected (behaves as the following lines). But the
second and following columns still lack the proper text background
colour.

Rein.

wine/dlls/comctl32 listview.c

[Dimitrie O. Paun]

On Thu, 8 May 2003, Rein Klazes wrote:
> The first line is corrected (behaves as the following lines). But the
> second and following columns still lack the proper text background
> colour.

Excellent! This is what I wanted to hear. In fact, I would have
been very worried if all problems would have been solved, because
(as I was saying) we weren't setting CDDS_SUBITEM flag before,
so why would have that fixed anything?

wine/dlls/comctl32 listview.c

[Maxime Bellengé]

I think I've found the problem, it comes from the fact that we don't
send the customdraw notify messages properly.

the correct sequence is in report mode:
1 - CDDS_PREPAINT
2 - CDDS_ITEMPREPAINT for each row if 1 contains CDRF_NOTIFYITEMDRAW
    - if the returned value is CDRF_NEWFONT, the color and font 	  values
are valid for all the subitems (ie : columns)
    - if the return value is CDRF_NOTIFYSUBITEMDRAW, we must send 	  the
CDDS_SUBITEM | CDDS_PREPAINT before drawing each column.
3 - CDDS_ITEMPOSTPAINT  if the returned value for the above call
    contains CDRF_NOTIFYPOSTPAINT
4 - CDDS_POSTPAINT if 1 returned CDRF_NOTIFYPOSTPAINT

Here is what is done in wine

1 - CDDS_PREPAINT
2 - CDDS_ITEMPREPAINT for each row and subitems(columns) if the returned
    value from 1 contains 	CDRF_NOTIFYITEMDRAW
3 - CDDS_ITEMPOSTPAINT if the returned value for the above call
    contains CDRF_NOTIFYPOSTPAINT
4 - CDDS_POSTPAINT if 1 returned CDRF_NOTIFYPOSTPAINT

I think it is why the colors in the subitems are not correct. I think
newsbin returns CDRF_NEWFONT so we must not send CDDS_NOTIFYITEMDRAW for
subitems (we should never send that, it is wrong). For each subitems we
send CDDS_NOTIFYITEMDRAW so the colors are set wrong by newsbin I think

To fix that we should change LISTVIEW_RefreshReport to send
CDDS_NOTIFYITEMDRAW if needed and we should change LISTVIEW_DrawItem to
send CDDS_NOTIFYSUBITEMDRAW if needed otherwise set the colors/font for
all the subitems of a row.

Dimitrie, what do you think of my idea ?

a+

Max

On Thu, 2003-05-08 at 17:22, Dimitrie O. Paun wrote:
> On Thu, 8 May 2003, Rein Klazes wrote:
> > The first line is corrected (behaves as the following lines). But the
> > second and following columns still lack the proper text background
> > colour.
>
> Excellent! This is what I wanted to hear. In fact, I would have
> been very worried if all problems would have been solved, because
> (as I was saying) we weren't setting CDDS_SUBITEM flag before,
> so why would have that fixed anything?