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

Default Style Alignment #3924

Merged
merged 4 commits into from Mar 9, 2024
Merged

Default Style Alignment #3924

merged 4 commits into from Mar 9, 2024

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Feb 29, 2024

Fix #3918, sort of. Xlsx cells use the default style when they omit the s tag, or when s="0" is specified. LibreOffice does not honor Alignment in the default Style unless the cell explicitly uses the second form, even though it honors other default Styles (e.g. bold font) when s is omitted. Gnumeric seems to have the same problem. A bug report has been filed with LibreOffice.

In the meantime, this PR adds to Xlsx Writer an optional boolean property explicitStyle0 with setter and getter. Default is false, which will continue the current behavior by adding an s tag only when the cell uses a non-default style (this is how Excel itself behaves). When set to true, Xlsx Writer will explicity write s="0" for all cells using default style. This will allow users to create an Xlsx spreadsheet with default alignment of cells that will show up correctly when the spreadsheet is viewed with LibreOffice. Technically speaking, it is probably safe to always use true, except that the spreadsheet size will be a bit larger. However, my hope is that this is a temporary measure which can go away when the vendors have had a chance to fix their problems, hence the false default.

This is:

  • a bugfix (workaround for vendor problem)
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

Fix PHPOffice#3918, sort of. Xlsx cells use the default style when they omit the `s` tag, or when `s="0"` is specified. LibreOffice does not honor Alignment in the default Style unless the cell explicitly uses the second form, even though it honors other default Styles (e.g. bold font) when `s` is omitted. Gnumeric seems to have the same problem. A bug report has been filed with LibreOffice.

In the meantime, this PR adds to Xlsx Writer an optional boolean property `explicitStyle0` with setter and getter. Default is false, which will continue the current behavior by adding an `s` tag only when the cell uses a non-default style (this is how Excel itself behaves). When set to true, Xlsx Writer will explicity write `s="0"` for all cells using default style. This will allow users to create an Xlsx spreadsheet with default alignment of cells that will show up correctly when the spreadsheet is viewed with LibreOffice. Technically speaking, it is *probably* safe to always use `true`, except that the spreadsheet size will be a bit larger. However, my hope is that this is a temporary measure which can go away when the vendors have had a chance to fix their problems, hence the `false` default.
@oleibman
Copy link
Collaborator Author

A win for Scrutinizer! I had miscoded some tests, which resulted in a Scrutinizer warning, and which should have resulted in a test failure but didn't. We have always cautioned that it is dangerous to store a Cell in a variable. It appears that the same is true for Style. The problem shows up in the following code adapted from my test:

        $styleA1 = $sheet2->getStyle('A1');
        var_dump($styleA1->getFont()->getItalic());  // false, because A1 is not italic
        $styleA2 = $sheet2->getCell('A2')->getStyle();
        var_dump($styleA1->getFont()->getItalic()); // true, because A2 is talic

I will correct the test, and attempt to update the documentation.

It pointed out a problem that I would have thought should result in test failure. Code corrected, and documentation updated to describe the unexpected behavior.
@oleibman
Copy link
Collaborator Author

No concerns with Scrutinizer "complexity" complaint.

@oleibman oleibman added this pull request to the merge queue Mar 9, 2024
Merged via the queue into PHPOffice:master with commit 9a94aea Mar 9, 2024
13 of 14 checks passed
@oleibman oleibman deleted the issue3918 branch March 9, 2024 05:16
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.

Default style alignment does not work for Xlsx Writer
1 participant