Issue 119989

Created attachment 78322 [details]
snapshot

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.

Created attachment 78327 [details]
patch for this issue

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?

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?

@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)".

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?

Created attachment 78412 [details]
how to adjust 3D height

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?

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...

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.

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.

Created attachment 78431 [details]
Created with OOo2.4.3 by chart2 module

ALG: Regaina, good Idea, thanks for the examples!

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

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.

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!

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.

Hi Armin, thanks a lot for your modification! I have checked with your recommendation just before and I see your adapted patch is ok!

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!

Armin, I have checked it works with sample files from Yan Ji and Reginas, also checked creating chart inside SC. Thanks!

ALG: Thanks Tan Li for checking. I repeated some checks, too. Comitted as r1354914, thanks for the patch!

Verified on r1374181. It works.

set Target Milestone to AOO 3.5.0 for PM purpose.