Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active Cell Anywhere #1323

Closed
wants to merge 4 commits into from
Closed

Active Cell Anywhere #1323

wants to merge 4 commits into from

Conversation

oleibman
Copy link
Collaborator

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.

This is:

- [ ] a bugfix
- [x] a new feature

Checklist:

Why this change is needed?

As described above, add the ability to open a worksheet anywhere even when freeze pane is used.

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.
Apply Scrutinizer recommendations.
Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. As you mentioned yourself it would be best to have xls and xlsx writers kept in sync as much as possible. As such is best to modify existing (incorrect) behavior, rather than add a new method.

Could you please edit your PR in that way ? That would probably make it much simpler and smaller.

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.
@oleibman oleibman requested a review from PowerKiKi January 27, 2020 06:52
@oleibman
Copy link
Collaborator Author

Change simplified per request.
The Travis CI build failed, but I do not know why. There is no explanation.
Examining the job log, I see the following message around line 10107:
Version master
Parsing ############### 31% 1 error
PhpOffice\PhpSpreadsheet\Calculation\Calculation
However, (a) it does not say what the error is (it did not stop all the other tests from passing),
and (b) this change does not touch PhpOffice\PhpSpreadsheet\Calculation\Calculation.
So, if you can let me know what needs to be done here, I can try to take care of it,
but, if not ...

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.
@oleibman
Copy link
Collaborator Author

I noticed that Calculation had been changed 3 days ago. Manually applying that simple change to my pull request eliminated the Travis CI problem. I don't know if that was the best way to solve the problem or not, but it worked.

@PowerKiKi PowerKiKi closed this in fb37938 Mar 2, 2020
@PowerKiKi
Copy link
Member

Thanks, merged in fb37938

paulkned pushed a commit to paulkned/PhpSpreadsheet that referenced this pull request Mar 6, 2020
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.

Closes PHPOffice#1323
@oleibman oleibman deleted the activecell branch April 28, 2020 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants