From 46ee63647810fab8ad2fc528730bda53f56f1fad Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Tue, 14 Jan 2020 19:44:06 -0800 Subject: [PATCH 1/4] Active Cell Anywhere When freeze pane is in use on a worksheet, PhpSpreadsheet saves to Xlsx in such a way that the active cell is always set to the top left cell below the freeze pane. I find it difficult to understand why: 1. You have given users the setSelectedCells function, but then choose to ignore it. 2. Excel itself does not act in this manner. 3. PHPExcel did not act in this manner. 4. PhpSpreadsheet when writing to Xls does not act in this manner. This is especially emphasized because the one test in FreezePaneTest which would expose the difference is the only test in that member which is not made for both Xls and Xlsx. 5. It is *really* useful to be able to open a spreadsheet anywhere, even when it has header rows. Rather than create regression problems by changing this behavior all the time, I instead introduce a new method, setActiveCellAnywhere, to Writer/Xlsx/Worksheet to control it via a private variable, with the default (false) being the current behavior. When it is set to true, the active cell is left as the user intended. The documentation is updated to describe the new method. New tests are added, and the Xlsx-only test is changed to test both Xls and Xlsx. --- docs/topics/reading-and-writing-to-file.md | 14 +++ src/PhpSpreadsheet/Writer/Xlsx.php | 31 +++++ src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php | 6 +- .../Functional/AbstractFunctional.php | 6 +- .../Functional/FreezePaneTest.php | 112 +++++++++++++++++- 5 files changed, 160 insertions(+), 9 deletions(-) diff --git a/docs/topics/reading-and-writing-to-file.md b/docs/topics/reading-and-writing-to-file.md index 3b6a037cdc..c79fe27fc6 100644 --- a/docs/topics/reading-and-writing-to-file.md +++ b/docs/topics/reading-and-writing-to-file.md @@ -150,6 +150,20 @@ $writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); $writer->save("05featuredemo.xlsx"); ``` +#### Active cell + +By default, this writer will set the active cell on any worksheet which +uses freeze panes to the top left cell following the freeze pane. +You can instead set the active cell to be anywhere on the sheet +with the Worksheet setSelectedCells method +in conjuction with the following: + +``` php +$writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); +$writer->setActiveCellAnywhere(true); +$writer->save("05featuredemo.xlsx"); +``` + #### Formula pre-calculation By default, this writer pre-calculates all formulas in the spreadsheet. diff --git a/src/PhpSpreadsheet/Writer/Xlsx.php b/src/PhpSpreadsheet/Writer/Xlsx.php index 5889763902..ea9a7de14c 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx.php +++ b/src/PhpSpreadsheet/Writer/Xlsx.php @@ -34,6 +34,13 @@ class Xlsx extends BaseWriter */ private $office2003compatibility = false; + /** + * Active cell anywhere (true) or top left cell (false). + * + * @var bool + */ + private $activeCellAnywhere = false; + /** * Private writer parts. * @@ -555,4 +562,28 @@ public function setOffice2003Compatibility($pValue) return $this; } + + /** + * Get active cell anywhere. + * + * @return bool + */ + public function getActiveCellAnywhere() + { + return $this->activeCellAnywhere; + } + + /** + * Set active cell anywhere. + * + * @param bool $pValue active cell anywhere? + * + * @return Xlsx + */ + public function setActiveCellAnywhere($pValue) + { + $this->activeCellAnywhere = $pValue; + + return $this; + } } diff --git a/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php b/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php index bf811bdcd4..2a028a938b 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php +++ b/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php @@ -262,8 +262,10 @@ private function writeSheetViews(XMLWriter $objWriter, PhpspreadsheetWorksheet $ --$ySplit; $topLeftCell = $pSheet->getTopLeftCell(); - $activeCell = $topLeftCell; - $sqref = $topLeftCell; + if (!$this->getParentWriter()->getActiveCellAnywhere()) { + $activeCell = $topLeftCell; + $sqref = $topLeftCell; + } // pane $pane = 'topRight'; diff --git a/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php b/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php index da9f76e056..3818d861d0 100644 --- a/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php +++ b/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php @@ -18,13 +18,17 @@ abstract class AbstractFunctional extends TestCase * @param Spreadsheet $spreadsheet * @param string $format * @param null|callable $readerCustomizer + * @param null|callable $writerCustomizer * * @return Spreadsheet */ - protected function writeAndReload(Spreadsheet $spreadsheet, $format, callable $readerCustomizer = null) + protected function writeAndReload(Spreadsheet $spreadsheet, $format, callable $readerCustomizer = null, callable $writerCustomizer = null) { $filename = tempnam(File::sysGetTempDir(), 'phpspreadsheet-test'); $writer = IOFactory::createWriter($spreadsheet, $format); + if ($writerCustomizer) { + $writerCustomizer($writer); + } $writer->save($filename); $reader = IOFactory::createReader($format); diff --git a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php index fda67231bb..92f4b01d9c 100644 --- a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php +++ b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php @@ -38,20 +38,27 @@ public function testFreezePane($format) self::assertSame($topLeftCell, $actualTopLeftCell, 'should be able to set the top left cell'); } - public function providerFormatsInvalidSelectedCells() + public function providerFormatsAndActiveFlag() { return [ - ['Xlsx'], + ['Xls', true], + ['Xls', null], + ['Xls', false], + ['Xlsx', true], + ['Xlsx', null], + ['Xlsx', false], ]; } /** - * @dataProvider providerFormatsInvalidSelectedCells + * @dataProvider providerFormatsAndActiveFlag * * @param string $format + * @param mixed $actv */ - public function testFreezePaneWithInvalidSelectedCells($format) + public function testFreezePaneWithInvalidSelectedCells($format, $actv) { + $callback = $this->setCallback($format, $actv); $cellSplit = 'A7'; $topLeftCell = 'A24'; @@ -61,7 +68,7 @@ public function testFreezePaneWithInvalidSelectedCells($format) $worksheet->freezePane('A7', 'A24'); $worksheet->setSelectedCells('F5'); - $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format); + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); @@ -70,6 +77,99 @@ public function testFreezePaneWithInvalidSelectedCells($format) self::assertSame($cellSplit, $actualCellSplit, 'should be able to set freeze pane'); self::assertSame($topLeftCell, $actualTopLeftCell, 'should be able to set the top left cell'); - self::assertSame('A24', $reloadedActive->getSelectedCells(), 'selected cell should default to be first cell after the freeze pane'); + $expected = ($format === 'Xls' || $actv === true) ? 'F5' : 'A24'; + self::assertSame($expected, $reloadedActive->getSelectedCells()); + } + + public function setActiveTrue($writer) + { + $writer->setActiveCellAnywhere(true); + } + + public function setActiveFalse($writer) + { + $writer->setActiveCellAnywhere(false); + } + + public function setCallback($format, $actv) + { + if ($format === 'Xls') { + return null; + } + if ($actv === true) { + return [$this, 'setActiveTrue']; + } + if ($actv === false) { + return [$this, 'setActiveFalse']; + } + + return null; + } + + /** + * @dataProvider providerFormatsAndActiveFlag + * + * @param string $format + * @param mixed $actv + */ + public function testFreezePaneUserSelectedCell($format, $actv) + { + $callback = $this->setCallback($format, $actv); + $spreadsheet = new Spreadsheet(); + $worksheet = $spreadsheet->getActiveSheet(); + $worksheet = $spreadsheet->getActiveSheet(); + $worksheet->setCellValue('A1', 'Header1'); + $worksheet->setCellValue('B1', 'Header2'); + $worksheet->setCellValue('C1', 'Header3'); + $worksheet->setCellValue('A2', 'Data1'); + $worksheet->setCellValue('B2', 'Data2'); + $worksheet->setCellValue('C2', 'Data3'); + $worksheet->setCellValue('A3', 'Data4'); + $worksheet->setCellValue('B3', 'Data5'); + $worksheet->setCellValue('C3', 'Data6'); + $worksheet->freezePane('A2'); + $worksheet->setSelectedCells('C3'); + + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); + // Read written file + $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); + $selectedCell = $reloadedActive->getSelectedCells(); + + $expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; + self::assertSame($expected, $reloadedActive->getSelectedCells()); + } + + /** + * @dataProvider providerFormatsAndActiveFlag + * + * @param string $format + * @param mixed $actv + */ + public function testNoFreezePaneUserSelectedCell($format, $actv) + { + $callback = $this->setCallback($format, $actv); + $spreadsheet = new Spreadsheet(); + $worksheet = $spreadsheet->getActiveSheet(); + $worksheet = $spreadsheet->getActiveSheet(); + $worksheet->setCellValue('A1', 'Header1'); + $worksheet->setCellValue('B1', 'Header2'); + $worksheet->setCellValue('C1', 'Header3'); + $worksheet->setCellValue('A2', 'Data1'); + $worksheet->setCellValue('B2', 'Data2'); + $worksheet->setCellValue('C2', 'Data3'); + $worksheet->setCellValue('A3', 'Data4'); + $worksheet->setCellValue('B3', 'Data5'); + $worksheet->setCellValue('C3', 'Data6'); + //$worksheet->freezePane('A2'); + $worksheet->setSelectedCells('C3'); + + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); + // Read written file + $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); + $selectedCell = $reloadedActive->getSelectedCells(); + + //$expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; + $expected = 'C3'; + self::assertSame($expected, $reloadedActive->getSelectedCells()); } } From 23906dcae9fd199fd4f5adf564b34e222b8dc3df Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Tue, 14 Jan 2020 20:34:19 -0800 Subject: [PATCH 2/4] Scrutinizer Comments for Test Class Apply Scrutinizer recommendations. --- tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php index 92f4b01d9c..84bedcb422 100644 --- a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php +++ b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php @@ -117,7 +117,6 @@ public function testFreezePaneUserSelectedCell($format, $actv) $callback = $this->setCallback($format, $actv); $spreadsheet = new Spreadsheet(); $worksheet = $spreadsheet->getActiveSheet(); - $worksheet = $spreadsheet->getActiveSheet(); $worksheet->setCellValue('A1', 'Header1'); $worksheet->setCellValue('B1', 'Header2'); $worksheet->setCellValue('C1', 'Header3'); @@ -133,7 +132,6 @@ public function testFreezePaneUserSelectedCell($format, $actv) $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); - $selectedCell = $reloadedActive->getSelectedCells(); $expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; self::assertSame($expected, $reloadedActive->getSelectedCells()); @@ -150,7 +148,6 @@ public function testNoFreezePaneUserSelectedCell($format, $actv) $callback = $this->setCallback($format, $actv); $spreadsheet = new Spreadsheet(); $worksheet = $spreadsheet->getActiveSheet(); - $worksheet = $spreadsheet->getActiveSheet(); $worksheet->setCellValue('A1', 'Header1'); $worksheet->setCellValue('B1', 'Header2'); $worksheet->setCellValue('C1', 'Header3'); @@ -166,7 +163,6 @@ public function testNoFreezePaneUserSelectedCell($format, $actv) $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); - $selectedCell = $reloadedActive->getSelectedCells(); //$expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; $expected = 'C3'; From 817910ad84884e4ea6a21ade1d44a2cfe9c6a977 Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sun, 26 Jan 2020 22:29:24 -0800 Subject: [PATCH 3/4] Simplify Per Review Comments Not knowing best way to handle potential changes to existing behavior, existing change is more complex than needed by adding a new method to override existing behavior only on request. PowerKiKi reviewed request, and asked that it be simplified by modifying existing behavior, eliminating the need for override method. This change modifies the PR as requested. --- docs/topics/reading-and-writing-to-file.md | 14 ---- src/PhpSpreadsheet/Writer/Xlsx.php | 31 --------- src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php | 4 -- .../Functional/AbstractFunctional.php | 6 +- .../Functional/FreezePaneTest.php | 67 +++---------------- 5 files changed, 12 insertions(+), 110 deletions(-) diff --git a/docs/topics/reading-and-writing-to-file.md b/docs/topics/reading-and-writing-to-file.md index c79fe27fc6..3b6a037cdc 100644 --- a/docs/topics/reading-and-writing-to-file.md +++ b/docs/topics/reading-and-writing-to-file.md @@ -150,20 +150,6 @@ $writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); $writer->save("05featuredemo.xlsx"); ``` -#### Active cell - -By default, this writer will set the active cell on any worksheet which -uses freeze panes to the top left cell following the freeze pane. -You can instead set the active cell to be anywhere on the sheet -with the Worksheet setSelectedCells method -in conjuction with the following: - -``` php -$writer = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($spreadsheet); -$writer->setActiveCellAnywhere(true); -$writer->save("05featuredemo.xlsx"); -``` - #### Formula pre-calculation By default, this writer pre-calculates all formulas in the spreadsheet. diff --git a/src/PhpSpreadsheet/Writer/Xlsx.php b/src/PhpSpreadsheet/Writer/Xlsx.php index ea9a7de14c..5889763902 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx.php +++ b/src/PhpSpreadsheet/Writer/Xlsx.php @@ -34,13 +34,6 @@ class Xlsx extends BaseWriter */ private $office2003compatibility = false; - /** - * Active cell anywhere (true) or top left cell (false). - * - * @var bool - */ - private $activeCellAnywhere = false; - /** * Private writer parts. * @@ -562,28 +555,4 @@ public function setOffice2003Compatibility($pValue) return $this; } - - /** - * Get active cell anywhere. - * - * @return bool - */ - public function getActiveCellAnywhere() - { - return $this->activeCellAnywhere; - } - - /** - * Set active cell anywhere. - * - * @param bool $pValue active cell anywhere? - * - * @return Xlsx - */ - public function setActiveCellAnywhere($pValue) - { - $this->activeCellAnywhere = $pValue; - - return $this; - } } diff --git a/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php b/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php index 2a028a938b..1d5a995a8b 100644 --- a/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php +++ b/src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php @@ -262,10 +262,6 @@ private function writeSheetViews(XMLWriter $objWriter, PhpspreadsheetWorksheet $ --$ySplit; $topLeftCell = $pSheet->getTopLeftCell(); - if (!$this->getParentWriter()->getActiveCellAnywhere()) { - $activeCell = $topLeftCell; - $sqref = $topLeftCell; - } // pane $pane = 'topRight'; diff --git a/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php b/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php index 3818d861d0..da9f76e056 100644 --- a/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php +++ b/tests/PhpSpreadsheetTests/Functional/AbstractFunctional.php @@ -18,17 +18,13 @@ abstract class AbstractFunctional extends TestCase * @param Spreadsheet $spreadsheet * @param string $format * @param null|callable $readerCustomizer - * @param null|callable $writerCustomizer * * @return Spreadsheet */ - protected function writeAndReload(Spreadsheet $spreadsheet, $format, callable $readerCustomizer = null, callable $writerCustomizer = null) + protected function writeAndReload(Spreadsheet $spreadsheet, $format, callable $readerCustomizer = null) { $filename = tempnam(File::sysGetTempDir(), 'phpspreadsheet-test'); $writer = IOFactory::createWriter($spreadsheet, $format); - if ($writerCustomizer) { - $writerCustomizer($writer); - } $writer->save($filename); $reader = IOFactory::createReader($format); diff --git a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php index 84bedcb422..3870971644 100644 --- a/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php +++ b/tests/PhpSpreadsheetTests/Functional/FreezePaneTest.php @@ -38,27 +38,13 @@ public function testFreezePane($format) self::assertSame($topLeftCell, $actualTopLeftCell, 'should be able to set the top left cell'); } - public function providerFormatsAndActiveFlag() - { - return [ - ['Xls', true], - ['Xls', null], - ['Xls', false], - ['Xlsx', true], - ['Xlsx', null], - ['Xlsx', false], - ]; - } - /** - * @dataProvider providerFormatsAndActiveFlag + * @dataProvider providerFormats * * @param string $format - * @param mixed $actv */ - public function testFreezePaneWithInvalidSelectedCells($format, $actv) + public function testFreezePaneWithInvalidSelectedCells($format) { - $callback = $this->setCallback($format, $actv); $cellSplit = 'A7'; $topLeftCell = 'A24'; @@ -68,7 +54,7 @@ public function testFreezePaneWithInvalidSelectedCells($format, $actv) $worksheet->freezePane('A7', 'A24'); $worksheet->setSelectedCells('F5'); - $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); @@ -77,44 +63,16 @@ public function testFreezePaneWithInvalidSelectedCells($format, $actv) self::assertSame($cellSplit, $actualCellSplit, 'should be able to set freeze pane'); self::assertSame($topLeftCell, $actualTopLeftCell, 'should be able to set the top left cell'); - $expected = ($format === 'Xls' || $actv === true) ? 'F5' : 'A24'; - self::assertSame($expected, $reloadedActive->getSelectedCells()); - } - - public function setActiveTrue($writer) - { - $writer->setActiveCellAnywhere(true); - } - - public function setActiveFalse($writer) - { - $writer->setActiveCellAnywhere(false); - } - - public function setCallback($format, $actv) - { - if ($format === 'Xls') { - return null; - } - if ($actv === true) { - return [$this, 'setActiveTrue']; - } - if ($actv === false) { - return [$this, 'setActiveFalse']; - } - - return null; + self::assertSame('F5', $reloadedActive->getSelectedCells()); } /** - * @dataProvider providerFormatsAndActiveFlag + * @dataProvider providerFormats * * @param string $format - * @param mixed $actv */ - public function testFreezePaneUserSelectedCell($format, $actv) + public function testFreezePaneUserSelectedCell($format) { - $callback = $this->setCallback($format, $actv); $spreadsheet = new Spreadsheet(); $worksheet = $spreadsheet->getActiveSheet(); $worksheet->setCellValue('A1', 'Header1'); @@ -129,23 +87,21 @@ public function testFreezePaneUserSelectedCell($format, $actv) $worksheet->freezePane('A2'); $worksheet->setSelectedCells('C3'); - $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); - $expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; + $expected = 'C3'; self::assertSame($expected, $reloadedActive->getSelectedCells()); } /** - * @dataProvider providerFormatsAndActiveFlag + * @dataProvider providerFormats * * @param string $format - * @param mixed $actv */ - public function testNoFreezePaneUserSelectedCell($format, $actv) + public function testNoFreezePaneUserSelectedCell($format) { - $callback = $this->setCallback($format, $actv); $spreadsheet = new Spreadsheet(); $worksheet = $spreadsheet->getActiveSheet(); $worksheet->setCellValue('A1', 'Header1'); @@ -160,11 +116,10 @@ public function testNoFreezePaneUserSelectedCell($format, $actv) //$worksheet->freezePane('A2'); $worksheet->setSelectedCells('C3'); - $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format, null, $callback); + $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format); // Read written file $reloadedActive = $reloadedSpreadsheet->getActiveSheet(); - //$expected = ($format === 'Xls' || $actv === true) ? 'C3' : 'A2'; $expected = 'C3'; self::assertSame($expected, $reloadedActive->getSelectedCells()); } From 0b0757cbfd4fa0e1f3a3e3ed787a9c8317b3b13c Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sun, 26 Jan 2020 23:15:40 -0800 Subject: [PATCH 4/4] Merge with PR 1362 Bugfix 1161 Travis CI reported problem with Calculation. That was changed in master 3 days ago (delete some unused code). Perhaps the lack of that change is the problem here. --- src/PhpSpreadsheet/Calculation/Calculation.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 1e299f47c7..cc8a8bd864 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -4159,13 +4159,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) if ($pCellParent) { $pCell->attach($pCellParent); } - if (($cellID == 'AC99') || (isset($pCell) && $pCell->getCoordinate() == 'AC99')) { - if (defined('RESOLVING')) { - define('RESOLVING2', true); - } else { - define('RESOLVING', true); - } - } $functionName = $matches[1]; $argCount = $stack->pop();