[Libreoffice] [REVIEW] fdo#40701 Base crashes when "Find Record" button is clicked

[Lionel Elie Mamane]

On Mon, Sep 12, 2011 at 01:50:29AM +0200, Lionel Elie Mamane wrote:
> On Mon, Sep 12, 2011 at 01:07:53AM +0200, Eike Rathke wrote:
>> On Monday, 2011-09-12 00:25:51 +0200, Lionel Elie Mamane wrote:

>>> 0001-fdo-40701-DbGridControl-RemoveColumn-even-if-no-corr.patch
>>> fixes the root cause of the bug, which caused
>>> void DbGridControl::EnableHandle(sal_Bool bEnable)
>>> {
>>>     RemoveColumn(0);
>>>     m_bHandle = bEnable;
>>>     InsertHandleColumn();
>>> }
>>> to misfunction: RemoveColumn(0) silently did not remove the column, so
>>> the call to InsertHandleColumn() added a second Handle Column, which
>>> confused the code in other places.

>> Hmm.. this

>> @@ -1723,11 +1723,12 @@ sal_uInt16 DbGridControl::AppendColumn(const XubString& rName, sal_uInt16 nWidth
>>  void DbGridControl::RemoveColumn(sal_uInt16 nId)
>>  {
>>      sal_uInt16 nIndex = GetModelColumnPos(nId);
>> -    if (nIndex == GRID_COLUMN_NOT_FOUND)
>> -        return;
>>  
>>      DbGridControl_Base::RemoveColumn(nId);
>>  
>> +    if (nIndex == GRID_COLUMN_NOT_FOUND)
>> +        return;
> +
>>      delete m_aColumns[ nIndex ];
>>      DbGridColumns::iterator it = m_aColumns.begin();
>>      ::std::advance( it, nIndex );


>> now attempts to unconditionally remove any column nId. I don't know if
>> and how the underlying code handles such cases,

> Good point; it handles it well

>> but a safer approach would be to still check for a valid index and
>> additionally the handle column case, so

>>     if (nIndex != GRID_COLUMN_NOT_FOUND || nId == 0)
>>         DbGridControl_Base::RemoveColumn(nId);

>> might be better suited.

> I don't think it makes a real difference either way.

After having slept on it, no. The situation is that there are *two*
column sequences:

1) BrowseBox::pCols

2) DbGridControl::m_aColumns

The call to "DbGridControl_Base::RemoveColumn(nId)" does "remove
column from BrowseBox::pCols, if it is present there";
DbGridControl_base is a class derived from BrowseBox.

The rest of the code of the function does "remove column from
DbGridControl::m_aColumns, if it is present there".

Tour version implements the logic is "if the column is not in
DbGridControl::m_aColumns, then do not remove it from
BrowseBox::pCols, except for the known case if nId==0".