Opened 3 years ago

Closed 12 months ago

#13883 closed defect (fixed)

[MSW] wxChoice/wxListBox - different client data handling

Reported by: ericj Owned by: vadz
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: wxChoice wxListBox clientdata Cc: rivdsl@…
Blocked By: Blocking:
Patch: yes

Description

Retrieving the client data is handled slightly different between wxChoice and wxListBox. For wxListBox, the client data is silently returned without error checking. In wxChoice the return value is checked for error.

This becomes relevant when the user tries to store data that happens to be the same as the Windows error code, CB_ERR = -1. Retrieving this value will fail for wxChoice, but work for wxListBox.

The following patch unifies handling between the two controls and makes the error checking more robust by also checking the value of GetLastError().

Attachments (2)

msw_choice_listbox_itemdata.patch download (1.2 KB) - added by ericj 3 years ago.
msw_choice_listbox_itemdata_unittest.patch download (3.6 KB) - added by RIVDSL 12 months ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by ericj

comment:1 Changed 3 years ago by VZ

(In [70414]) Check that an error really occurred when getting wxChoice data in wxMSW.

We could wrongly return NULL client data if -1 was stored as client data in
wxChoice because it's the same value as CB_ERR and we always interpreted it as
an error, while it may not be one if GetLastError() doesn't indicate it.

See #13883.

comment:2 Changed 3 years ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [70415]) Add error checking when retrieving client data from wxMSW wxListBox.

Verify if retrieving client data failed which might happen if the index
is invalid for example. This makes code more robust and also consistent with
wxChoice.

Closes #13883.

comment:3 Changed 12 months ago by RIVDSL

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is still not working properly. Apparently SendMessage does call SetLastError when there is an error, but not when the function is successful. Therefore if SendMessage successfully returns -1 the last error remains unchanged. If there was a previous error just before this (from another unrelated function), this function will assume an error and return 0.

I would suggest calling SetLastError before calling SendMessage as shown below:

void* wxChoice::DoGetItemClientData(unsigned int n) const
{
    SetLastError(ERROR_SUCCESS);
    LPARAM rc = SendMessage(GetHwnd(), CB_GETITEMDATA, n, 0);
    if ( rc == CB_ERR && GetLastError() != ERROR_SUCCESS )
    {
        wxLogLastError(wxT("CB_GETITEMDATA"));

        // unfortunately, there is no way to return an error code to the user
        rc = (LPARAM) NULL;
    }

    return (void *)rc;
}

Not sure if this causes large overhead or side effects though. Alternatively we could do this only when -1 is returned:

void* wxChoice::DoGetItemClientData(unsigned int n) const
{
    LPARAM rc = SendMessage(GetHwnd(), CB_GETITEMDATA, n, 0);
    if ( rc == CB_ERR )
    {
        SetLastError(ERROR_SUCCESS);
        rc = SendMessage(GetHwnd(), CB_GETITEMDATA, n, 0);

        if ( rc == CB_ERR && GetLastError() != ERROR_SUCCESS )
        {
            wxLogLastError(wxT("CB_GETITEMDATA"));

            // unfortunately, there is no way to return an error code to the user
            rc = (LPARAM) NULL;
        }
    }

    return (void *)rc;
}

comment:4 Changed 12 months ago by vadz

  • Owner set to vadz
  • Status changed from reopened to accepted

Thanks, I'll apply your fix soon (I don't think always calling SetLastError() can have a noticeable effect on performance and as this is simpler, I'll just do it).

It would be great to have a test for this case in the unit tests suite to avoid breaking it in the future, if you could make a patch adding one, it would be very welcome.

comment:5 Changed 12 months ago by RIVDSL

I added a patch containing the fix and the new unit test for both wxChoice and wxListBox.

comment:6 Changed 12 months ago by RIVDSL

  • Cc rivdsl@… added

comment:7 Changed 12 months ago by vadz

Thanks again, will apply soon.

comment:8 Changed 12 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [74994]) Avoid spurious errors when getting wxChoice or wxListBox client data in wxMSW.

Even after the changes of r70415, we could still report an error when the
item client data was set to -1 (== CB_ERR) as checking for GetLastError() was
not enough, we need to also ensure that the last error is reset before trying
to get the client data.

Also apply the same fix to wxListBox and add the tests verifying that this
does work correctly.

Closes #13883.

Also fix wxListBox::GetClientData().

Note: See TracTickets for help on using tickets.