row height

[jonnott]

I'm trying to add support for setting row height, but I don't know how to do it.

I've re-added getter/setter and a property which were removed in a recent commit due to being unused. I've also added a failing test.

What I don't know is what the syntax in the raw XML output (XLS or ODS) would be for specifying the height of the row, or where this code would need to be.

The code would check for strlen() of the height property, and then apply the height to the row if height has been set via the setter.

I recently did a PR for celllVerticalAlignment and explicit setWrapText to false: https://github.com/openspout/openspout/pull/63

I'm trying to replicate doing something like this equivalent in XLSXWriter:

if ($lines > 1) { $row_options['height'] = $lines * 12.1; }
$writer->writeSheetRow('Sheet1', array_values($row), $row_options);

Any help or pointers would be much appreciated. I'd be happy to fully finish this PR and tests if only I knew how!

[codecov[bot]]

Codecov Report

Base: 99.45% // Head: 99.45% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (602873b) compared to base (e0a70a1). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##                4.x      #77   +/-   ##
=========================================
  Coverage     99.45%   99.45%           
- Complexity     1078     1086    +8     
=========================================
  Files            95       95           
  Lines          3091     3105   +14     
=========================================
+ Hits           3074     3088   +14     
  Misses           17       17           

Impacted Files	Coverage Δ
src/Common/Entity/Row.php	100.00% <100.00%> (ø)
src/Writer/XLSX/Manager/WorksheetManager.php	96.38% <100.00%> (+0.04%)	:arrow_up:
src/Writer/ODS/Manager/WorksheetManager.php	98.18% <0.00%> (-0.04%)	:arrow_down:
src/Reader/ODS/SheetIterator.php	100.00% <0.00%> (ø)
src/Reader/Wrapper/XMLReader.php	100.00% <0.00%> (ø)
src/Common/Helper/Escaper/XLSX.php	100.00% <0.00%> (ø)
src/Writer/Common/Helper/CellHelper.php	100.00% <0.00%> (ø)
.../Writer/Common/Manager/AbstractWorkbookManager.php	100.00% <0.00%> (ø)
src/Writer/XLSX/Manager/Style/StyleManager.php	99.18% <0.00%> (+0.02%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[jonnott]

Implementing the feature is the easiest part, we need the test first.

I've updated with a working implementation and passing test for XLSX. I have no idea how to do the equivalent for ODS.

This is the diff between my test XLSX files:

..and the diff between my test ODS files:

Anyone able to help with the ODS implementation?

[jonnott]

@Slamdunk

[faytekin]

@Slamdunk

[Slamdunk]

@faytekin may you help @jonnott with the ODS implementation?

[delameter]

Fixed broken pipeline in 7f8ff57a90b7453135138e4a595df040c69424ed (made a PR to original branch in author's repo).

Also researched a bit about ODS implementation, and it's a lot harder than it seems to be. I'll cut to the chase, in ODS "height" is the attribute of style, not the attribute of row. I attempted to insert elements describing row style <style:table-row-properties style:row-height="$$$pt" style:use-optimal-row-height="false"/> into table section before row definition, but they were ignored by Libre (small wonder). Libre itself saves row height like this:

But AFAIK writing row styles is not implemented in current version (only cell styles). It's hard to say how long will it take for someone to fully implement this, if it's even possible considering write-only approach.

At the same time, setting the height of XLSX rows seems to work (the only thing that I can't comprehend is why @jonnott used string variable to store the height and not float), so how about separating these two features and releasing customizable heights for XLSX only?

[jonnott]

(the only thing that I can't comprehend is why @jonnott used string variable to store the height and not float)

@delameter Feel free to change this if you feel float would be better.

[jonnott]

(the only thing that I can't comprehend is why @jonnott used string variable to store the height and not float)

@delameter Feel free to change this if you feel float would be better.

I think it was just because the pre-existing setHeight() and getHeight() (which I reinstated after they had been removed) used string for this property.

[delameter]

I also made Dockerfile and docker-compose.yml for fast tests launching and debugging without fiddling with dependencies, but don't know will it fit in or not. If it's acceptable I can create a separate PR for adding Docker support.

[delameter]

(the only thing that I can't comprehend is why @jonnott used string variable to store the height and not float)

@delameter Feel free to change this if you feel float would be better.

I think it was just because the pre-existing setHeight() and getHeight() (which I reinstated after they had been removed) used string for this property.

Well I can assume that it was initially made for possibility to set the height with units, like "15pt" or "2cm", but then there should be at least some validation mechanism (even if rudimentary) to ensure that internal file markup will not be broken.

[jonnott]

Well I can assume that it was initially made for possibility to set the height with units, like "15pt" or "2cm",

Yes, I think that's why.

[jonnott]

@Slamdunk Any ideas who might be able to help get this one over the line?

[jonnott]

Committed those changes @Slamdunk

[jonnott]

Fixed test also

[jonnott]

@Slamdunk I have no idea why the Static Analysis says "Error: Cannot call method getAttribute() on DOMNode|false.". I therefore cannot implement a solution. I can't see how it's got anything to do with this PR specifically.