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
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
