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

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

From a general robustness principle, my version seems preferable. For
example, it makes this code work as intended:

uInt16 nId = ...;
try
{
    addColumTo_pCols(nId, ...);

    // some code that may throw
    (...)

    addColumnTo_m_aColumns(nId, ...);
}
catch (...)
{
    removeColumn(nId);
    throw;
}

And actually, now that this is clear in my mind, I think the patch
should be, for more clarity:

 void DbGridControl::RemoveColumn(sal_uInt16 nId)
 {
+    DbGridControl_Base::RemoveColumn(nId);
+
     sal_uInt16 nIndex = GetModelColumnPos(nId);
     if (nIndex == GRID_COLUMN_NOT_FOUND)
         return;
 
-    DbGridControl_Base::RemoveColumn(nId);
-
     delete m_aColumns[ nIndex ];
     DbGridColumns::iterator it = m_aColumns.begin();
     ::std::advance( it, nIndex );


This makes it more clear that the call to
DbGridControl_Base::RemoveColumn and the result of
GetModelColumnPos(nId) are meant to be completely independent.

-- 
Lionel