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

2011-09-12 Thread 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.

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

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

2011-09-11 Thread Lionel Elie Mamane 
The attached patches are backport to 3.4 of my master commits
b71d12fbfdd4aad0b576645a0d7bbf71d1da7a3a
0ef7ce4a234c6649dcca219b96185acb7634fe34
1b6310433280ae4e8439dcbf17dfa763bf2826cc

They are related to fdo#40701.

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.

0002-FmXGridPeer-getByIndex-Error-checking-of-pGrid-GetMo.patch
fixes the reason the bug was a crasher instead of just broken
feature, nothing happens when Find Record button is clicked.

0003-Don-t-touch-handle-when-setting-property-Enabled.patch
is not per se directly related to fdo#40701, but it looks like a
(subtle) bug waiting to be discovered: The code enables/disables the
handle when the property Enabled is set, according to the value of
the Enabled property, while should do so (only) when setting the
property RecordMarker, according to its value. If one looks at the
commit message of 448cd2de7919401a0a84cd6e5dd5ede276ddf28e and its
diff, it looks rather strongly like this was *not* an intended change,
as that commit looks like a janitorial commit that did not intend to
change behaviour of the code.

As such, my case for
0003-Don-t-touch-handle-when-setting-property-Enabled.patch
in libreoffice-3-4 is much weaker, but I'm throwing it in and let's
see what you guys think of it.

-- 
Lionel
From f6a89eaf00a6230ddbfbb8bc5a09cc0f32b519af Mon Sep 17 00:00:00 2001
From: Lionel Elie Mamane lio...@mamane.lu
Date: Mon, 12 Sep 2011 00:00:33 +0200
Subject: [PATCH 1/3] fdo#40701: DbGridControl::RemoveColumn even if no corresponding Model column

That case crops up when nId==0 i.e. the Handle column
---
 svx/source/fmcomp/gridctrl.cxx |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/svx/source/fmcomp/gridctrl.cxx b/svx/source/fmcomp/gridctrl.cxx
index bb666bc..aec3738 100644
--- a/svx/source/fmcomp/gridctrl.cxx
+++ b/svx/source/fmcomp/gridctrl.cxx
@@ -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 );
-- 
1.7.2.5

From 9e34b69c3db910c51a910c8be3df9c740541fb12 Mon Sep 17 00:00:00 2001
From: Lionel Elie Mamane lio...@mamane.lu
Date: Sun, 11 Sep 2011 23:40:09 +0200
Subject: [PATCH 2/3] FmXGridPeer::getByIndex: Error checking of pGrid-GetModelColumnPos(nId) call

Fixes crash of fdo#40701, but not broken feature
---
 svx/source/fmcomp/fmgridif.cxx |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/svx/source/fmcomp/fmgridif.cxx b/svx/source/fmcomp/fmgridif.cxx
index 3080d77..a4146a9 100644
--- a/svx/source/fmcomp/fmgridif.cxx
+++ b/svx/source/fmcomp/fmgridif.cxx
@@ -2442,6 +2442,9 @@ Any FmXGridPeer::getByIndex(sal_Int32 _nIndex) throw( IndexOutOfBoundsException,
 // get the list position
 sal_uInt16 nPos = pGrid-GetModelColumnPos(nId);
 
+if ( nPos == GRID_COLUMN_NOT_FOUND )
+return aElement;
+
 DbGridColumn* pCol = pGrid-GetColumns().at( nPos );
 Reference ::com::sun::star::awt::XControl   xControl(pCol-GetCell());
 aElement = xControl;
-- 
1.7.2.5

From 17f18e8d57639f000565cdb57468b983f2412474 Mon Sep 17 00:00:00 2001
From: Lionel Elie Mamane lio...@mamane.lu
Date: Sun, 11 Sep 2011 23:44:41 +0200
Subject: [PATCH 3/3] Don't touch handle when setting property Enabled

Handle enabling/disabling is controlled by property RecordMarker,
not by property Enabled.

Source of the error is most probably a copy/paste error in
commit 448cd2de7919401a0a84cd6e5dd5ede276ddf28e
Author: Frank Schoenheit [fs] frank.schoenh...@oracle.com
Date:   Tue Nov 23 11:38:49 2010 +0100
---
 svx/source/fmcomp/fmgridif.cxx |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/svx/source/fmcomp/fmgridif.cxx b/svx/source/fmcomp/fmgridif.cxx
index a4146a9..222b2b5 100644
--- a/svx/source/fmcomp/fmgridif.cxx
+++ b/svx/source/fmcomp/fmgridif.cxx
@@ -2090,7 +2090,6 @@ void FmXGridPeer::setProperty( const ::rtl::OUString PropertyName, const Any V
 {
 sal_Bool bValue( sal_True );
 OSL_VERIFY( Value = bValue );
-pGrid-EnableHandle( bValue );
 
 // Im DesignModus nur das Datenfenster disablen
 // Sonst kann das Control nicht mehr konfiguriert werden
-- 
1.7.2.5

___
LibreOffice mailing list

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

2011-09-11 Thread Eike Rathke 
Hi Lionel,

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

  Eike

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

2011-09-11 Thread Lionel Elie Mamane 
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
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice