Bug 58652 - cut / paste is not reflected in dependant formulas
Summary: cut / paste is not reflected in dependant formulas
Status: RESOLVED FIXED
Alias: None
Product: calligrasheets
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: HI grave
Target Milestone: ---
Assignee: Calligra Sheets (KSpread) Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2003-05-18 21:39 UTC by Ferdinand Gassauer
Modified: 2007-12-07 18:56 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ferdinand Gassauer 2003-05-18 21:39:44 UTC
Version:           1.2.90 (using KDE 3.1.9)
Compiler:          gcc version 3.3 20030226 (prerelease) (SuSE Linux)
OS:          Linux (i686) release 2.4.20-4GB

Hi!
example
A1=1
B1=2
C1=A1+B1

now cut/paste B1 to B2

C1 results in 1 which is wrong, because the formula remains A1+B1

cu
ferdinand
Comment 1 Ferdinand Gassauer 2003-07-08 05:18:52 UTC

*** This bug has been marked as a duplicate of 58094 ***
Comment 2 Ferdinand Gassauer 2005-09-28 23:04:48 UTC
unfortunately it is not the same bug, sorry!

BTW, no automatic recalculation is made to reflect the changes.

Only after manual recalculation the problem becomes "visible", the value changes, because the formula does NOT point to the new location.

Would be great if this could be fixed soon, because looking at the release schedule we wont't be able to get a usable kspread in the near future.
Comment 3 Robert Knight 2005-10-06 02:49:24 UTC
I suggest the severity of this should be changed to normal.  This is annoying, but not grave.
Comment 4 Ferdinand Gassauer 2005-10-06 06:54:24 UTC
IMHO it is a show stoppper and even critical!

IMHO the main purpose of a spread sheet ist to calculate and rearange the layout. If context of formulas is not maintained, the application is hardly usable.

kspread MUST conform in this respect to all major spread sheets on the market (excel, open office, gnumeric, lotus). 
(only planmaker , which I just discoverd now, behaves like kspread)

although I agree, that the current functionality might be usefull in some cases, as it is similar to copy/paste with erasing the source cell.
Comment 5 Raphael Langerhorst 2005-12-11 11:29:51 UTC
> Only after manual recalculation the problem becomes "visible",

This is now fixed. The formula is recalculated right away after "moving" a cell away.
Comment 6 Raphael Langerhorst 2005-12-11 11:37:15 UTC
Wasn't this issue discussed a long time ago already? At least I'm not convinced that KSpread should automagically change a formula, just when cells are moved around.

Consider the following: A cell which is part of a formula contains a value that is precious to me. So if I want to enter a new value but don't want to overwrite the precious cell I first move it out of the way(!) and then enter a new value in the old place. Now, if all dependent formulas change their references that's certainly not something I wanted to achieve.

In particular I think it's bad behaviour if KSpread changes formula content without telling the user. On the other hand warning an user if he moves cell content out of the way (so the formula is maybe incorrect) might be useful. IIRC we played around with this notification and found it unusable if many formulas reference one cell (which is moved).

So... what is currently your POV, is it ok if KSpread doesn't magically touch formulas just when cells are moved around? This the way KSpread works right now. If that's the case the bug can again be closed.
Comment 7 Ferdinand Gassauer 2005-12-14 14:15:26 UTC
Yes the issue is around for a long time,

IMHO there heave been many reasons for not dealing with this problem (available time, change of maintainer, rewriting of the formula engine ...)

I can agree with your point that SOMETIMES it makes sense to preserve the "precious" value of a cell, which in all spreadsheets can be done by copy / (special) paste.

But nevertheless kspread 
a) needs a method to move cell around WITH changing all dependent formulas.
b) should conform to the standard used by OO, Excel, Gnumeric etc.

propably there are 2 kinds of problems
the moving cell is a
a) value
b) formula itself.

IMHO copy / (special) paste [no formulas] solves the problem of preserving "precious" values for (b)

hope that helps
Comment 8 Ferdinand Gassauer 2005-12-18 07:07:13 UTC
may be I am missing something,
but how can I move some cells (a range of cells) of a sheet to another without loosing the formulas. example, because having all on one page gets to complex.


Comment 9 Raphael Langerhorst 2006-01-04 11:31:57 UTC
I see you have a point in this (moving a range of cells). Maybe there should be an option (or a question dialog) when "moving" cells - whether all dependent formulas should refer to the new location. Maybe it should even be possible to check the dependent formulas one-by-one and let the user chose which formulas should be updated.
Comment 10 Ferdinand Gassauer 2006-01-04 12:56:07 UTC
cut/paste is what other spreadsheets do by default and what you are calling "move".
all formulas are adapted automatically. 

I personally do not see a big necessity to implement the checks which formula should be adapted and which one not, except for one situation.

- if the "moved" cells are referenced by aggregate formulas like sum and not all cells of the aggregate formula are moved. this causes usually wrong results. 
OO only adapts the formula only if all cells are in the range which is moved.

IMHO it would be nice to get a warning in the case, if formulas are not adapted.


cut/"special paste without formulas" is what kspread currently does.

