Additional Support for Chart DataSeriesValues

[oleibman]

Fix #2863. DataSeriesValues now extends Properties, allowing it to share code in common with Axis and Gridlines. This causes some minor breakages; in particular line width is now initialized to null instead of Excel's default value, and is specified in points, as the user would expect from Excel, rather than the value stored in Xml.

This change:

• adds support for 1 or 2 marker colors.
• adds support for smoothLine to DataSeriesValues.
• will determine catAx or valAx for Axis based on what is read from the Xml when available, rather than guessing based on format. (Another minor break.)
• reads formatCode and sourceLinked for Axis.
• correct 2 uses of $plotSeriesRef to $plotSeriesIndex in Writer/Xlsx/Chart.
• pushes coverage over 90% for Chart (88.70% overall).
• Incorporates PR Allow setting fill colour for bar chart data points #2724 from @DanWin.

This is:

- [x] a bugfix
- [ ] 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
• 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.

[bridgeplayr]

All your changes are very similar to my work to address these issues. I admit your implementation is better.

Adding Class ChartColor is good. That makes the various object's colors much easier to deal with.

I have some suggestions:

Property smoothLine (and its getter/setter methods) should be removed from DataSeries. They belong only in DataSeriesValues.

TYPOS:
I am embarrassed to point out "Properties::LINE_STYLE_DASH_SQUERE_DOT", but since I have used it in a chart, it should be renamed ---------------"LINE_STYLE_DASH_SQUARE_DOT".

You found and corrected almost all of the misnamed parameters "$AlphaType", but it escaped your correction in Axis. I renamed it "$ColorType", but it appears that you prefer "$colorType". Your call.

(Someday - I suggest adding another Class - ChartLine - as an extension of Properties, which can accommodate all of the classes Axis, Gridlines, Scatterlines (as well as the yet-to-be-added Trendline), all of which have very similar line properties (linewidth and color), as well as special effects: LineStyle, Shadow, Glow, and SoftEdges. The implications of implementing it in Writer/Xlsx are messy, of course.)

[bridgeplayr]

One more suggestion:
Inside an Excel chart, marker colors are listed as "Fill color" and "Border Color". Might it be better to rename markerColor1 as markerFillColor, and rename
markerColor2 as markerBorderColor?

And rename their respective getter/setter methods:
getMarkerFillColor(): ChartColor
return $this->markerFillColor;
getMarkerBorderColor(): ChartColor
return $this->markerBorderColor;

Further:
Now that markers have fill & border colors defined by class ChartColor, what is the purpose of the 7th parameter ($fillColor) in DataSeriesValues' constructor?

