Bug 115182 - FILEOPEN XLSX Pie chart default percentage format does not match Excels
Summary: FILEOPEN XLSX Pie chart default percentage format does not match Excels
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Chart (show other bugs)
Version:
(earliest affected)
5.1.0.3 release
Hardware: All All
: medium normal
Assignee: Eike Rathke
URL:
Whiteboard: target:6.2.0 target:6.1.3
Keywords: bibisected, bisected, filter:xlsx, regression
Depends on:
Blocks: OOXML-Chart
  Show dependency treegraph
 
Reported: 2018-01-23 22:52 UTC by Gabor Kelemen
Modified: 2018-10-28 04:25 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Example file made with Excel 2013 (20.62 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2018-01-23 22:52 UTC, Gabor Kelemen
Details
Screenshot of the document in LO 5.4 and Excel 2013 (104.05 KB, image/png)
2018-01-23 22:55 UTC, Gabor Kelemen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Kelemen 2018-01-23 22:52:54 UTC
Created attachment 139307 [details]
Example file made with Excel 2013

Attached file has a simple 3D pie chart with values shown as percentages, made with Excel 2013.

In Excel this displays the percentage values rounded to whole numbers, while Calc displays them with two decimal places.
Comment 1 Gabor Kelemen 2018-01-23 22:55:26 UTC
Created attachment 139309 [details]
Screenshot of the document in LO 5.4 and Excel 2013

LO 6.1 master shows the same problem.
Also one may note:
- no shadow in Calc
- no ALL CAPS character format for the chart title in Calc
Comment 2 m.a.riosv 2018-01-24 16:17:05 UTC
Repro
Version: 5.4.4.2 (x64)
Build ID: 2524958677847fb3bb44820e40380acbe820f960
CPU threads: 4; OS: Windows 6.19; UI render: GL; 
Locale: es-ES (es_ES); Calc: CL

The 3D is marked on the char type but not selection between 'Simple' or 'Realistic'
Data format is imported 'Show values as percentage' with 'Percent format' as 'Source format'
Comment 3 Andras Timar 2018-10-15 21:04:57 UTC
A related commit:
commit d22a4d945ccf1456fbdb2c39802d956afa583a2a
Author: Matúš Kukan <matus.kukan@collabora.com>
Date:   Thu Sep 11 08:56:22 2014 +0200

    bnc#892610: OOXML import: Improve chart number formats.
    
    If sourceLinked is used, do not set "PercentageNumberFormat" even if
    showPercent is true. The format string should be used for "NumberFormat".
    
    c8cc89ff802d86b1f3a69afe1b4835b7df7f70c7 unnecessarily disabled
    "LinkNumberFormatToSource". Use that for data labels but not for axis.
    
    Also, actually make attaching number format supplier work for Calc.
    Previously, non standard formats were added into wrong supplier,
    and they were thrown away later because it was attached too late.
    (See also ChartModel::attachNumberFormatsSupplier)
    
    Change-Id: Iaf9945abc3d82d0ac63d9f36b8888eb49f39ab57

It seems this logic is wrong. 

diff --git a/oox/source/drawingml/chart/objectformatter.cxx b/oox/source/drawingml/chart/objectformatter.cxx
index 8d49c0d01fef..a90ca8f49f33 100644
--- a/oox/source/drawingml/chart/objectformatter.cxx
+++ b/oox/source/drawingml/chart/objectformatter.cxx
@@ -1080,7 +1080,7 @@ void ObjectFormatter::convertNumberFormat( PropertySet& rPropSet, const NumberFo
     if( mxData->mxNumFmts.is() )
     {
         const bool bGeneral = rNumberFormat.maFormatCode.equalsIgnoreAsciiCase("general");
-        const bool bPercent = !bAxis && bShowPercent && !rNumberFormat.mbSourceLinked;
+        const bool bPercent = !bAxis && bShowPercent;
         sal_Int32 nPropId = bPercent ? PROP_PercentageNumberFormat : PROP_NumberFormat;
         OUString sFormatCode(rNumberFormat.maFormatCode);
         if (bPercent && bGeneral)

After this, percent values are shown as expected, but it breaks a unit test (and indeed, import of chart2/qa/extras/data/xlsx/number-formats.xlsx becomes wrong).
Comment 4 Aron Budea 2018-10-15 21:42:12 UTC
This seems to be a regression, which started from the commit referenced below (bibisected using repo bibisect-win32-5.1). I can't establish if this is indeed a regression, or if it had worked fine by accident before the patch, Eike, could you please share your thoughts?

https://cgit.freedesktop.org/libreoffice/core/commit/?id=0f4b3cb7d3d68906de316a64dcec281da2a641bd
author		Eike Rathke <erack@redhat.com>	2015-08-10 17:04:13 +0200
committer	Eike Rathke <erack@redhat.com>	2015-08-10 17:12:45 +0200

if we have a number formatter then use it, dammit..
Comment 5 Eike Rathke 2018-10-16 09:45:48 UTC
Old code obtained all percent formats for the locale and used the first it encountered, which by chance was one without decimals. New code uses the defined default percent format of the locale. If here it has to be without decimals (which for a pie chart certainly makes sense) then we'll have to do it differently.
Comment 6 Commit Notification 2018-10-16 20:17:32 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9672d034b9e760f24ac9a6652ab45dee15ee260a

Resolves: tdf#115182 default Chart percentage format is integer

It will be available in 6.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Commit Notification 2018-10-16 22:49:30 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7d17e8e1798b7120b9e8559d042de2afc0a078e3&h=libreoffice-6-1

Resolves: tdf#115182 default Chart percentage format is integer

It will be available in 6.1.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Xisco Faulí 2018-10-17 16:23:26 UTC
Verified in

Version: 6.2.0.0.alpha0+
Build ID: 647fc41763d1310479d59262734caa296f6e558d
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: en-US (ca_ES.UTF-8); Calc: threaded

@Eike, Thanks for fixing this!!

The problem with the percentage position is already reported in bug 117705 as it's caused by e1697600253361a26d77a1ef61f8bde16af0ed2c
Comment 9 Commit Notification 2018-10-19 08:35:25 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-6-1-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e3e7f45ea4b07e2cbeaad75d46edc1b8fb9160fc&h=libreoffice-6-1-3

Resolves: tdf#115182 default Chart percentage format is integer

It will be available in 6.1.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.