Re: comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Nikolay Sivov]

On 3/13/2010 06:35, Jeff Latimer wrote:
> If not set then no value is returned and in the case of uTorrent the 
> next entry selected is renamed. 
> http://bugs.winehq.org/show_bug.cgi?id=17251

No value returned from what?
> A test case has proved difficult to construct as the event is 
> triggered by mouse clicks and the behaviour of EditLabel appears to be 
> different in test.
>
> dlls/comctl32/listview.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>   dispInfo.item.iItem = editedItem;
>   dispInfo.item.iSubItem = 0;
>   dispInfo.item.stateMask = ~0;
> - dispInfo.item.cchTextMax = textlenT(pszText, isW)+2;
>  if (!LISTVIEW_GetItemW(infoPtr,&dispInfo.item))

Could you explain what this change fixes exactly? As I remember uTorrent 
uses an LVS_OWNERDATA listview, so this cchTextMax isn't processed in 
LISTVIEW_GetItemW.

Also I'm not sure what you mean here
---
textlenT(pszText, isW)+2
---
pszText is Edit control text at this point (or NULL). How it's related 
to LISTVIEW_GetItemW? And what this +2 adding mean?

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Alexandru Balut]

On Sat, Mar 13, 2010 at 10:30, Nikolay Sivov  wrote:
> Also I'm not sure what you mean here
> textlenT(pszText, isW)+2
>
> pszText is Edit control text at this point (or NULL). How it's related to
> LISTVIEW_GetItemW? And what this +2 adding mean?

The +2 appears because cchTextMax "includes the terminating NULL". I
think it should be 1 if not isW.

Thanks,
Alex

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Nikolay Sivov]

On 3/13/2010 13:38, Alexandru Balut wrote:
> On Sat, Mar 13, 2010 at 10:30, Nikolay Sivov  wrote:
> > Also I'm not sure what you mean here
> > textlenT(pszText, isW)+2
> >
> > pszText is Edit control text at this point (or NULL). How it's related to
> > LISTVIEW_GetItemW? And what this +2 adding mean?
>
> The +2 appears because cchTextMax "includes the terminating NULL". I
> think it should be 1 if not isW.

It doesn't depend on A<->W conversions, it's measured in characters. 
Anyway this is wrong.
> Thanks,
> Alex

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Jeff Latimer]

On 13/03/10 20:30, Nikolay Sivov wrote:
> On 3/13/2010 06:35, Jeff Latimer wrote:
> > If not set then no value is returned and in the case of uTorrent the 
> > next entry selected is renamed. 
> > http://bugs.winehq.org/show_bug.cgi?id=17251
>
> No value returned from what?

LISTVIEW_GetItemW
> >   dispInfo.item.iItem = editedItem;
> >   dispInfo.item.iSubItem = 0;
> >   dispInfo.item.stateMask = ~0;
> > - dispInfo.item.cchTextMax = textlenT(pszText, isW)+2;
> >  if (!LISTVIEW_GetItemW(infoPtr,&dispInfo.item))
>
> Could you explain what this change fixes exactly? As I remember 
> uTorrent uses an LVS_OWNERDATA listview, so this cchTextMax isn't 
> processed in LISTVIEW_GetItemW.

I have added some traces and a code fragment to 
http://bugs.winehq.org/show_bug.cgi?id=17251 showing the before and 
after inserting  dispInfo.item.cchTextMax = textlenT(pszText, isW)+2;
into LISTVIEW_EndEditLabelT
> Also I'm not sure what you mean here
> textlenT(pszText, isW)+2
>
> pszText is Edit control text at this point (or NULL). How it's related 
> to LISTVIEW_GetItemW? And what this +2 adding mean?

Yes pszText is Edit control text or NULL.  textLenT returns the length 
of the without  the trailing Null.  What's required here is a buffer at 
least 1 character longer than pszText+Null so that when 
LISTVIEW_GetItemW returns dispInfo.item.pszText we can determine whether 
it is longer than pszText.  We need +2 at least for this.

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Nikolay Sivov]