Comment 11 Philippe Rigault 2006-02-02 15:16:23 UTC
As of koffice-1.5beta1, this bug is now fixed. The cell (C1) recalculates the result of the formula as soon as any of its input variables is modified.

However, there is a new bug concerning the various ways of entering the
formula in C1 as the sum of A1 and B1.
See bug 121229
Comment 12 Robert Knight 2006-02-02 20:25:34 UTC
Thanks - we can finally mark this one as sorted :)
Comment 13 Ferdinand Gassauer 2006-02-03 07:56:55 UTC
unfortunately the example given in the bug opening comment is still not working.

did someone realy try it?

what seems to work is the immediate recalculation of cell C1, but this is not the bug.
Comment 14 Raphael Langerhorst 2006-02-03 22:34:05 UTC
Be careful with closing this bug, it is for real. For now we can't deal with it for the 1.5 release as we are in message freeze (see below for reason). Of course fixing bugs is actually allowed even in message freeze (so if anyone feels like having the time to deal with this now...).

I would suggest to deal with this situation in a clean way. This means neither doing one (not adapting depending formulas) or the other (adapting the dependant formulas), but make such questionable user interactions controlable by the user. This definitely requires either a dialog asking whether to update dependencies or not - or at least some way for the user to control how moves should be handled regarding dependencies.

A possibility would also be a setting in the kspread configuration itself, which can be set to:

* update dependencies
* don't update dependencies
* always ask

... sth like that. And that requires string changes. Maybe there is even a nicer solution that I don't see. In any case this requires some thought on how to deal with the dependency recalculations themselves.

So I would suggest to tackle that issue for the next major (2.0) KOffice release. And do it right (that is, make no assumptions what the user would expect and give him the choice, also make it possible to always use that choice if one of the ways is the user's preferred way).
Comment 15 Ferdinand Gassauer 2006-02-05 11:54:44 UTC
IMHO the proposition is something for post 1.5, but the missing "update dependancy" is a show stopper for serious use.
Comment 16 Robert Knight 2006-02-05 14:27:41 UTC
> This definitely requires either a dialog asking whether to update 
> dependencies or not - or at least some way for the user to control 
> how moves should be handled regarding dependencies. 

I think that adding an option to the Special Paste dialog would be preferable.  A modal dialog that appeared every time you moved any cells upon which formulae were dependant would be extremely irritating.  

Thinking about it, does this behaviour really need to be configurable?  If you wanted to move a cell as in your earlier scenario, you would just copy and paste the value, then erase the original cell.  I notice that neither Gnumeric nor OpenOffice appear to offer a way of customising this behaviour.  

Comment 17 Ferdinand Gassauer 2006-02-05 15:04:34 UTC
Please!
copy/past is not cut/paste

copy/paste copys the source cells with their absolute and relativ formulas 
dependant cells are NEVER affected.

cut/paste cuts the source cells with their absolute and relativ formulas 
dependant cells are ALWAYS affected (except if the cutted cells are only part of a range with exceeds the cut range)
Comment 18 Robert Knight 2006-02-05 15:23:35 UTC
> Please! 
> copy/past is not cut/paste 
 
We know that, why are you telling us?
Comment 19 Ferdinand Gassauer 2006-02-05 16:07:45 UTC
I quote Roberts statement
"Thinking about it, does this behaviour really need to be configurable?  If you wanted to move a cell as in your earlier scenario, you would just copy and paste the value, then erase the original cell.  I notice that neither Gnumeric nor OpenOffice appear to offer a way of customising this behaviour."

I do not know why this discussion comes up again and again.
Robert you are completely right, 
cut/paste does not need to be configurable, it just has to be implemented in a correct way ;-) (and IMHO an implementation does not affect the message freeze as it is below the surface, and IMHO again it is a functional bug)
copy/special paste does what sometimes is needed
copy/paste is what we have.
Comment 20 Ferdinand Gassauer 2006-03-11 18:35:42 UTC
Any news on this topic?
Comment 21 Sebastian Sauer 2006-08-19 18:48:28 UTC
Still valid for KSpread as in KOffice 1.6.
Comment 22 Stefan Nikolaus 2006-08-20 21:32:29 UTC
SVN commit 575099 by nikolaus:

Operations	Cut & paste, insertion/deletion of cells
		Update formulas refering to parts of the moved region.
CCBUG: 58652


 M  +85 -0     DependencyManager.cpp  
 M  +18 -3     DependencyManager.h  
 M  +11 -0     Sheet.cpp  


--- trunk/koffice/kspread/DependencyManager.cpp #575098:575099
@@ -214,6 +214,91 @@
   return d->dependencies;
 }
 
