Issue 119989 - [From Symphony]Pie chart height becomes greater when open Excel file
[From Symphony]Pie chart height becomes greater when open Excel file
Status: CLOSED FIXED
Product: General
Classification: Code
Component: chart
OOo 3.3 or older
All All
: P3 normal (vote)
: 4.0.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-14 08:03 UTC by Yan Ji
Modified: 2013-07-11 13:34 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
sample (10.50 KB, application/octet-stream)
2012-06-14 08:03 UTC, Yan Ji
no flags Details
snapshot (55.96 KB, image/png)
2012-06-14 08:04 UTC, Yan Ji
no flags Details
patch for this issue (9.52 KB, patch)
2012-06-14 12:53 UTC, Tan Li
litanbj: review?
Details | Diff
how to adjust 3D height (172.64 KB, image/pjpeg)
2012-06-21 05:15 UTC, Tan Li
no flags Details
created with OOo1.1.5 by old chart module (35.44 KB, application/vnd.sun.xml.calc)
2012-06-21 18:43 UTC, Regina Henschel
no flags Details
Created with OOo2.4.3 by chart2 module (35.25 KB, application/vnd.sun.xml.calc)
2012-06-21 18:46 UTC, Regina Henschel
no flags Details
Adapted patch (7.99 KB, patch)
2012-06-27 15:52 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Yan Ji 2012-06-14 08:03:48 UTC
Created attachment 78321 [details]
sample

build: AOO3.4
Open sample file and compare with MS Excel

Defect: Pie chart height in AOO is higher than MS Excel
Comment 1 Yan Ji 2012-06-14 08:04:15 UTC
Created attachment 78322 [details]
snapshot
Comment 2 Tan Li 2012-06-14 12:47:16 UTC
I have investigated this issue, and my opinion is
Root Cause:
 When import .xls files, AOO3.4 don't read 3D height of Piechart in files and let it show a fixed value of height.

