Listview notify_dispinfoT Messageformat

[Thorsten Kani]

Hello,
Testing showed that the current implementation of notify_dispinfoT 
doesnt work with W2K. (or at least not with SP4 (:   )
The attached patch corrects that our Listview doesnt display anything on 
native.

Following assumption proved to be wrong for the Message LVN_GETDISPINFOW:
/*
With testing on Windows 2000 it looks like the notify format
has nothing to do with this message. It ALWAYS seems to be
in ansi format. */

This time, only LVN_*ETDISPINFOW is patched, anything else is untouched.
-Anybody please tell me if you see problems with this Fix since i dont 
want to break things.-

Tho

[Robert Shearman]

Thorsten Kani wrote:
> Hello,
> Testing showed that the current implementation of notify_dispinfoT 
> doesnt work with W2K. (or at least not with SP4 (:   )
> The attached patch corrects that our Listview doesnt display anything 
> on native.
>
> Following assumption proved to be wrong for the Message LVN_GETDISPINFOW:
> /*
> With testing on Windows 2000 it looks like the notify format
> has nothing to do with this message. It ALWAYS seems to be
> in ansi format. */
>
> This time, only LVN_*ETDISPINFOW is patched, anything else is untouched.
> -Anybody please tell me if you see problems with this Fix since i dont 
> want to break things.-

I have seen exactly the same problem with the IShellView implementation 
in native shell32. I have attached the patch I did at the time. I didn't 
submit it, because I soon got into a tangled mess where the function 
name was wrong and the isW parameter completely unnecessary and a whole 
bunch of functions calling it were also wrong.

To summarise: *all* common control notifications should be sent in the 
same format (ANSI/Unicode) as their parent (except if overriden by the 
CCS_SETUNICODEFORMAT message). It should not be based on the message 
sent to the control.

Rob

Index: listview.c
===================================================================
RCS file: /home/wine/wine/dlls/comctl32/listview.c,v
retrieving revision 1.394
diff -u -p -r1.394 listview.c
--- listview.c	2 Sep 2004 23:00:53 -0000	1.394
+++ listview.c	27 Oct 2004 20:16:51 -0000
@@ -818,10 +818,6 @@ static int get_ansi_notification(INT uni
 }
 
 /*
-  With testing on Windows 2000 it looks like the notify format
-  has nothing to do with this message. It ALWAYS seems to be
-  in ansi format.
-
   infoPtr : listview struct
   notificationCode : *Unicode* notification code
   pdi : dispinfo structure (can be unicode or ansi)
@@ -830,14 +826,10 @@ static int get_ansi_notification(INT uni
 static BOOL notify_dispinfoT(LISTVIEW_INFO *infoPtr, INT notificationCode, LPNMLVDISPINFOW pdi, BOOL isW)
 {
     BOOL bResult = FALSE;
-    BOOL convertToAnsi = FALSE;
     INT cchTempBufMax = 0, savCchTextMax = 0;
     LPWSTR pszTempBuf = NULL, savPszText = NULL;
 
-    if ((pdi->item.mask & LVIF_TEXT) && is_textT(pdi->item.pszText, isW))
-	    convertToAnsi = isW;
-
-    if (convertToAnsi)
+    if ((pdi->item.mask & LVIF_TEXT) && (infoPtr->notifyFormat == NFR_ANSI))
     {
 	if (notificationCode != LVN_GETDISPINFOW)
 	{
@@ -861,15 +853,16 @@ static BOOL notify_dispinfoT(LISTVIEW_IN
         savPszText = pdi->item.pszText;
         pdi->item.pszText = pszTempBuf;
         pdi->item.cchTextMax = cchTempBufMax;
+
+        notificationCode = get_ansi_notification(notificationCode);
     }
 
     TRACE(" pdi->item=%s\n", debuglvitem_t(&pdi->item, infoPtr->notifyFormat !=
            NFR_ANSI));
 
-    bResult = notify_hdr(infoPtr, get_ansi_notification(notificationCode),
-                        (LPNMHDR)pdi);
+    bResult = notify_hdr(infoPtr, notificationCode, &pdi->hdr);
 
-    if (convertToAnsi)
+    if ((pdi->item.mask & LVIF_TEXT) && (infoPtr->notifyFormat == NFR_ANSI))
     {
         MultiByteToWideChar(CP_ACP, 0, (LPSTR) pdi->item.pszText, -1,
                             savPszText, savCchTextMax);

[Thorsten Kani]

Testing out your more general solution introduced no Problems with the 
Listview on my Testsystem.
If the Patch gets applied, i will modify the callers of notify_dispinfo 
to respect that "isW" isnt longer used.  

Tho

Robert Shearman wrote:
> Thorsten Kani wrote:
> > Hello,
> > Testing showed that the current implementation of notify_dispinfoT 
> > doesnt work with W2K. (or at least not with SP4 (:   )
> > The attached patch corrects that our Listview doesnt display anything 
> > on native.
> >
> > Following assumption proved to be wrong for the Message 
> > LVN_GETDISPINFOW:
> > /*
> > With testing on Windows 2000 it looks like the notify format
> > has nothing to do with this message. It ALWAYS seems to be
> > in ansi format. */
> >
> > This time, only LVN_*ETDISPINFOW is patched, anything else is untouched.
> > -Anybody please tell me if you see problems with this Fix since i 
> > dont want to break things.-
>
> I have seen exactly the same problem with the IShellView 
> implementation in native shell32. I have attached the patch I did at 
> the time. I didn't submit it, because I soon got into a tangled mess 
> where the function name was wrong and the isW parameter completely 
> unnecessary and a whole bunch of functions calling it were also wrong.
>
> To summarise: *all* common control notifications should be sent in the 
> same format (ANSI/Unicode) as their parent (except if overriden by the 
> CCS_SETUNICODEFORMAT message). It should not be based on the message 
> sent to the control.
>
> Rob
>
> ------------------------------------------------------------------------
>
> Index: listview.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/comctl32/listview.c,v
> retrieving revision 1.394
> diff -u -p -r1.394 listview.c
> --- listview.c	2 Sep 2004 23:00:53 -0000	1.394
> +++ listview.c	27 Oct 2004 20:16:51 -0000
> @@ -818,10 +818,6 @@ static int get_ansi_notification(INT uni
> }
>
> /*
> -  With testing on Windows 2000 it looks like the notify format
> -  has nothing to do with this message. It ALWAYS seems to be
> -  in ansi format.
> -
>   infoPtr : listview struct
>   notificationCode : *Unicode* notification code
>   pdi : dispinfo structure (can be unicode or ansi)
> @@ -830,14 +826,10 @@ static int get_ansi_notification(INT uni
> static BOOL notify_dispinfoT(LISTVIEW_INFO *infoPtr, INT notificationCode, LPNMLVDISPINFOW pdi, BOOL isW)
> {
>     BOOL bResult = FALSE;
> -    BOOL convertToAnsi = FALSE;
>     INT cchTempBufMax = 0, savCchTextMax = 0;
>     LPWSTR pszTempBuf = NULL, savPszText = NULL;
>
> -    if ((pdi->item.mask & LVIF_TEXT) && is_textT(pdi->item.pszText, isW))
> -	    convertToAnsi = isW;
> -
> -    if (convertToAnsi)
> +    if ((pdi->item.mask & LVIF_TEXT) && (infoPtr->notifyFormat == NFR_ANSI))
>     {
>   if (notificationCode != LVN_GETDISPINFOW)
>   {
> @@ -861,15 +853,16 @@ static BOOL notify_dispinfoT(LISTVIEW_IN
>         savPszText = pdi->item.pszText;
>         pdi->item.pszText = pszTempBuf;
>         pdi->item.cchTextMax = cchTempBufMax;
> +
> +        notificationCode = get_ansi_notification(notificationCode);
>     }
>
> TRACE(" pdi->item=%s\n", debuglvitem_t(&pdi->item, infoPtr->notifyFormat !=
>            NFR_ANSI));
>
> -    bResult = notify_hdr(infoPtr, get_ansi_notification(notificationCode),
> -                        (LPNMHDR)pdi);
> +    bResult = notify_hdr(infoPtr, notificationCode, &pdi->hdr);
>
> -    if (convertToAnsi)
> +    if ((pdi->item.mask & LVIF_TEXT) && (infoPtr->notifyFormat == NFR_ANSI))
>     {
>         MultiByteToWideChar(CP_ACP, 0, (LPSTR) pdi->item.pszText, -1,
>                             savPszText, savCchTextMax);

[Dimitrie O. Paun]

On Wed, Oct 27, 2004 at 09:28:33PM +0100, Robert Shearman wrote:
> To summarise: *all* common control notifications should be sent in the 
> same format (ANSI/Unicode) as their parent (except if overriden by the 
> CCS_SETUNICODEFORMAT message). It should not be based on the message 
> sent to the control.

Just to be 100% clear:
  -- what do you mean by "in the same format as their parent"?
     From your patch, it seems that:
	1. In WM_CREATE, we have to query the notify format as such:

  infoPtr->hwndNotify = lpcs->hwndParent;
  infoPtr->notifyFormat = SendMessageW(infoPtr->hwndNotify, WM_NOTIFYFORMAT,
                                       (WPARAM)infoPtr->hwndSelf, (LPARAM)NF_QUERY);

	2. Handle WM_NOTIFYFORMAT, by querying the parent again:

  infoPtr->notifyFormat = SendMessageW(hwndFrom, WM_NOTIFYFORMAT, (WPARAM)infoPtr->hwndSelf, NF_QUERY);

        3. When sending notifications, convert to ASCII iff infoPtr->notifyFormat == NFR_ANSI


     If so, this is what we had before Aric did this change:
	http://cvs.winehq.org/cvsweb/wine/dlls/comctl32/listview.c.diff?r1=1.330&r2=...
     What was wrong with the code before? It tried to send the notification
     in the format specified by infoPtr->notifyFormat. The dependance on
     the the type of message that was sent to the control is just to do
     the conversion when we have to. That is:

            fmt-of-msg-sent-to-ctrl     infoPtr->notifyFormat       conversion-required
               !isW (ASCII)                    NFR_ANSI                   no
                isW (Unicode)                  NFR_ANSI                   yes
               !isW (ASCII)                    NFR_UNICODE                yes
                isW (Unicode)                  NFR_UNICODE                no

     The old code looked as isW only to determine if any conversion was required
     (which I think you have to), but the format of the actual notification was
     strictly determined by infoPtr->notifyFormat:

     if (infoPtr->notifyFormat == NFR_ANSI)
         realNotifCode = get_ansi_notification(notificationCode);
     else
         realNotifCode = notificationCode;
     bResult = notify_hdr(infoPtr, realNotifCode, (LPNMHDR)pdi);

     So, what was wrong with the original code?

  -- CCS_SETUNICODEFORMAT message: this is a flag, not a message, no? If so, what is
     its semantics? If set, we have to ignore infoPtr->notifyFormat, and always send
     notifications in Unicode format?


It's important to figure this one once and for all, so that we can fix all the
controls properly.

-- 
Dimi.

[Robert Shearman]

Dimitrie O. Paun wrote:
> On Wed, Oct 27, 2004 at 09:28:33PM +0100, Robert Shearman wrote:
> > To summarise: *all* common control notifications should be sent in the 
> > same format (ANSI/Unicode) as their parent (except if overriden by the 
> > CCS_SETUNICODEFORMAT message). It should not be based on the message 
> > sent to the control.
>
> Just to be 100% clear:
>  -- what do you mean by "in the same format as their parent"?

The format is retrieved using the WM_NOTIFYFORMAT message, but can be 
overridden by CCM_SETUNICODEFORMAT.
> From your patch, it seems that:
>   1. In WM_CREATE, we have to query the notify format as such:
>
> infoPtr->hwndNotify = lpcs->hwndParent;
>  infoPtr->notifyFormat = SendMessageW(infoPtr->hwndNotify, WM_NOTIFYFORMAT,
>                                       (WPARAM)infoPtr->hwndSelf, (LPARAM)NF_QUERY);

Yes, that is correct. It would be a lot better to use a boolean flag 
rather than storing the actual format code so that:
if (infoPtr->notifyFormat == NFR_UNICODE)
becomes:
if (infoPtr->bUnicode)
> 2. Handle WM_NOTIFYFORMAT, by querying the parent again:
>
> infoPtr->notifyFormat = SendMessageW(hwndFrom, WM_NOTIFYFORMAT, (WPARAM)infoPtr->hwndSelf, NF_QUERY);

Yes. It should also handle the NF_QUERY case in the WM_NOTIFYFORMAT 
handler so that child windows, like the header control, will get a sane 
value.
> 3. When sending notifications, convert to ASCII iff infoPtr->notifyFormat == NFR_ANSI
>
> If so, this is what we had before Aric did this change:
>   http://cvs.winehq.org/cvsweb/wine/dlls/comctl32/listview.c.diff?r1=1.330&r2=...
>     What was wrong with the code before? It tried to send the notification
>     in the format specified by infoPtr->notifyFormat. The dependance on
>     the the type of message that was sent to the control is just to do
>     the conversion when we have to. That is:
>
> fmt-of-msg-sent-to-ctrl     infoPtr->notifyFormat       conversion-required
>               !isW (ASCII)                    NFR_ANSI                   no
>                isW (Unicode)                  NFR_ANSI                   yes
>               !isW (ASCII)                    NFR_UNICODE                yes
>                isW (Unicode)                  NFR_UNICODE                no
>
> The old code looked as isW only to determine if any conversion was required
>     (which I think you have to), but the format of the actual notification was
>     strictly determined by infoPtr->notifyFormat:
>
> if (infoPtr->notifyFormat == NFR_ANSI)
>         realNotifCode = get_ansi_notification(notificationCode);
>     else
>         realNotifCode = notificationCode;
>     bResult = notify_hdr(infoPtr, realNotifCode, (LPNMHDR)pdi);
>
> So, what was wrong with the original code?

The old code does indeed send the correct notifications. Maybe there was 
a bug in the function somewhere that prompted Aric to make his change.
However, the code could be a lot simpler by doing conversions from A to 
W on incoming messages and then only doing W to A conversions for the 
notifications, rather than the matrix of 4 cases you described above.
> -- CCS_SETUNICODEFORMAT message: this is a flag, not a message, no? If so, what is
>     its semantics? If set, we have to ignore infoPtr->notifyFormat, and always send
>     notifications in Unicode format?

That was a mistake. I meant CCM_SETUNICODEFORMAT. 
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/pla...)
> It's important to figure this one once and for all, so that we can fix all the
> controls properly.

Rob

[Dimitrie O. Paun]

On Sun, Oct 31, 2004 at 10:50:33AM +0000, Robert Shearman wrote:
> Yes, that is correct. It would be a lot better to use a boolean flag 
> rather than storing the actual format code so that:
> if (infoPtr->notifyFormat == NFR_UNICODE)
> becomes:
> if (infoPtr->bUnicode)

Maybe a bit simpler yes, but surely not a lot :) Anyway, this flag is
tested for NFR_* only _once_ in a 10000 line file, so I actually think
that sticking to the standard is a better option here.
> The old code does indeed send the correct notifications. Maybe there was 
> a bug in the function somewhere that prompted Aric to make his change.
> However, the code could be a lot simpler by doing conversions from A to 
> W on incoming messages and then only doing W to A conversions for the 
> notifications, rather than the matrix of 4 cases you described above.

Oh, come on, it was just a few more lines of code. In fact, I was happy
with the code as it standed (that's no surprise, as I wrote it :)).
I think we should just revert to it, and accept changes only with
supporting tests. At the time that Aric submitted the patch, I was
rather unhappy with it, but he claimed he performed extensive tests
that proved that behaviour. He should have submitted them as part
of the test suite.

-- 
Dimi.

[Thorsten Kani]

> Oh, come on, it was just a few more lines of code. In fact, I was happy
> with the code as it standed (that's no surprise, as I wrote it :)).
> I think we should just revert to it, and accept changes only with
> supporting tests. At the time that Aric submitted the patch, I was
> rather unhappy with it, but he claimed he performed extensive tests
> that proved that behaviour. He should have submitted them as part
> of the test suite.

As i understand it,  the old Listview code was assuming that 
LVN_GETDISPINFOW  is  never  used and tried to convince apps with the 
"A" version of the Message..
This leads to problems with Braindamaged_Apps(TM) wich handle only the 
Unicode Message.
(Tested App was "Explorer")

[Dimitrie O. Paun]

On Mon, Nov 01, 2004 at 01:43:44AM +0100, Thorsten Kani wrote:
> As i understand it,  the old Listview code was assuming that 
> LVN_GETDISPINFOW  is  never  used and tried to convince apps with the 
> "A" version of the Message..

Why are you saying that. The code treats LVN_GETDISPINFOW special
because it's the only type of message that *gets* textual data
rather then sending. SO for this reason, we just reset the buffer
on the way out. What other difference is there?
> This leads to problems with Braindamaged_Apps(TM) wich handle only the 
> Unicode Message.  (Tested App was "Explorer")

Heh. Right now we only send ASCII notifications, which would be
even worse. We need proper test cases.

-- 
Dimi.
>

[Thorsten Kani]

I asked the List about the Problem and how to solve it correctly.
Please excuse my bad English (:  - will try your patch asap.
Tho
> Why are you saying that. The code treats LVN_GETDISPINFOW special
> because it's the only type of message that *gets* textual data
> rather then sending. SO for this reason, we just reset the buffer
> on the way out. What other difference is there?
> > This leads to problems with Braindamaged_Apps(TM) wich handle only the 
> > Unicode Message.  (Tested App was "Explorer")
>
> Heh. Right now we only send ASCII notifications, which would be
> even worse. We need proper test cases.