+void DependencyManager::regionMoved( const Region& movedRegion, const Region::Point& destination )
+{
+  Region::Point locationOffset( destination.pos() - movedRegion.boundingRect().topLeft() );
+
+  Region::ConstIterator end( movedRegion.constEnd() );
+  for ( Region::ConstIterator it( movedRegion.constBegin() ); it != end; ++it )
+  {
+    Sheet* const sheet = (*it)->sheet();
+    locationOffset.setSheet( ( sheet == destination.sheet() ) ? 0 : destination.sheet() );
+    const QRect range = (*it)->rect();
+
+    if ( d->dependants.contains( sheet ) )
+    {
+      const QRectF rangeF = QRectF( range ).adjusted( 0, 0, -0.1, -0.1 );
+      QList<Region::Point> dependantLocations = d->dependants[sheet]->intersects( rangeF );
+
+      for ( int i = 0; i < dependantLocations.count(); ++i )
+      {
+        const Region::Point location = dependantLocations[i];
+        Cell* const cell = location.sheet()->cellAt( location.pos().x(), location.pos().y() );
+        updateFormula( cell, (*it), locationOffset );
+      }
+    }
+  }
+}
+
+void DependencyManager::updateFormula( Cell* cell, const Region::Element* oldLocation, const Region::Point& offset )
+{
+  // Not a formula -> no dependencies
+  if (!cell->isFormula())
+    return;
+
+  // Broken formula -> meaningless dependencies
+  // (tries to avoid cell->formula() being null)
+  if (cell->testFlag(Cell::Flag_ParseError))
+    return;
+
+  const Formula* formula = cell->formula();
+  if ( !formula )
+  {
+    kDebug() << "Cell at row " << cell->row() << ", col " << cell->column() << " marked as formula, but formula is 0. Formula string: " << cell->text() << endl;
+    return;
+  }
+
+  Tokens tokens = formula->tokens();
+
+  //return empty list if the tokens aren't valid
+  if (!tokens.valid())
+    return;
+
+  QString expression = "=";
+  Sheet* sheet = cell->sheet();
+  Region region;
+  for( int i = 0; i < tokens.count(); i++ )
+  {
+    Token token = tokens[i];
+    Token::Type tokenType = token.type();
+
+    //parse each cell/range and put it to our RangeList
+    if (tokenType == Token::Cell || tokenType == Token::Range)
+    {
+      const Region region( sheet->workbook(), token.text(), sheet );
+      const Region::Element* element = *region.constBegin();
+
+      kDebug() << region.name() << endl;
+      // the offset contains a sheet, only if it was an intersheet move.
+      if ( ( oldLocation->sheet() == element->sheet() ) &&
+           ( oldLocation->rect().contains( element->rect() ) ) )
+      {
+        const Region yetAnotherRegion( element->rect().translated( offset.pos() ), offset.sheet() ? offset.sheet() : sheet );
+        expression.append( yetAnotherRegion.name( sheet ) );
+      }
+      else
+      {
+        expression.append( token.text() );
+      }
+    }
+    else
+    {
+      expression.append( token.text() );
+    }
+  }
+  cell->setCellText( expression );
+}
+
 void DependencyManager::Private::reset ()
 {
   dependencies.clear();
--- trunk/koffice/kspread/DependencyManager.h #575098:575099
@@ -63,15 +63,30 @@
    */
   Region getDependants(const Cell* cell);
 
+  /**
+   * Adjusts formulas after cut & paste operations or column/row insertions/deletions.
+   *
+   * \param movedRegion the region, that was moved
+   * \param destination the new upper left corner of the region
+   */
+  void regionMoved( const Region& movedRegion, const Region::Point& destination );
+
 protected:
   QMap<Region::Point, Region> dependencies() const;
 
-  /** local d-pointer */
+  /**
+   * \param cell the cell which formula should  be altered
+   * \param oldLocation the location/range, that was cut
+   * \param offset the relative movement and new sheet, if applicable
+   *
+   * \see regionMoved()
+   */
+  void updateFormula( Cell* cell, const Region::Element* oldLocation, const Region::Point& offset );
+
   class Private;
   Private * const d;
 };
 
-//end of namespace
-}
+} // namespace KSpread
 
 #endif // KSPREAD_DEPENDENCIES
--- trunk/koffice/kspread/Sheet.cpp #575098:575099
@@ -2024,6 +2024,7 @@
 void Sheet::cutSelection( Selection* selectionInfo )
 {
     QDomDocument doc = saveCellRegion(*selectionInfo, true, true);
+    doc.documentElement().setAttribute( "cut", selectionInfo->Region::name() );
 
     // Save to buffer
     QBuffer buffer;
@@ -2146,6 +2147,16 @@
   }
 
   KoXmlElement root = doc.documentElement(); // "spreadsheet-snippet"
+  if ( root.hasAttribute( "cut" ) )
+  {
+    const Region cutRegion( workbook(), root.attribute( "cut" ), this );
+    if ( cutRegion.isValid() )
+    {
+      Region::Point destination( pasteArea.topLeft() );
+      destination.setSheet( this );
+      workbook()->dependencyManager()->regionMoved( cutRegion, destination );
+    }
+  }
 
   int rowsInClpbrd    =  root.attribute( "rows" ).toInt();
   int columnsInClpbrd =  root.attribute( "columns" ).toInt();
Comment 23 Ferdinand Gassauer 2006-10-14 10:06:04 UTC
can this patch be backported ? to 1.6.1 ?

it would make kspread much more usefull and enterprise ready.
Comment 24 Ferdinand Gassauer 2007-12-07 18:56:21 UTC
fixed in 2.0 alpha 5