In Writer/Xlsx function writePlotGroup(), in the loop for $plotSeriesIdx => $plotSeriesRef, inside qualifiers "if ($plotSeriesValues) {
....
if ($plotSeriesMarker !== 'none') {
...
this superfluous statement appears: $fillColor = $plotSeriesValues->getFillColor();
The temporary variable $fillColor is not used. It is the 7th parameter passed to DataSeriesValues' constructor.
This leads to an important missing section of code between
// Labels
$plotSeriesLabel = ...
....
and
....
// Formatting for the points [markers]

Between those sections of xml generation must be the description of the line connecting the markers (or points). And, because it is a 'line', it can have color, linewidth, Styling, Shadow, Glow, SoftEdges - the whole barnyard of special effects. That is where all that stuff must be added. No fun, but I've taken a stab at it. More later.

[oleibman]

@bridgeplayr, thank you for the original suggestions, and for your thoughtful review of the change. There must be a way to insert replies in the correct place in the original message in Github, but I haven't figured it out yet. So I'll just put all my replies here.

• removing smoothLine from DataSeries falls in the "breaking change" category, and I hope to get to those eventually (this isn't the first potential break that I've come across during these PRs), but, for now, it will stay.
• I will spell Square correctly and add a deprecation notice for Squere. I will probably add a new commit to this PR over the weekend.
• our properties are almost always camel case starting with lowercase, so colorType will remain.
• $AlphaType should just be $type. Unfortunately, with Php8, changing parameter names is a breaking change, because you can now specify keyword parameters. So, see earlier comment about breaking changes.
• I considered adding ChartLine as its own class, like ChartColor. I found that I didn't need it for this change; there may be a time when I change my mind. ChartColor was especially needed because it is used by non-Chart classes, in particular Font. Right now, everything that needs Line inherits it from Properties.
• I will rename the properties to markerFillColor and markerBorderColor as you suggest. Part of this weekend's commit.
• I agree that some constructor parameters no longer make sense. See earlier comment about breaking changes.
• I have not yet had a chance to analyze your superfluous statement. If your comment makes sense, I will try to incorporate it over the weekend.
• Work on labels is next on my list. It is too big to add to this change.
• This change does deal with the lines between the markers. Otherwise, I would not have been able to handle your sample file.

[oleibman]

I think you are correct that the statement which initializes fillColor is superfluous. However, I don't think we miss any important executable code as a result. Writing of the line styles takes place with the call to writeLineStyles just at the end of the if statement immediately following "Formatting for the points". You are right that the effects aren't written there. In theory, they could be written, and quite simply, if the necessary support is added to Reader. Those changes are too big to add to this one; they are on my to-do list.

[bridgeplayr]

Success. Not the prettiest code you will see. Feel free to clean things up as you see fit.
(I also commented in a different thread - #2863)

33_Chart_create_Color_scatter.php.txt

[bridgeplayr]

***@***.***> @oleibman, thank you for your thoughtful reply. I confess to being glad that I am unencumbered by the many constraints of being a Developer. I understand that it is far easier for me to make suggestions than for you to implement them. Being a new dabbler in server-side code and php, I am gratified that my input is recognized. Thank you for that. I understand all your comments below about the extra effort required for those suggestions and tasks. I’m confident that I can create the sample xlsx using your PR, and I will feedback any modifications I think are necessary. Lastly, I am so happy to gratefully acknowledge the importance of your consolidating all of the line special effects xml generation functions into ‘writeShadow()’, ‘writeGlow()’, ‘writeSoftEdges()’,’writeLineStyles()’. My final self-assigned task was to attempt precisely this job, but the scope of reorganizing writing XAxis, YAxis, Major/MinorGridlines as well as ScatterLines xml was more than I could accomplish. I could hardly believe my eyes when I saw how neatly you did this. I understand the utility of writeNotEmpty() and writeColor() too. Very clever. Well done. Back to the keyboard. Rick Garvin From: oleibman ***@***.***> Sent: Friday, June 24, 2022 3:38 PM To: PHPOffice/PhpSpreadsheet ***@***.***> Cc: bridgeplayr ***@***.***>; Mention ***@***.***> Subject: Re: [PHPOffice/PhpSpreadsheet] Additional Support for Chart DataSeriesValues (PR #2906) @bridgeplayr <https://github.com/bridgeplayr> , thank you for the original suggestions, and for your thoughtful review of the change. There must be a way to insert replies in the correct place in the original message in Github, but I haven't figured it out yet. So I'll just put all my replies here. * removing smoothLine from DataSeries falls in the "breaking change" category, and I hope to get to those eventually (this isn't the first potential break that I've come across during these PRs), but, for now, it will stay. * I will spell Square correctly and add a deprecation notice for Squere. I will probably add a new commit to this PR over the weekend. * our properties are almost always camel case starting with lowercase, so colorType will remain. * $AlphaType should just be $type. Unfortunately, with Php8, changing parameter names is a breaking change, because you can now specify keyword parameters. So, see earlier comment about breaking changes. * I considered adding ChartLine as its own class, like ChartColor. I found that I didn't need it for this change; there may be a time when I change my mind. ChartColor was especially needed because it is used by non-Chart classes, in particular Font. Right now, everything that needs Line inherits it from Properties. * I will rename the properties to markerFillColor and markerBorderColor as you suggest. Part of this weekend's commit. * I agree that some constructor parameters no longer make sense. See earlier comment about breaking changes. * I have not yet had a chance to analyze your superfluous statement. If your comment makes sense, I will try to incorporate it over the weekend. * Work on labels is next on my list. It is too big to add to this change. * This change does deal with the lines between the markers. Otherwise, I would not have been able to handle your sample file. — Reply to this email directly, view it on GitHub <#2906 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYXANOBW5TUU4CRMGQ72R4DVQY2GHANCNFSM5ZSK7EJA> . You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AYXANODWDUTINJHKNBA2FGLVQY2GHA5CNFSM5ZSK7EJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIV723EY.gif> Message ID: ***@***.*** ***@***.***> >

[oleibman]

Thank you for the nice comments. I too have learned a lot from your suggestions (e.g. I had never actually created a chart in Excel before starting to work on them). Don't hesitate to let us know what's still missing.