Re: comctl32: Use strcmpW instead of pointer comparison to ensure treeview updates correctly

[Nikolay Sivov]

On 10/19/2014 20:01, Dan Bassi wrote:
> Hi Nikolay. In uTorrent, the Treeview box on the left doesn't update 
> under Wine unless the box is clicked (whereas under Windows, it 
> updates automatically without clicking).
>
> I later traced the error to the "item_changed()" function in 
> comctl32/treeview.c, which was (incorrectly) detecting the text as 
> never having changed. This patch seems to fix that behaviour.
>
> Please let me know your thoughts. Many thanks.
> What I mean is that we need more tests in comctl32/tests/treeview.c to 
> prove that patch is correct. The fact that it fixes some case with some 
> application is not enough.
>
> On Sun, Oct 19, 2014 at 4:49 PM, Nikolay Sivov <bunglehead(a)gmail.com 
> <mailto:bunglehead(a)gmail.com>> wrote:
>
> On 10/19/2014 18:44, Dan Bassi wrote:
>
> +    /* Old text is a null pointer due to HeapAlloc() failing,
>         therefore unable to perform strcmpW so assume text has changed */
>         +    if (!tiOld->pszText)
>         +       return TRUE;
>         +
>
> I don't think you should necessary assume it's changed.
>
> /* Text has changed and it's not a callback */
>         -    if ((tvChange->mask & TVIF_TEXT) && (tiOld->pszText !=
>         tiNew->pszText) &&
>         +    if ((tvChange->mask & TVIF_TEXT) &&
>         (strcmpW(tiOld->pszText, tiNew->pszText) != 0) &&
>                 tiNew->pszText != LPSTR_TEXTCALLBACKW)
>                 return TRUE;
>
> This is wrong, you can't compare if it's a special callback value.
>
> +    originalItem.pszText = HeapAlloc(GetProcessHeap(), 0,
>         originalItem.cchTextMax * sizeof(WCHAR));
>
> Normally comctl32 code uses Alloc()/Free() functions.
>
> This patch needs tests first, those tests should cover all
>     possible combinations regarding callback cases, different pointers
>     but same data,
>     same data but different pointers, maybe something else.