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

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:

void BrowseBox::RemoveColumn( sal_uInt16 nItemId )
{
    // Spaltenposition ermitteln
    sal_uInt16 nPos = GetColumnPos(nItemId);
    if ( nPos >= ColCount() )
        // nicht vorhanden
        return;
    (...)
}

and

sal_uInt16 BrowseBox::GetColumnPos( sal_uInt16 nId ) const
{
    DBG_CHKTHIS(BrowseBox,BrowseBoxCheckInvariants);

    for ( sal_uInt16 nPos = 0; nPos < pCols->size(); ++nPos )
        if ( (*pCols)[ nPos ]->GetId() == nId )
            return nPos;
    return BROWSER_INVALIDID;
}

and

#define BROWSER_INVALIDID           USHRT_MAX

> 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.

This does the same as long as the class invariant of "one column in
BrowseBox::pCols for each DbGridControl::m_aColumns and vice-versa,
except for the handle column" holds. OTOH, if the invariant is broken
in that the column is in BrowseBox::pCols, but not in
DbGridControl::m_aColumns, my version actually "fixes" that, while
yours does not :) OTOH, your version is indeed more prudent in that it
avoids to do anything if the situation is not as it expects (invariant
broken).

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

-- 
Lionel