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

Added more chart supports. #144

Closed
wants to merge 6 commits into from
Closed

Added more chart supports. #144

wants to merge 6 commits into from

Conversation

sandeeptiwari32
Copy link

Added more chart supports.
fixed bug while creating chart with bar and line series.

fixed bug while creating chart with bar and line series.
@asfgit
Copy link

asfgit commented May 4, 2019

Can one of the admins verify this patch?

XDDFBarChartData.Series series1 = (XDDFBarChartData.Series) bar.addSeries(xs, ys1);
series1.setTitle("Bars", new CellReference("Sheet1!$B$1"));
bar.setVaryColors(true);
bar.setBarDirection(BarDirection.COL);
chart.plot(bar);

// the line chart
// the line chart on secondary axis
XDDFLineChartData lines = (XDDFLineChartData) chart.createData(ChartTypes.LINE, lineCategories,
Copy link
Author

Choose a reason for hiding this comment

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

create line series with secondary axis

XDDFLineChartData.Series series2 = (XDDFLineChartData.Series) lines.addSeries(xs, ys2);
series2.updateIdXVal(1);
Copy link
Author

Choose a reason for hiding this comment

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

update axis value and order value of series.

@@ -17,9 +17,16 @@ Licensed to the Apache Software Foundation (ASF) under one or more
package org.apache.poi.xddf.usermodel.chart;

public enum ChartTypes {
AREA,
Copy link
Author

Choose a reason for hiding this comment

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

enum for new chart types

@@ -0,0 +1,165 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for area 3d chart

@@ -0,0 +1,168 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for area chart

@@ -0,0 +1,201 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for bar 3d chart

@@ -187,5 +187,16 @@ protected CTAxDataSource getAxDS() {
protected CTNumDataSource getNumDS() {
return series.getVal();
}

Copy link
Author

Choose a reason for hiding this comment

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

overrided method to update series id and order id

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see these methods, but your comment does not help to understand what problem they were supposed to solve in the first place.

I first of all question their public visibility. Since I don't think it is good practice to rely on the developer using the library to know when to use and what to expect from them...

Copy link
Author

Choose a reason for hiding this comment

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

so we created update methods, because in a chart, we need to provide unique value to each series, now while creating combo chart, i.e. line series and bar series in that case we assigning same values to each series due to that it was corrupting the office file. Now either we can create a new constructor with new parameter i.e. index or can provide an update method to re-assign axis id and order id. so i chose update method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I finally understood what you were trying to achieve! I had a similar code in a PR that I left aside because of some more changes required after a comment about API requiring changes.

https://github.com/apache/poi/pull/139/files#diff-17a515ee7210ab25ee2bcedbf5e49a87R116

Copy link
Author

Choose a reason for hiding this comment

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

I also tried to get all series list in chart data, but as there is no relationship between different series types, so we are left with only series list of particular chart data.

*
* @return this method will add 3D view
*/
public XDDFView3D getOrAddView3D()
Copy link
Author

Choose a reason for hiding this comment

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

method to add 3d view into chart

Map<Long, XDDFChartAxis> categories = null;
Map<Long, XDDFValueAxis> mapValues = null;

if(ChartTypes.PIE != type && ChartTypes.PIE3D != type)
Copy link
Author

Choose a reason for hiding this comment

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

handled null pointer exception in case of pie chart.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

Copy link
Author

Choose a reason for hiding this comment

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

thank you :)

categories = Collections.singletonMap(category.getId(), category);
mapValues = Collections.singletonMap(values.getId(), values);
}

final CTPlotArea plotArea = getCTPlotArea();
switch (type) {
Copy link
Author

Choose a reason for hiding this comment

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

added case to create instance for new added chart

@@ -98,6 +98,10 @@ public XDDFCategoryAxis getCategoryAxis() {

protected abstract CTNumDataSource getNumDS();

protected abstract void updateIdXVal(long val);
Copy link
Author

Choose a reason for hiding this comment

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

abstract methods to update series id and order id

@@ -0,0 +1,233 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for Line 3d chart

@@ -67,7 +67,11 @@ public Grouping getGrouping() {
}

public void setGrouping(Grouping grouping) {
if (chart.getGrouping()!=null) {
Copy link
Author

Choose a reason for hiding this comment

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

handled case in case of grouping was not created in chart

@@ -0,0 +1,157 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for pie 3d chart

@@ -0,0 +1,168 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for surface 3d chart

@@ -0,0 +1,167 @@
/* ====================================================================
Copy link
Author

Choose a reason for hiding this comment

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

support for surface chart

@@ -0,0 +1,106 @@
/*
Copy link
Author

Choose a reason for hiding this comment

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

created class to use 3d view

Copy link
Author

@sandeeptiwari32 sandeeptiwari32 left a comment

Choose a reason for hiding this comment

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

please review.

Copy link
Contributor

@Alain-Bearez Alain-Bearez left a comment

Choose a reason for hiding this comment

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

After formatting details are fixed, we will start discussing some more in depth design decisions.

@@ -134,17 +134,26 @@ public static void main(String[] args) throws Exception {
lineCategories.crossAxis(rightValues);

// the bar chart
XDDFBarChartData bar = (XDDFBarChartData) chart.createData(ChartTypes.BAR, lineCategories, rightValues);
XDDFBarChartData bar = (XDDFBarChartData) chart.createData(ChartTypes.BAR, barCategories, leftValues);
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch!

Copy link
Author

Choose a reason for hiding this comment

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

thank you :)

XDDFLineChartData.Series series2 = (XDDFLineChartData.Series) lines.addSeries(xs, ys2);
series2.updateIdXVal(1);
series2.updateOrderVal(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Your updateXXXVal() methods are disguised setters. I am not quite sure this is the best way to achieve your goal (which I still does not understand at this point).

Copy link
Author

@sandeeptiwari32 sandeeptiwari32 May 7, 2019

Choose a reason for hiding this comment

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

so we created update methods, because in a chart, we need to provide unique value to each series, now while creating combo chart, i.e. line series and bar series in that case we assigning same values to each series due to that it was corrupting the office file. Now either we can create a new constructor with new parameter i.e. index or can provide an update method to re-assign axis id and order id. so i chose update method.

SCATTER
SCATTER,
SURFACE,
SURFACE3D
Copy link
Contributor

Choose a reason for hiding this comment

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

Seven new chart types at once!!!

Very impressive, indeed!

Copy link
Author

Choose a reason for hiding this comment

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

Thank u :)

@@ -187,5 +187,16 @@ protected CTAxDataSource getAxDS() {
protected CTNumDataSource getNumDS() {
return series.getVal();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see these methods, but your comment does not help to understand what problem they were supposed to solve in the first place.

I first of all question their public visibility. Since I don't think it is good practice to rely on the developer using the library to know when to use and what to expect from them...

public XDDFView3D getOrAddView3D()
{
CTView3D view3D;
if(chart.isSetView3D()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing whitespace between if and (

Copy link
Author

Choose a reason for hiding this comment

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

updated code.


@Override
public void setVaryColors(boolean varyColors) {

Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Copy link
Author

Choose a reason for hiding this comment

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

added comment.


@Override
public void setShowLeaderLines(boolean showLeaderLines) {

Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Copy link
Author

Choose a reason for hiding this comment

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

added comment.

view3D.getRotY().setVal(val);
}
else
view3D.addNewRotY().setVal(val);
Copy link
Contributor

Choose a reason for hiding this comment

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

The three lines above don't follow formatting rules for the project...

And the pattern is (wrongly) repeated until the end of this class!

Copy link
Author

Choose a reason for hiding this comment

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

updated code.

}

public int getRotXVal() {
return view3D.getRotX().getVal();
Copy link
Contributor

Choose a reason for hiding this comment

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

Despite being internally represented as an int value, the rotation represents an angle which developers using this library will expect to be expressed in degrees instead of the scaled range allowed by the specs.

We will have to provide a converting mechanism. I am sorry I got it wrong in the first place on the other properties representing angles...

Copy link
Author

Choose a reason for hiding this comment

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

yes we need to write a utility method for conversion.

Copy link
Author

@sandeeptiwari32 sandeeptiwari32 May 7, 2019

Choose a reason for hiding this comment

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

i am writing that utility method. so if it's completed early so i will check in that code with this pull request.

Copy link
Contributor

@Alain-Bearez Alain-Bearez May 12, 2019

Choose a reason for hiding this comment

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

While reading the specs, this is one of the few places where the angles are given as integer values, without scaling by 60,000 to get more precision.

return view3D.getHPercent().getVal();
}

public void setHPercentVal(int val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As matter of facts, all the methods names have to be adapted to the conventions used otherwise in the project:

  1. no Val suffix
  2. no unpronounceable name such as RAngAx
  3. no shortcut as RotX, RotY or HPercent but RotationX, RotationY and HeightPercent

Copy link
Author

Choose a reason for hiding this comment

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

updated method names.

Copy link
Author

@sandeeptiwari32 sandeeptiwari32 left a comment

Choose a reason for hiding this comment

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

updated suggested changes.

@Alain-Bearez
Copy link
Contributor

applied as r1859590

@asfgit asfgit closed this in 5e070fd May 21, 2019
asfgit pushed a commit that referenced this pull request Oct 6, 2019
Alain-Bearez pushed a commit to cuali/poi that referenced this pull request Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants