DbGridControl's confusion of BrowseBox's column number vs. column ID

[Lionel Elie Mamane]

On Fri, Mar 10, 2017 at 01:52:36PM +0100, Stephan Bergmann wrote:

> svtools' BrowseBox class (include/svtools/brwbox.hxx; effectively a
> collection of BrowseColumn via the BrowseBox::pCols member) apparently
> distinguishes between a column's index/position/number (i.e., the
> BrowseColumn's position within the BrowseBox::pCols vector) and its ID
> (i.e., the BrowseColumn::_nId member, svtools/source/brwbox/datwin.hxx).

> Now, BrowseBox::GetColumnAtXPosPixel (svtools/source/brwbox/brwbox1.cxx)
> apparently always returned a column number,

> while DbGridControl::GetModelColumnPos (svx/source/fmcomp/gridctrl.cxx)
> apparently always expected a column ID as input,


> But there is two occurrences of code in svx/source/fmcomp/gridctrl.cxx that
> feeds the output of GetColumnAtXPosPixel as input into GetModelColumnPos,
> i.e., misinterprets a column number as a column ID. Meanwhile that code
> lives in DbGridControl::StartDrag,

> [...]
> >     sal_uInt16 nColId = GetColumnAtXPosPixel(rPosPixel.X());
> >     long   nRow = GetRowAtYPosPixel(rPosPixel.Y());
> >     if (nColId != HandleColumnId && nRow >= 0)
> >     {
> >         if (GetDataWindow().IsMouseCaptured())
> >             GetDataWindow().ReleaseMouse();
> > 
> >         size_t Location = GetModelColumnPos( nColId );
> [...]

There are a few bugs around dragging columns that I know of, but they
don't look obviously directly caused by what you describe. Maybe in
combination with something else I'm not aware of. It is not known to
me that they appear only in the presence of hidden columns. One bug
that comes to mind is tdf#54021: dragging a column copies it instead
of moving it. But AFAIK this is a regression, so if this confusion was
"always" there... Maybe it was latent but exposed by another change.

> and DbGridControl::Command,
> 
> [...]
> >             sal_uInt16 nColId = GetColumnAtXPosPixel(rEvt.GetMousePosPixel().X());
> >             long   nRow = GetRowAtYPosPixel(rEvt.GetMousePosPixel().Y());
> > 
> >             if (nColId == HandleColumnId)
> >             {
> >                 executeRowContextMenu( nRow, rEvt.GetMousePosPixel() );
> >             }
> >             else if (canCopyCellText(nRow, nColId))
> >             {
> >                 ScopedVclPtrInstance<PopupMenu> aContextMenu(SVX_RES(RID_SVXMNU_CELL));
> >                 aContextMenu->RemoveDisabledEntries(true, true);
> >                 switch (aContextMenu->Execute(this, rEvt.GetMousePosPixel()))
> >                 {
> >                     case SID_COPY:
> >                         copyCellText(nRow, nColId);
> [...]
> 
> where DbGridControl::copyCellText (svx/source/fmcomp/gridctrl.cxx) calls
> GetModelColumnPos,
> 
> > void DbGridControl::copyCellText(sal_Int32 _nRow, sal_uInt16 _nColId)
> > {
> >     DBG_ASSERT(canCopyCellText(_nRow, _nColId), "DbGridControl::copyCellText: invalid call!");
> >     DbGridColumn* pColumn = m_aColumns[ GetModelColumnPos(_nColId) ];
> >     SeekRow(_nRow);
> >     OStringTransfer::CopyString( GetCurrentRowCellText( pColumn,m_xPaintRow ), this );
> > }

Look at DbGridControl::HideColumn:

    DbGridControl_Base::RemoveColumn(nId);
         // don't use my own RemoveColumn, this would remove it from m_aColumns, too

I now realise there are *three* numbers linked to a column:

 - position in m_aColumns
 - position in pCols
 - stored (immutable?) id

See also the big comment in DbGridControl::ColumnMoved, where they are
called (I think in that order):

 - model pos
 - view pos
 - ID

It looks like ID and model pos could always be the same *initially*
but it looks like this is not the case when BrowseBox::SetColumnPos /
DbGridControl::ColumnMoved is called.


My reading now is that DbGridControl::copyCellText gets a "view pos"
and passes it to GetModelColumnPos which expects an id and returns a
"model pos".


It starts to look like bugs will appear if you:

1) Open a document with a form with a table control.

2) Move a column (which may be possible only pogrammatically or after
   tdf#54021 is fixed)

3) Try to copy the cell text (you'll copy from the wrong column)

-- 
Lionel