Solution:
Set the piechart 3D height according to XclImpChChart3d record in imported  xls file. Correspondingly, add 3D height property names in xlchart.hxx and set it to ScfPropertySet in xichart.cxx. When create 3D piechart, we will get its 3D height from model and set height fDepth PieChart::createShapes(). In chart model, we also add default value of 3D height property as usual.
Comment 3 Tan Li 2012-06-14 12:53:00 UTC
Created attachment 78327 [details]
patch for this issue
Comment 4 Armin Le Grand 2012-06-20 15:34:47 UTC
ALG: Taking a look (applying, building). Just one question: May it be that modules outside sc may load excel charts and may set this properties, too? May a excel-generated chart be embedded in PPT, and will/could the property be read/set?
Comment 5 Armin Le Grand 2012-06-20 15:39:56 UTC
ALG: Looks good, does what is expected.
Another question: To test more deep, how can I change the height of the chart in Excel (I'm a excel beginner) to play with the feature a bit?
Comment 6 Regina Henschel 2012-06-20 17:18:46 UTC
@ALG: To change the height: Use the context menu of the data series or "Diagrammbereich formatieren" of the chart (Please excuse German terms, I have only German Excel). Therein the item "3D-Drehung" and in that dialog in the lower part "Diagrammskalierung" the option "Höhe (% der Basis)".
Comment 7 Tan Li 2012-06-21 05:14:05 UTC
Regina Henschel answers the steps to adjust 3D piechart height, thanks, and I'd like attach a picture to show this.
(In reply to comment #5)
> ALG: Looks good, does what is expected.
Another question: To test more deep,
> how can I change the height of the chart in Excel (I'm a excel beginner) to
> play with the feature a bit?
Comment 8 Tan Li 2012-06-21 05:15:25 UTC
Created attachment 78412 [details]
how to adjust 3D height
Comment 9 Tan Li 2012-06-21 08:30:09 UTC
I'd like to answer with following experiment results,
  1, modules outside sc load excel charts:
     Insert an Excel file which contain 3D PieChart to Impress, and this will call Excel charts filter process including reading/setting 3D height properties as patch for this issue; 
     Copy 3D PieChart from Excel and paste as OLE, and it will copy and show a picture at first. If double click it, OS will call Excel to open the chart, in this way, AOO will not read/set 3D height properties; if don't have Excel installed, double click will call AOO to open the chart and will read/set 3D height properties as patch for this issue.
  2, Excel-generated chart embedded in PPT:
    Insert an Excel file which contain 3D PieChart or copy Excel 3D PieChart to PPT, open this PPT with Impress, and it will load and show a picture at first, then double click will call Excel to open the chart, AOO will not read/set 3D height properties; if don't have Excel installed, AOO will read/set 3D height properties as patch for this issue.
   In summary, Excel 3D PieChart height in above cases will work well.     


(In reply to comment #4)
> ALG: Taking a look (applying, building). Just one question: May it be that
> modules outside sc may load excel charts and may set this properties, too?
> May a excel-generated chart be embedded in PPT, and will/could the property
> be read/set?
Comment 10 Armin Le Grand 2012-06-21 09:01:38 UTC
ALG: Thanks to Regina for the explanation. Thanks to Tan Li for the picture (says more than words). I have found it now :-)

Thanks Tan Li also for checking the possible combinations of embedded charts, I can not think about others and see these as complete.

Taking a 2nd look, testing changed charts and looking at the code...
Comment 11 Armin Le Grand 2012-06-21 09:24:05 UTC
ALG: Sorry, I have another question: Is the property "IsExcel3DChart" needed?

The value can be converted from percent to the absolute value in sc/source/filter/excel/xichart.cxx line 2405, 

    rPropSet.SetProperty( EXC_CHPROP_3DHEIGHT, (sal_Int32)maData.mnRelHeight);

before setting it. Then, chart2/source/view/charttypes/PieChart.cxx line 425

            double fDepth  = bIsExcel3DChart ?  (this->getTransformedDepth()) * n3DHeight /(2*100) : this->getTransformedDepth();

would not need to react on the bool IsExcel3DChart and the standard line

           // double fDepth  = this->getTransformedDepth();    

could be used unchanged.

The API value "3DHeight" would then not have two different units (absolute and relative if excel), and thus two different meanings, but just a single, explicit one. This allows to work with this value without always to have to check "IsExcel3DChart", too.

Also a minimal API is always better. So, is the bool "IsExcel3DChart" needed...? Maybe I have overseen something, please check.
Comment 12 Regina Henschel 2012-06-21 18:43:58 UTC
Created attachment 78430 [details]
created with OOo1.1.5 by old chart module

When changing something for charts, you should also test, whether old charts look still fine. I'll attach two files both sxc-format, including screenshots. One document was created with OOo1.1.5, which has the old chart module, the second document was created with OOo2.4.3, which has already the chart2 module.
Comment 13 Regina Henschel 2012-06-21 18:46:50 UTC
Created attachment 78431 [details]
Created with OOo2.4.3 by chart2 module
Comment 14 Armin Le Grand 2012-06-22 13:24:01 UTC
ALG: Regaina, good Idea, thanks for the examples!
Comment 15 Tan Li 2012-06-23 04:44:44 UTC
Regina, thanks again for your attachments. I have checked them with my patch and find no shape changes from released AOO3.4.(In reply to comment #13)
> Created attachment 78431 [details]
Created with OOo2.4.3 by chart2 module
Comment 16 Tan Li 2012-06-23 05:18:06 UTC
Armin, thanks for your question! We are on traditional holiday and sorry for late answer! I have debug code "fDepth  = this->getTransformedDepth();", and the function getTransformedDepth is
double VSeriesPlotter::getTransformedDepth() const
{
    double MinZ = m_pMainPosHelper->getLogicMinZ();
    double MaxZ = m_pMainPosHelper->getLogicMaxZ();
    m_pMainPosHelper->doLogicScaling( 0, 0, &MinZ );
    m_pMainPosHelper->doLogicScaling( 0, 0, &MaxZ );
    return FIXED_SIZE_FOR_3D_CHART_VOLUME/(MaxZ-MinZ);
}    

FIXED_SIZE_FOR_3D_CHART_VOLUME/(MaxZ-MinZ) always is fixed value, so I have no idea how to change return value of getTransformedDepth() according to Excel 3D height. In my patch, bIsExcel3DChart is used to differentiate between Excel 3D PieChart and AOO 3D Piechart, because AOO 3D Piechart height is fixed value which is return value of getTransformedDepth(); while Excel 3D PieChart will adjust according to  n3DHeight of Excel record, so we use it as coefficient to multiply with return value of getTransformedDepth(). If don't use "IsExcel3DChart", I found Excel 3D PieChart height will behave the same as AOO and lose its difference.

(In reply to comment #11)
> ALG: Sorry, I have another question: Is the property "IsExcel3DChart"
> needed?

The value can be converted from percent to the absolute value in
> sc/source/filter/excel/xichart.cxx line 2405, 

    rPropSet.SetProperty(
> EXC_CHPROP_3DHEIGHT, (sal_Int32)maData.mnRelHeight);

before setting it.
> Then, chart2/source/view/charttypes/PieChart.cxx line 425

           
> double fDepth  = bIsExcel3DChart ?  (this->getTransformedDepth()) *
> n3DHeight /(2*100) : this->getTransformedDepth();

would not need to react
> on the bool IsExcel3DChart and the standard line

           // double
> fDepth  = this->getTransformedDepth();    

could be used unchanged.

The
> API value "3DHeight" would then not have two different units (absolute and
> relative if excel), and thus two different meanings, but just a single,
> explicit one. This allows to work with this value without always to have to
> check "IsExcel3DChart", too.

Also a minimal API is always better. So, is
> the bool "IsExcel3DChart" needed...? Maybe I have overseen something, please
> check.
Comment 17 Armin Le Grand 2012-06-25 11:33:17 UTC
ALG: Hi Tan Li, thanks for the answer. I hope I have not interrupted your traditional holiday; holiday is holiday, no need to break it :-)

I see your point, but still, is "IsExcel3DChart" needed?

- "3DHeight" is a new API value, it does not exist before this proposed change
- If it did not exist, it is not used yet, so if defining there is free choice how to define it
- If true == IsExcel3DChart, 3DHeight is a relative value, applied to getTransformedDepth in the implementation
- if false == IsExcel3DChart, getTransformedDepth is used directly, a evtl. set value of "3DHeight" is ignored. Noone is setting "3DHeight" up to today.

Why not:
- define property "3DRelativeHeight" instead of "3DHeight", default 100%, "Defines the Height of a pie segment relative to it's pedefined height"
- No need to define "IsExcel3DChart" at all
- In all cases, use getTransformedDepth and relative height value
- With default, old behaviour will use 100% of getTransformedDepth, no change
- With value set, new behaviour will work as needed

Thus, "IsExcel3DChart" is not needed and the API stays minimal, only one more value needs to be added. The value in question (3DHeight) is a relative definition anyways, why not call it so and use/add it as such?

Thanks in advance!
Comment 18 Armin Le Grand 2012-06-27 15:52:34 UTC
Created attachment 78502 [details]
Adapted patch

ALG: Adapted patch without the need for a boolean parameter to know that it's an excel chart. Please have a look.
Comment 19 Tan Li 2012-06-28 05:24:08 UTC
Hi Armin, thanks a lot for your modification! I have checked with your recommendation just before and I see your adapted patch is ok!
Comment 20 Armin Le Grand 2012-06-28 09:06:37 UTC
ALG->TanLi: Please, could you tell if you checked if it works, also with Reginas test files? I did some checks, but just to make sure. Thanks!
Comment 21 Tan Li 2012-06-28 09:34:57 UTC
Armin, I have checked it works with sample files from Yan Ji and Reginas, also checked creating chart inside SC. Thanks!
Comment 22 Armin Le Grand 2012-06-28 11:02:13 UTC
ALG: Thanks Tan Li for checking. I repeated some checks, too. Comitted as r1354914, thanks for the patch!
Comment 23 Shan Zhu 2012-08-20 05:45:40 UTC
Verified on r1374181. It works.
Comment 24 Shenfeng Liu 2012-11-07 08:37:43 UTC
set Target Milestone to AOO 3.5.0 for PM purpose.