Re: tdf#50916 : Calc : Dynamic column container

Hi All

It has been quite a while since I worked on this bug. Lately I have been
thinking about "a better way to handle row formattings".
In the current setting, if someone formats whole row, it is required that
we have all columns from 0 to MAXCOL allocated, which is bad when MAXCOL is
a big number.

The formatting attributes of a column are stored in ScColumn::pAttrArray (
type is ScAttrArray* )

The methods of ScAttrArray are mostly to set and get specific attributes in
addition to
SetPattern(), SetPatternArea(), GetPattern() and GetPatternRange() which
sets/gets whole set of formatting attributes for a row/row segment.

One of my ideas to solve the row formatting issue was to create and
maintain a ScAttrArray object in ScTable (say aRowAttrArray) to hold only
the row formattings.
In the ScTable *set* methods related to formatting we could check if the
request is for the column range 0 to MAXCOL (full row operation) and
store the specified row formatting in aRowAttrArray in addition to letting
the existing columns to receive the row formatting.

*For example* :

void ScTable::ApplyStyleArea( SCCOL nStartCol, SCROW nStartRow, SCCOL
nEndCol, SCROW nEndRow, const ScStyleSheet& rStyle )
{
    if (ValidColRow(nStartCol, nStartRow) && ValidColRow(nEndCol, nEndRow))
    {
        PutInOrder(nStartCol, nEndCol);
        PutInOrder(nStartRow, nEndRow);
        for (SCCOL i = nStartCol; i <= nEndCol; i++)
            aCol[i].ApplyStyleArea(nStartRow, nEndRow, rStyle);
    }
}

can be modified to :

void ScTable::ApplyStyleArea( SCCOL nStartCol, SCROW nStartRow, SCCOL
nEndCol, SCROW nEndRow, const ScStyleSheet& rStyle )
{
    if (ValidColRow(nStartCol, nStartRow) && ValidColRow(nEndCol, nEndRow))
    {
        PutInOrder(nStartCol, nEndCol);
        PutInOrder(nStartRow, nEndRow);

        if( nStartCol == 0 && nEndCol == MAXCOL )
        {
            aRowAttrArray.ApplyStyleArea(nStartRow, nEndRow,
const_cast<ScStyleSheet*>(&rStyle));
            SCCOL nLastCol = aCol.size() - 1;
            for (SCCOL i = 0; i <= nLastCol; i++)
                aCol[i].ApplyStyleArea(nStartRow, nEndRow, rStyle);
        }
        else
        {
            if ( aCol.size() <= nEndCol )
                aCol.CreateCol( nEndCol, nTab ); // This method has to be
added again as the commit for loplugin:unusedmethods removed it.
            for (SCCOL i = nStartCol; i <= nEndCol; i++)
                aCol[i].ApplyStyleArea(nStartRow, nEndRow, rStyle);
        }
    }
}

Now this aRowAttrArray can be used to instantiate pAttrArray of column to
be created later on as it represents the attributes of all columns that are
yet to be created.

Next we need to modify the Get methods of ScTable related to formatting in
a way that it will respond with
correct formatting on the not-yet-created columns.

*For example* :

const ScPatternAttr* ScTable::GetPattern( SCCOL nCol, SCROW nRow ) const
{
     if (ValidColRow(nCol,nRow))
         return aCol[nCol].GetPattern( nRow );
     else
     {
         OSL_FAIL("wrong column or row");
         return pDocument->GetDefPattern();      // for safety
     }
}

needs to be modified to :

const ScPatternAttr* ScTable::GetPattern( SCCOL nCol, SCROW nRow ) const
{
     if (!ValidColRow(nCol,nRow))
     {
         OSL_FAIL("wrong column or row");
         return pDocument->GetDefPattern();      // for safety
     }
     if ( nCol < aCol.size() )
         return aCol[nCol].GetPattern( nRow );
     return aRowAttrArray.GetPattern( nRow );
}




While the above idea might work for a new document getting edited; but I am
not sure what the situation is when a non trivial document is
loaded/saved. During file loading if row attributes get applied column by
column separately, it will defeat the idea presented in the sample code
above.
*For example*, a row stylesheet get applied by the import code by calling
either :

1) for ( nCol = 0; nCol <= MAXCOL; ++nCol)
       ScTable::ApplyStyle(nCol, nRow, rStyle)
or

2) ScTable::ApplyStyleArea( 0, nRow, MAXCOL, nRow, rStyle )

In case 2) we can avoid creating all columns by special handling, but not
in case 1)

In oox and excel import filters, it looks like attributes are applied
column by column (more like case 1)
See
http://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/sheetdatabuffer.cxx#496
and
http://opengrok.libreoffice.org/xref/core/sc/source/filter/excel/xistyle.cxx#2030
where it calls ScDocumentImport::setAttrEntries( SCTAB nTab, SCCOL nCol,
Attrs& rAttrs ), which in-turn sets the Attr entries by directly
manipulating pAttrArray of each column.
In oox and excel formats, I am not sure if there is a way to store row
attributes.

For ods doc, I could not find direct evidence on how row attributes are
applied, but I found out that ods has a way to store row attributes by
examining the content.xml of an ods file with a row formatting, so
the import code would definitely have a chance to call the Set*Area() or
Set*Range() or ApplyStyleArea() directly rather than setting attributes for
each column in a loop for the row attributes.


It would be great if someone could comment on the approach and offer some
advice on my import code doubts presented above.



Thanks a lot,
Dennis

On Wed, Jan 20, 2016 at 1:34 PM, Dennis Francis <dennisfrancis.in@gmail.com>
wrote:

> Hi All
>
> I have submitted a prelim patch at https://gerrit.libreoffice.org/21620
> following Kohei's suggestion of
> solving this bug incrementally.
>
> Thanks,
> Dennis
>
> On Wed, Oct 14, 2015 at 10:22 PM, Kohei Yoshida <libreoffice@kohei.us>
> wrote:
>
> > > On October 14, 2015 at 11:18 AM Wols Lists <antlists@youngman.org.uk>
> > wrote:
> > > On 14/10/15 02:08, Kohei Yoshida wrote:
> > > > Also, this will be an on-going process.  This is not going to be like
> > > > "if you do A, B and C it's okay to increase the column size and no
> > > > problems will occur".  Rather, we'll probably encounter lots of
> > > > performance issues that we'll have to spend some time fixing after the
> > > > column size is increased.
> > >
> > > Just throwing an idea into the mix...
> > >
> > > Howsabout a function that, on being given the cell co-ordinates, returns
> > > a pointer to the cell. Force everything through that.
> >
> > Its internal representation is no longer cell-based. So, there would be no
> > "pointer to cell" to return.
> > _______________________________________________
> > LibreOffice mailing list
> > LibreOffice@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/libreoffice