Fix for Issue #1887 - Lose Track of Selected Cells After Save #1908
Conversation
Issue #1887 reports that selected cells are lost after saving Xlsx. Testing indicates that this applies to the object in memory, though not to the saved spreadsheet. Xlsx writer tries to save calculated values for cells which contain formulas. Calculation::_calculateFormulaValue issues a getStyle call merely to retrieve the quotePrefix property, which, if set, indicates that the cell does not contain a formula even though it looks like one. A side-effect of calls to getStyle is that selectedCell is updated. That is clearly accidental, and highly undesirable, in this case. Code is changed to save selectedCell before getStyle call and restore it afterwards. The problem was reported only for Xlsx save. To be on the safe side, test is made for output formats of Xlsx, Xls, Ods, Html (which basically includes Pdf), and Csv. For all of those, the object in memory is tested after the save. For Xlsx and Xls, the saved file is also tested. It does not make sense to test the saved file for Csv and Html. It does make sense to test it for Ods, but the necessary support is not yet present in either the Ods Reader or Ods Writer - a project for another day.
|
Actually, I do not understand why getStyle should affect the selected cells at all. I chose this approach so as to have the minimum effect on users. If they frequently use getStyle for this purpose, then perhaps its best to leave it alone. On the other hand, why should they use in this way? There's a really simple API setSelectedCells available for precisely this purpose. So the application change required by anyone adversely affected would be really trivial. I would certainly be willing to eliminate the selectedCellls side effect of getStyle (which would mean we don't have to save and restore) if you feel that is the way we should proceed. |
|
No, that seems like a non-starter. Everything I try to do in Worksheet winds up with a ton of both errors and failures in the test suite. "Everything" includes:
I especially don't understand why the second option doesn't work; it seems like it should be somewhat equivalent to what's already been done. |
|
Aha! I can't move this to Worksheet, but I can move it to Cell. And there is justification for this - Cell::getCalculatedValue saves off ActiveSheetIndex before doing the calculation and restores it after. Saving and restoring SelectedCells here would be done for exactly the same reason. I can have a new version ready tomorrow. |
|
I have also figured out support for ODS ActiveSheet and SelectedCells for Reader and Writer. I will add it to tomorrow's change as well, unless you'd prefer me to schedule that for later. |
…lectedCells Mark Baker thought logic belonged in Worksheet, not Calculation. I couldn't get it to work in Worksheet, but doing it in Cell works, and that has already been used to preserve ActiveSheet over call to getCalculatedValue, so this just extends that idea to SelectedCells. Original tests could not completely support Ods because of a lack of support for ActiveSheet and SelectedCells in Ods Reader and Writer. There's a lot missing in Ods support, but a journey of 1000 miles ... Those two particular concepts are now supported for Ods.
There was a problem hiding this comment.
Nice work, and good to see selected cells added to ODS as well
I'm slightly concerned about calculations that call functions which in turn call the calculation engine again (e.g. the Database functions, or functions that assess conditions like COUNTIF(), MAXIFS(), etc... but I think this will be enough for now
Issue #1887 reports that selected cells are lost after saving Xlsx. Testing indicates that this applies to the object in memory, though not to the saved spreadsheet.
Xlsx writer tries to save calculated values for cells which contain formulas. Calculation::_calculateFormulaValue issues a getStyle call merely to retrieve the quotePrefix property, which, if set, indicates that the cell does not contain a formula even though it looks like one. A side-effect of calls to getStyle is that selectedCell is updated. That is clearly accidental, and highly undesirable, in this case. Code is changed to save selectedCell before getStyle call and restore it afterwards.
The problem was reported only for Xlsx save. To be on the safe side, test is made for output formats of Xlsx, Xls, Ods, Html (which basically includes Pdf), and Csv. For all of those, the object in memory is tested after the save. For Xlsx and Xls, the saved file is also tested. It does not make sense to test the saved file for Csv and Html. It does make sense to test it for Ods, but the necessary support is not yet present in either the Ods Reader or Ods Writer - a project for another day.
This is:
Checklist:
Why this change is needed?
Bug fix - see details above.