[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: some patches for kspread and series in general
From: Marc Heyvaert <marc_heyvaert () yahoo ! com>
Date: 2003-12-27 17:19:16
[Download RAW message or body]
See also my previous message with this subjectline :
Attached to this message are 3 files : patch2.diff ,
patch3.diff and patch4.diff
If you are working on kspread please read on, because
I really need your advice.
While testing this series feature it struck me that
building a large series was very slow. Building a
simple linear series of 10.000 numbers took on my
Athlon 2200 about 17 sec. Looking at the code, it
struck me that -I guess in order to avoid repeats in
the code- the conditions where built in a very
contorted way, e.g. putting a test like 'if
(type==linear) came al the way at the bottom of the
decision tree, meaning that for a long series, like
mine, it would be tested 10.000. So for speeding up
the code, I simplified the decision tree, but at the
price of more lines of code. I don't know if you agree
with this decision, but the spreed increased on my
machine from 17 sec. to 13 sec. for the same test.
(well also because I don't go checking for obscuring
cells when there are no merged cells in the first
place, I use a flag while checking the undo function)
But I think this is still unacceptable. I could still
notice a flicker in the screen, while the code runs,
so there was something going on with the screen. The
problem was with the setNumber(float) function that
was called, because this function always calls
update(), which emits a sig_updateView...so I rebuild
this introducing a default bool parameter of false (so
you can still use the setNumber(double) function as
before), which I set to false before calling the
function. One updateView and one recalc() at the end
takes care of everything...Now the result of my test
was 6 sec. versus the original 17 sec. But...
Still a lot of flickering and that has to do with the
looong routine that is used to determine the Qrect
that has to be saved in order to allow an undo. Now
bear with me just for a little bit more :
Undo doens't work for the moment in certain situations
where there are merged cells as explained in my
bug-report
http://bugs.kde.org/show_bug.cgi?id=70708
I have looked at the code, but I can't figure out what
is wrong. Seems ok, but doesn't work. Now I really
want to get rid of this time consuming code for a very
limited number of cases, especially because warning
when you are going to overwrite data is not even
catered for...
Now looking at MS Excel (not that it is my role model
perse) I see that they want you to select a rectangle
before you start the series dialog, and that they do
not allow series to be built in a rectangle that
contains merged cells?! I am not pleading for copying
Excell, but these design choices surely make their
life easier...
I think the options are :
1. Live with the broken undo :-(
2. Having the user select a rectangle before starting
(cumbersome if you want to construct long series) :-(
3. Ignoring merged cells and let the user do the
clean-up if a number has been 'misplaced' (fast so..)
:-)
4. Do 3 and use the time freed for checking if cell in
the range are empty and issue a warning if they are
not :-)
Well, perhaps there are more options. So please some
feedback on my patches and my ideas, and...
If my code is good enough, could someone please commit
it?
Regards
Marc
__________________________________
Do you Yahoo!?
New Yahoo! Photos - easier uploading and sharing.
http://photos.yahoo.com/
["patch2.diff" (application/octet-stream)]
--- /home/kdevelop/source/diffs/kspread_cell_orig.h 2003-11-08 00:55:44.000000000 +0000
+++ /home/kdevelop/source/diffs/kspread_cell.h 2003-12-25 15:28:49.000000000 +0000
@@ -451,7 +451,7 @@
void setDate( QString const & dateString );
void setDate( QDate const & date );
void setTime( QTime const & time );
- void setNumber( double number );
+ void setNumber( double number, bool updt=true );
const KSpreadValue value() const;
["patch3.diff" (application/octet-stream)]
--- /home/kdevelop/source/diffs/kspread_cell_orig.cc 2003-11-08 00:55:44.000000000 +0000
+++ /home/kdevelop/source/diffs/kspread_cell.cc 2003-12-25 15:29:19.000000000 +0000
@@ -4030,7 +4030,7 @@
update();
}
-void KSpreadCell::setNumber( double number )
+void KSpreadCell::setNumber( double number, bool updt )
{
clearAllErrors();
clearFormula();
@@ -4044,7 +4044,8 @@
setFlag( Flag_LayoutDirty );
setFlag( Flag_TextFormatDirty );
checkNumberFormat();
- update();
+ if (updt==true)
+ update();
}
void KSpreadCell::setCellText( const QString& _text, bool updateDepends, bool asText )
["patch4.diff" (application/octet-stream)]
--- /home/kdevelop/source/diffs/kspread_sheet_orig.cc 2003-11-24 00:44:15.000000000 \
+0000
+++ /home/kdevelop/source/diffs/kspread_sheet.cc 2003-12-27 17:25:54.000000000 +0000
@@ -1621,31 +1621,64 @@
{
doc()->emitBeginOperation();
- QString cellText;
+// calculate the number of elements in the series
+
+
+int numberOfCells = 0; //length of the series
+int x = 0; //loop counter
+int y = 0; //loop counter
+const int seriesMax = 10000; //to keep the delays in calculating acceptable
+int rowsRemaining = 0; //rows between selected cell and bottom of sheet
+int columnsRemaining = 0; //columns between selected cell and right-hand \
border of sheet +
+/* 2 cases for linear: (end >=start and step > 0) or end <= start and step < 0)
+ In both cases (end - start)/step > 0. Adding 1 will ensure that numberOfCells >= \
0 +*/
+
+ if (type == Linear)
+ numberOfCells = (int) ((end-start) / step + 1);
+
+/* only one case for geometric
+ n-th term = start * step^n
+ so number of elements in the series is reached when this n-th term equals the end \
value + so end = start * step^n
+ step^n = end/start
+ n = log(base step)(step^n)
+ = log(base step)(end/start)
+ = ln(end/start)/ln(step)
+ What was here before didn't work, step wasn't even in the equation!. This works \
for decreasing + and increasing series
+*/
- int x,y; /* just some loop counters */
- /* the actual number of columns or rows that the series will span.
- i.e. this will count 3 cells for a single cell that spans three rows
- */
- int numberOfCells;
- if (end > start)
- numberOfCells = (int) ((end - start) / step + 1); /*initialize for linear*/
- else if ( end <start )
- numberOfCells = (int) ((start - end) / step + 1); /*initialize for linear*/
- else //equal ! => one cell fix infini loop
- numberOfCells = 1;
if (type == Geometric)
- {
- /* basically, A(n) = start ^ n
- * so when does end = start ^ n ??
- * when n = ln(end) / ln(start)
- */
- numberOfCells = (int)( (log((double)end) / log((double)start)) +
- DBL_EPSILON) + 1;
- }
- KSpreadCell * cell = NULL;
+ numberOfCells = (int)( (log(end/start) / log(step)) + 1); //add 1 to take into \
account the starting value; +
+
+//here I have to check for the bounds of the sheet
+// 1st vertical
+
+ if (mode == Column)
+ rowsRemaining = KS_rowMax - _marker.y();
+
+// 2nd horizontal
+
+ if (mode == Row)
+ columnsRemaining = KS_colMax - _marker.x();
+
+
+/* Check for merged cells... If thy exist within the range it will affect :
+ -the length of the series in terms of cells needed
+ -the undo region
+ -a flag will be set to prevent from checking again when constructing the \
series. +
+ This module should also check for non empty cells and display a warning
+*/
+
+
+ KSpreadCell * cell = NULL;
+ bool mergedCells = false;
/* markers for the top-left corner of the undo region. It'll probably
* be the top left corner of where the series is, but if something in front
@@ -1668,13 +1701,15 @@
*/
if ( mode == Column )
{
- for ( y = _marker.y(); y <= (_marker.y() + numberOfCells - 1); y++ )
+ for ( y = _marker.y(); y <= (QMIN((_marker.y() + numberOfCells - \
1),rowsRemaining)); y++ ) {
cell = cellAt( _marker.x(), y );
if ( cell->isObscuringForced() )
{
- /* case 2. */
+ mergedCells = true;
+
+ /* case 2. */
cell = cell->obscuringCells().first();
undoRegion.setLeft(QMIN(undoRegion.left(), cell->column()));
}
@@ -1689,9 +1724,10 @@
undoRegion.setRight( _marker.x() );
undoRegion.setBottom( y - 1 );
}
+
else if(mode == Row)
{
- for ( x = _marker.x(); x <=(_marker.x() + numberOfCells - 1); x++ )
+ for ( x = _marker.x(); x <= (QMIN((_marker.x() + numberOfCells - 1), \
columnsRemaining)); x++ ) {
/* see the code above for a column series for a description of
what is going on here. */
@@ -1699,203 +1735,264 @@
if ( cell->isObscuringForced() )
{
- cell = cell->obscuringCells().first();
+ mergedCells = true;
+ cell = cell->obscuringCells().first();
undoRegion.setTop(QMIN(undoRegion.top(), cell->row()));
}
numberOfCells += cell->extraXCells();
x += cell->extraXCells();
}
+
undoRegion.setBottom( _marker.y() );
undoRegion.setRight( x - 1 );
- }
+ }
kdDebug() << "Saving undo information" << endl;
- if ( !m_pDoc->undoBuffer()->isLocked() )
- {
- KSpreadUndoChangeAreaTextCell *undo = new
- KSpreadUndoChangeAreaTextCell( m_pDoc, this, undoRegion );
- m_pDoc->undoBuffer()->appendUndo( undo );
- }
+ if ( !m_pDoc->undoBuffer()->isLocked() )
+ {
+ KSpreadUndoChangeAreaTextCell *undo = new
+ KSpreadUndoChangeAreaTextCell( m_pDoc, this, undoRegion );
+ m_pDoc->undoBuffer()->appendUndo( undo );
+ }
- kdDebug() << "Saving undo information done" << endl;
+ kdDebug() << "Saving undo information done" << endl;
- x = _marker.x();
- y = _marker.y();
+/* now we're going to actually loop through and set the values */
- /* now we're going to actually loop through and set the values */
- double incr;
- KSpreadStyle * s = doc()->styleManager()->defaultStyle();
- if (step >= 0 && start < end)
- {
- for ( incr = start; incr <= end; )
- {
- cell = nonDefaultCell( x, y, false, s );
+int i = 0; // a loop counter
+KSpreadStyle * s = doc()->styleManager()->defaultStyle();
+x = _marker.x();
+y = _marker.y();
+
+/* Now one big if-structure for each type of series, currently only 'linear' and \
'geometric' +This will make the number of lines grow in comparison with the previous \
version of this function, +but whole branches of this decision tree will never be \
executed for a particular case. This should +be more efficient...
+*/
- if ( cell->isObscuringForced() )
+ if (type == Linear)
+ {
+ if (mode == Column)
+ {
+ numberOfCells = QMIN(numberOfCells,seriesMax);
+ if (mergedCells == false)
{
- cell = cell->obscuringCells().first();
- }
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
- // cell->setCellText(cellText.setNum( incr ));
- // feed the change recorder
- if ( map() && map()->changes() )
- {
- map()->changes()->addChange( this, cell, QPoint( x, y ),
- cell->getFormatString( x, y ),
- cell->text(), !cell->isEmpty() );
- }
- cell->setNumber( incr );
- if (mode == Column)
+ cell->setNumber( val, false );
+ y++;
+ val = val + step;
+ }
+ }
+ else // case with merged celles
{
- ++y;
- if (cell->isForceExtraCells())
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
{
- y += cell->extraYCells();
+ cell = nonDefaultCell( x, y, false, s );
+ if ( cell->isObscuringForced() )
+ {
+ cell = cell->obscuringCells().first();
+ }
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
}
- }
- else if (mode == Row)
- {
- ++x;
+
+ cell->setNumber( val, false );
if (cell->isForceExtraCells())
{
- x += cell->extraXCells();
+ y += cell->extraYCells();
+ i += cell->extraYCells();
+ }
+ val = val + step;
+ y++;
}
- }
- else
- {
- kdDebug(36001) << "Error in Series::mode" << endl;
- return;
- }
-
- if (type == Linear)
- incr = incr + step;
- else if (type == Geometric)
- incr = incr * step;
- else
- {
- kdDebug(36001) << "Error in Series::type" << endl;
- return;
}
}
- }
- else
- if (step >= 0 && start > end)
- {
- for ( incr = start; incr >= end; )
+ else if (mode == Row)
{
- cell = nonDefaultCell( x, y, false, s );
-
- if (cell->isObscuringForced())
+ numberOfCells = QMIN(numberOfCells,seriesMax);
+ if (mergedCells == false)
{
- cell = cell->obscuringCells().first();
- }
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
- // feed the change recorder
- if ( map() && map()->changes() )
- {
- map()->changes()->addChange( this, cell, QPoint( x, y ),
- cell->getFormatString( x, y ),
- cell->text() );
+ cell->setNumber( val, false );
+ x++;
+ val = val + step;
+ }
}
- // cell->setCellText(cellText.setNum( incr ));
- cell->setNumber( incr );
- if (mode == Column)
+ else // case with merged celles
{
- ++y;
- if (cell->isForceExtraCells())
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
{
- y += cell->extraYCells();
+ cell = nonDefaultCell( x, y, false, s );
+ if ( cell->isObscuringForced() )
+ {
+ cell = cell->obscuringCells().first();
+ }
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
}
- }
- else if (mode == Row)
- {
- ++x;
+
+ cell->setNumber( val, false );
if (cell->isForceExtraCells())
{
- x += cell->extraXCells();
+ y += cell->extraYCells();
+ i += cell->extraYCells();
+ }
+ val = val + step;
+ x++;
}
}
- else
+ }
+ else
{
kdDebug(36001) << "Error in Series::mode" << endl;
return;
}
- if (type == Linear)
- incr = incr + step;
- else if (type == Geometric)
- incr = incr * step;
- else
- {
- kdDebug(36001) << "Error in Series::type" << endl;
- return;
- }
- }
- }
- else
- {
- for ( incr = start; incr <= end; )
- {
- cell = nonDefaultCell( x, y, false, s );
+ }
- if (cell->isObscuringForced())
- {
- cell = cell->obscuringCells().first();
- }
- // feed the change recorder
- if ( map() && map()->changes() )
+ if (type == Geometric)
+ {
+ if (mode == Column)
+ {
+ numberOfCells = QMIN(numberOfCells,seriesMax);
+ if (mergedCells == false)
{
- map()->changes()->addChange( this, cell, QPoint( x, y ),
- cell->getFormatString( x, y ),
- cell->text() );
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
+
+ cell->setNumber( val, false );
+ y++;
+ val = val * step;
+ }
}
- //cell->setCellText(cellText.setNum( incr ));
- cell->setNumber( incr );
- if (mode == Column)
+ else // case with merged celles
{
- ++y;
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( cell->isObscuringForced() )
+ {
+ cell = cell->obscuringCells().first();
+ }
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
+
+ cell->setNumber( val, false );
if (cell->isForceExtraCells())
{
y += cell->extraYCells();
+ i += cell->extraYCells();
+ }
+ val = val * step;
+ y++;
}
}
- else if (mode == Row)
+ }
+ else if (mode == Row)
+ {
+ numberOfCells = QMIN(numberOfCells,seriesMax);
+ if (mergedCells == false)
+ {
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
+
+ cell->setNumber( val, false );
+ x++;
+ val = val * step;
+ }
+ }
+ else // case with merged celles
{
- ++x;
+ double val = start;
+ for ( i = 1; i <= numberOfCells; i++ )
+ {
+ cell = nonDefaultCell( x, y, false, s );
+ if ( cell->isObscuringForced() )
+ {
+ cell = cell->obscuringCells().first();
+ }
+ if ( map() && map()->changes() )
+ {
+ map()->changes()->addChange( this, cell, QPoint( x, y ),
+ cell->getFormatString( x, y ),
+ cell->text(), !cell->isEmpty() );
+ }
+ cell->setNumber( val, false );
if (cell->isForceExtraCells())
{
- x += cell->extraXCells();
+ y += cell->extraYCells();
+ i += cell->extraYCells();
}
- }
- else
+ val = val * step;
+ x++;
+ }
+ }
+ }
+ else
{
kdDebug(36001) << "Error in Series::mode" << endl;
return;
}
- if (type == Linear)
- incr = incr + step;
- else if (type == Geometric)
- {
+ }
- incr = incr * step;
- //a step = 1 into geometric serie is not good
- //we don't increase value => infini loop
- if (step == 1)
- return;
- }
- else
- {
- kdDebug(36001) << "Error in Series::type" << endl;
- return;
- }
- }
+
+// doc()->emitEndOperation();
+
+ recalc();
+ emit sig_updateView( this );
}
- // doc()->emitEndOperation();
- emit sig_updateView( this );
-}
struct SetSelectionPercentWorker : public KSpreadSheet::CellWorkerTypeA
_______________________________________________
koffice-devel mailing list
koffice-devel@mail.kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic