From 13b62becddfecb1c129132704884d8ad81799ea5 Mon Sep 17 00:00:00 2001 From: oleibman Date: Wed, 10 Mar 2021 12:23:08 -0800 Subject: [PATCH] Fix for Issue #1887 - Lose Track of Selected Cells After Save (#1908) * Fix for Issue #1887 - Lose Track of Selected Cells After Save 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. * Move Logic Out of Calculation, Add Support for Ods ActiveSheet and SelectedCells 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. --- src/PhpSpreadsheet/Cell/Cell.php | 2 + src/PhpSpreadsheet/Reader/Ods.php | 72 +++++++++++++++++ src/PhpSpreadsheet/Writer/Ods/Settings.php | 51 ++++++++++-- .../Calculation/CalculationTest.php | 9 +++ .../Reader/Ods/OdsTest.php | 14 ++++ .../Writer/RetainSelectedCellsTest.php | 77 +++++++++++++++++++ 6 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Writer/RetainSelectedCellsTest.php diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php index 5dee411b5f..f971a3c820 100644 --- a/src/PhpSpreadsheet/Cell/Cell.php +++ b/src/PhpSpreadsheet/Cell/Cell.php @@ -252,9 +252,11 @@ public function getCalculatedValue($resetLog = true) if ($this->dataType == DataType::TYPE_FORMULA) { try { $index = $this->getWorksheet()->getParent()->getActiveSheetIndex(); + $selected = $this->getWorksheet()->getSelectedCells(); $result = Calculation::getInstance( $this->getWorksheet()->getParent() )->calculateCellValue($this, $resetLog); + $this->getWorksheet()->setSelectedCells($selected); $this->getWorksheet()->getParent()->setActiveSheetIndex($index); // We don't yet handle array returns if (is_array($result)) { diff --git a/src/PhpSpreadsheet/Reader/Ods.php b/src/PhpSpreadsheet/Reader/Ods.php index 4ceac65301..59d934befb 100644 --- a/src/PhpSpreadsheet/Reader/Ods.php +++ b/src/PhpSpreadsheet/Reader/Ods.php @@ -23,6 +23,7 @@ use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Style\NumberFormat; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; +use Throwable; use XMLReader; use ZipArchive; @@ -646,10 +647,81 @@ public function loadIntoExisting($pFilename, Spreadsheet $spreadsheet) $this->readDefinedExpressions($spreadsheet, $workbookData, $tableNs); } $spreadsheet->setActiveSheetIndex(0); + + if ($zip->locateName('settings.xml') !== false) { + $this->processSettings($zip, $spreadsheet); + } // Return return $spreadsheet; } + private function processSettings(ZipArchive $zip, Spreadsheet $spreadsheet): void + { + $dom = new DOMDocument('1.01', 'UTF-8'); + $dom->loadXML( + $this->securityScanner->scan($zip->getFromName('settings.xml')), + Settings::getLibXmlLoaderOptions() + ); + //$xlinkNs = $dom->lookupNamespaceUri('xlink'); + $configNs = $dom->lookupNamespaceUri('config'); + //$oooNs = $dom->lookupNamespaceUri('ooo'); + $officeNs = $dom->lookupNamespaceUri('office'); + $settings = $dom->getElementsByTagNameNS($officeNs, 'settings') + ->item(0); + $this->lookForActiveSheet($settings, $spreadsheet, $configNs); + $this->lookForSelectedCells($settings, $spreadsheet, $configNs); + } + + private function lookForActiveSheet(DOMNode $settings, Spreadsheet $spreadsheet, string $configNs): void + { + foreach ($settings->getElementsByTagNameNS($configNs, 'config-item') as $t) { + if ($t->getAttributeNs($configNs, 'name') === 'ActiveTable') { + try { + $spreadsheet->setActiveSheetIndexByName($t->nodeValue); + } catch (Throwable $e) { + // do nothing + } + + break; + } + } + } + + private function lookForSelectedCells(DOMNode $settings, Spreadsheet $spreadsheet, string $configNs): void + { + foreach ($settings->getElementsByTagNameNS($configNs, 'config-item-map-named') as $t) { + if ($t->getAttributeNs($configNs, 'name') === 'Tables') { + foreach ($t->getElementsByTagNameNS($configNs, 'config-item-map-entry') as $ws) { + $setRow = $setCol = ''; + $wsname = $ws->getAttributeNs($configNs, 'name'); + foreach ($ws->getElementsByTagNameNS($configNs, 'config-item') as $configItem) { + $attrName = $configItem->getAttributeNs($configNs, 'name'); + if ($attrName === 'CursorPositionX') { + $setCol = $configItem->nodeValue; + } + if ($attrName === 'CursorPositionY') { + $setRow = $configItem->nodeValue; + } + } + $this->setSelected($spreadsheet, $wsname, $setCol, $setRow); + } + + break; + } + } + } + + private function setSelected(Spreadsheet $spreadsheet, string $wsname, string $setCol, string $setRow): void + { + if (is_numeric($setCol) && is_numeric($setRow)) { + try { + $spreadsheet->getSheetByName($wsname)->setSelectedCellByColumnAndRow($setCol + 1, $setRow + 1); + } catch (Throwable $e) { + // do nothing + } + } + } + /** * Recursively scan element. * diff --git a/src/PhpSpreadsheet/Writer/Ods/Settings.php b/src/PhpSpreadsheet/Writer/Ods/Settings.php index d458e8c2bd..301daf0335 100644 --- a/src/PhpSpreadsheet/Writer/Ods/Settings.php +++ b/src/PhpSpreadsheet/Writer/Ods/Settings.php @@ -2,6 +2,7 @@ namespace PhpOffice\PhpSpreadsheet\Writer\Ods; +use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Shared\XMLWriter; use PhpOffice\PhpSpreadsheet\Spreadsheet; @@ -16,7 +17,6 @@ class Settings extends WriterPart */ public function write(?Spreadsheet $spreadsheet = null) { - $objWriter = null; if ($this->getParentWriter()->getUseDiskCaching()) { $objWriter = new XMLWriter(XMLWriter::STORAGE_DISK, $this->getParentWriter()->getDiskCachingDirectory()); } else { @@ -39,13 +39,52 @@ public function write(?Spreadsheet $spreadsheet = null) $objWriter->writeAttribute('config:name', 'ooo:view-settings'); $objWriter->startElement('config:config-item-map-indexed'); $objWriter->writeAttribute('config:name', 'Views'); - $objWriter->endElement(); - $objWriter->endElement(); + $objWriter->startElement('config:config-item-map-entry'); + $spreadsheet = $spreadsheet ?? $this->getParentWriter()->getSpreadsheet(); + + $objWriter->startElement('config:config-item'); + $objWriter->writeAttribute('config:name', 'ViewId'); + $objWriter->writeAttribute('config:type', 'string'); + $objWriter->text('view1'); + $objWriter->endElement(); // ViewId + $objWriter->startElement('config:config-item-map-named'); + $objWriter->writeAttribute('config:name', 'Tables'); + foreach ($spreadsheet->getWorksheetIterator() as $ws) { + $objWriter->startElement('config:config-item-map-entry'); + $objWriter->writeAttribute('config:name', $ws->getTitle()); + $selected = $ws->getSelectedCells(); + if (preg_match('/^([a-z]+)([0-9]+)/i', $selected, $matches) === 1) { + $colSel = Coordinate::columnIndexFromString($matches[1]) - 1; + $rowSel = (int) $matches[2] - 1; + $objWriter->startElement('config:config-item'); + $objWriter->writeAttribute('config:name', 'CursorPositionX'); + $objWriter->writeAttribute('config:type', 'int'); + $objWriter->text($colSel); + $objWriter->endElement(); + $objWriter->startElement('config:config-item'); + $objWriter->writeAttribute('config:name', 'CursorPositionY'); + $objWriter->writeAttribute('config:type', 'int'); + $objWriter->text($rowSel); + $objWriter->endElement(); + } + $objWriter->endElement(); // config:config-item-map-entry + } + $objWriter->endElement(); // config:config-item-map-named + $wstitle = $spreadsheet->getActiveSheet()->getTitle(); + $objWriter->startElement('config:config-item'); + $objWriter->writeAttribute('config:name', 'ActiveTable'); + $objWriter->writeAttribute('config:type', 'string'); + $objWriter->text($wstitle); + $objWriter->endElement(); // config:config-item ActiveTable + + $objWriter->endElement(); // config:config-item-map-entry + $objWriter->endElement(); // config:config-item-map-indexed Views + $objWriter->endElement(); // config:config-item-set ooo:view-settings $objWriter->startElement('config:config-item-set'); $objWriter->writeAttribute('config:name', 'ooo:configuration-settings'); - $objWriter->endElement(); - $objWriter->endElement(); - $objWriter->endElement(); + $objWriter->endElement(); // config:config-item-set ooo:configuration-settings + $objWriter->endElement(); // office:settings + $objWriter->endElement(); // office:document-settings return $objWriter->getData(); } diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index 8e339207f5..337501f96e 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -4,6 +4,7 @@ use PhpOffice\PhpSpreadsheet\Calculation\Calculation; use PhpOffice\PhpSpreadsheet\Calculation\Functions; +use PhpOffice\PhpSpreadsheet\Cell\DataType; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PHPUnit\Framework\TestCase; @@ -159,6 +160,14 @@ public function testCellSetAsQuotedText(): void $cell->getStyle()->setQuotePrefix(true); self::assertEquals("=cmd|'/C calc'!A0", $cell->getCalculatedValue()); + + $cell2 = $workSheet->getCell('A2'); + $cell2->setValueExplicit('ABC', DataType::TYPE_FORMULA); + self::assertEquals('ABC', $cell2->getCalculatedValue()); + + $cell3 = $workSheet->getCell('A3'); + $cell3->setValueExplicit('=', DataType::TYPE_FORMULA); + self::assertEquals('', $cell3->getCalculatedValue()); } public function testCellWithDdeExpresion(): void diff --git a/tests/PhpSpreadsheetTests/Reader/Ods/OdsTest.php b/tests/PhpSpreadsheetTests/Reader/Ods/OdsTest.php index 0160f68d62..2cc5377a3d 100644 --- a/tests/PhpSpreadsheetTests/Reader/Ods/OdsTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Ods/OdsTest.php @@ -101,6 +101,20 @@ public function testLoadOneWorksheet(): void self::assertEquals('Sheet1', $spreadsheet->getSheet(0)->getTitle()); } + public function testLoadOneWorksheetNotActive(): void + { + $filename = 'tests/data/Reader/Ods/data.ods'; + + // Load into this instance + $reader = new Ods(); + $reader->setLoadSheetsOnly(['Second Sheet']); + $spreadsheet = $reader->load($filename); + + self::assertEquals(1, $spreadsheet->getSheetCount()); + + self::assertEquals('Second Sheet', $spreadsheet->getSheet(0)->getTitle()); + } + public function testLoadBadFile(): void { $this->expectException(ReaderException::class); diff --git a/tests/PhpSpreadsheetTests/Writer/RetainSelectedCellsTest.php b/tests/PhpSpreadsheetTests/Writer/RetainSelectedCellsTest.php new file mode 100644 index 0000000000..c1a57eb541 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Writer/RetainSelectedCellsTest.php @@ -0,0 +1,77 @@ +getActiveSheet(); + $sheet->setCellValue('A1', '=SIN(1)') + ->setCellValue('A2', '=SIN(2)') + ->setCellValue('A3', '=SIN(3)') + ->setCellValue('B1', '=SIN(4)') + ->setCellValue('B2', '=SIN(5)') + ->setCellValue('B3', '=SIN(6)') + ->setCellValue('C1', '=SIN(7)') + ->setCellValue('C2', '=SIN(8)') + ->setCellValue('C3', '=SIN(9)'); + $sheet->setSelectedCell('A3'); + $sheet = $spreadsheet->createSheet(); + $sheet->setCellValue('A1', '=SIN(1)') + ->setCellValue('A2', '=SIN(2)') + ->setCellValue('A3', '=SIN(3)') + ->setCellValue('B1', '=SIN(4)') + ->setCellValue('B2', '=SIN(5)') + ->setCellValue('B3', '=SIN(6)') + ->setCellValue('C1', '=SIN(7)') + ->setCellValue('C2', '=SIN(8)') + ->setCellValue('C3', '=SIN(9)'); + $sheet->setSelectedCell('B1'); + $sheet = $spreadsheet->createSheet(); + $sheet->setCellValue('A1', '=SIN(1)') + ->setCellValue('A2', '=SIN(2)') + ->setCellValue('A3', '=SIN(3)') + ->setCellValue('B1', '=SIN(4)') + ->setCellValue('B2', '=SIN(5)') + ->setCellValue('B3', '=SIN(6)') + ->setCellValue('C1', '=SIN(7)') + ->setCellValue('C2', '=SIN(8)') + ->setCellValue('C3', '=SIN(9)'); + $sheet->setSelectedCell('C2'); + $spreadsheet->setActiveSheetIndex(1); + + $reloaded = $this->writeAndReload($spreadsheet, $format); + self::assertEquals('A3', $spreadsheet->getSheet(0)->getSelectedCells()); + self::assertEquals('B1', $spreadsheet->getSheet(1)->getSelectedCells()); + self::assertEquals('C2', $spreadsheet->getSheet(2)->getSelectedCells()); + self::assertEquals(1, $spreadsheet->getActiveSheetIndex()); + // SelectedCells and ActiveSheet don't make sense for Html, Csv. + if ($format === 'Xlsx' || $format === 'Xls' || $format === 'Ods') { + self::assertEquals('A3', $reloaded->getSheet(0)->getSelectedCells()); + self::assertEquals('B1', $reloaded->getSheet(1)->getSelectedCells()); + self::assertEquals('C2', $reloaded->getSheet(2)->getSelectedCells()); + self::assertEquals(1, $reloaded->getActiveSheetIndex()); + } + } +}