On 3/15/2010 15:08, Jeff Latimer wrote:
> On 13/03/10 20:30, Nikolay Sivov wrote:
> > On 3/13/2010 06:35, Jeff Latimer wrote:
> > > If not set then no value is returned and in the case of uTorrent the 
> > > next entry selected is renamed. 
> > > http://bugs.winehq.org/show_bug.cgi?id=17251
> >
> > No value returned from what?
>
> LISTVIEW_GetItemW

Ok. From what I see now we have a special path for LVIF_TEXT:

---LISTVIEW_GetItemT---
         if (lpLVItem->mask & LVIF_TEXT)
         {
         if (lpLVItem->mask & LVIF_NORECOMPUTE)
             /* reset mask */
             dispInfo.item.mask &= ~(LVIF_TEXT | LVIF_NORECOMPUTE);
         else
         {
             dispInfo.item.pszText = lpLVItem->pszText;
             dispInfo.item.cchTextMax = lpLVItem->cchTextMax;
         }
         }
---

So if you request LVIF_TEXT cchTextMax should be overwritten here, if we 
are at LVIF_NORECOMPUTE we don't even request LVIF_TEXT.
Have you determine exact cause of failure returning text from 
LISTVIEW_GetItemW, including failing path? That's what we really need.
> > >   dispInfo.item.iItem = editedItem;
> > >   dispInfo.item.iSubItem = 0;
> > >   dispInfo.item.stateMask = ~0;
> > > - dispInfo.item.cchTextMax = textlenT(pszText, isW)+2;
> > >  if (!LISTVIEW_GetItemW(infoPtr,&dispInfo.item))
> >
> > Could you explain what this change fixes exactly? As I remember 
> > uTorrent uses an LVS_OWNERDATA listview, so this cchTextMax isn't 
> > processed in LISTVIEW_GetItemW.
>
> I have added some traces and a code fragment to 
> http://bugs.winehq.org/show_bug.cgi?id=17251 showing the before and 
> after inserting  dispInfo.item.cchTextMax = textlenT(pszText, isW)+2;
> into LISTVIEW_EndEditLabelT
> > Also I'm not sure what you mean here
> > textlenT(pszText, isW)+2
> >
> > pszText is Edit control text at this point (or NULL). How it's 
> > related to LISTVIEW_GetItemW? And what this +2 adding mean?
>
> Yes pszText is Edit control text or NULL.  textLenT returns the length 
> of the without  the trailing Null.  What's required here is a buffer 
> at least 1 character longer than pszText+Null so that when 
> LISTVIEW_GetItemW returns dispInfo.item.pszText we can determine 
> whether it is longer than pszText.  We need +2 at least for this.

I don't get it, sorry. What is a trick here? Assume I'm ok with +1 here, 
but how 'it is longer than pszText' is important?

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Nikolay Sivov]

On 3/15/2010 15:08, Jeff Latimer wrote:
> LISTVIEW_GetItemW

Hm, actually EditLabel and EndEditLabel looks quite different. EditLabel 
uses static buffer, and EndEditLabel doesn't even set a buffer pointer. 
That's most likely a real problem here, could you try to copy way we 
call LISTVIEW_GetItemW to EndEditLabel?

comctl32: [Bug 17251] cchTextMax needs to be set for LVS_OWNERDATA as it uses LVN_GETDISPINFO notifications

[Jeff Latimer]

On 15/03/10 23:33, Nikolay Sivov wrote:
> On 3/15/2010 15:08, Jeff Latimer wrote:
> > LISTVIEW_GetItemW
>
> Hm, actually EditLabel and EndEditLabel looks quite different. 
> EditLabel uses static buffer, and EndEditLabel doesn't even set a 
> buffer pointer. That's most likely a real problem here, could you try 
> to copy way we call LISTVIEW_GetItemW to EndEditLabel?

That looks to be a winner.  Will resubmit soon.  Ta