Bug 382427 - Incorrect formatting of number display of point label in graph reports
Summary: Incorrect formatting of number display of point label in graph reports
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: 4.8.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
: 383110 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-07-17 11:45 UTC by Ralf Habacker
Modified: 2019-08-29 00:54 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.8.1
ralf.habacker: Version_5-


Attachments
Net worth graphs from bug #382378 on master (74.41 KB, image/png)
2017-08-02 16:51 UTC, NSLW
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2017-07-17 11:45:16 UTC
https://bugsfiles.kde.org/attachment.cgi?id=106647
Comment 1 Ralf Habacker 2017-07-17 11:50:00 UTC
The mentioned screenshot shows that point labels are formatted wrongly. For example the number is 18.000.000 and the label shows 1.80.
Comment 2 Ralf Habacker 2017-07-17 14:02:37 UTC
The problem is located inside class void KDChart::AbstractDiagram::Private

methods

 void paintDataValueText( const AbstractDiagram* diag,
                                 QPainter* painter,
                                 const QModelIndex& index,
                                 const QPointF& pos,
                                 double value,
                                 bool justCalculateRect=false,
                                 QRectF* cumulatedBoundingRect=0 )

and  

QString roundValues( double value,
                     const int decimalPos,
                     const int decimalDigits) const;


They simply use QString::number(value) with default conversation settings which are QString::number(value, 'g', 6). A number of 18000000 is returned as "1.80e+7".

Also in KReportChartView::drawPivotChart() there is used 

  dataValueAttr.setDecimalDigits(MyMoneyMoney::denomToPrec(MyMoneyFile::instance()->baseCurrency().smallestAccountFraction()));

which limits the number of digits to 2 which results into 1.80 for the mentioned value 18000000. Another example is 180.000 which is displayed also as 1.80.

The fix is to refactor the mentioned methods.
Comment 3 NSLW 2017-07-30 08:00:47 UTC
Did you try master version?
Can you attach .kmy file with your graph?
I've worked in that area and would like it to look correct.
Comment 4 Ralf Habacker 2017-07-30 08:26:13 UTC
(In reply to NSLW from comment #3)
> Did you try master version?
no, cannot build master branch yet.
> Can you attach .kmy file with your graph?
see the attachments from bug 382378
Comment 5 Thomas Baumgart 2017-07-30 12:11:56 UTC
No, it does not. For some reason the opening balance of the account is not accounted in master using the demo file from bug 382378. The format of the numbers shown seems to be OK though in master. The incorrect values is certainly something we need to fix in 5.0 (maybe even open a separate bug entry).

For KDE4 I need to mention, that we do not fix stuff inside KDChart. This version was not maintained by KDE. This differs for KF5 and it could well be, that this has been fixed upstream (and that is why we see a different display in master). In case we cannot fix it using the API then it will stay the (broken) way it is.
Comment 6 Ralf Habacker 2017-07-31 09:04:54 UTC
For the record: 

In 4.8 branch kdchart has been updated from time to time to a more recent update:

- 2012-02-18 Update our internal copy of KDChart to version 2.4.3.
  https://cgit.kde.org/kmymoney.git/commit/libkdchart?h=4.8&id=9ec18e4857d654b3dfdace09228cba6a5da8216f

- 2011-05-24  Upgrade our copy of KDChart to version 2.4.1
  https://cgit.kde.org/kmymoney.git/commit/libkdchart?h=4.8&id=89d7f29479db157f82bc2d0d558357efe58ec5a5

Unfortunally after 2012 this has not been continued and results in a very outdated version of kdchart compared to the recent version 2.6.0.
Comment 7 NSLW 2017-08-02 16:51:17 UTC
Created attachment 107041 [details]
Net worth graphs from bug #382378 on master

No problem here. Aren't the graphs look correct?
Comment 8 Ralf Habacker 2017-08-02 18:59:36 UTC
(In reply to NSLW from comment #7)
> No problem here. Aren't the graphs look correct?

This is because kmymoney from master branch uses KDChart version 2.6.0 which has this bug fixed. 4.8 branch uses KDChart version 2.4.3 which is broken in this area.
Comment 9 NSLW 2017-08-03 12:55:26 UTC
(In reply to Ralf Habacker from comment #8)
> (In reply to NSLW from comment #7)
> > No problem here. Aren't the graphs look correct?
> 
> This is because kmymoney from master branch uses KDChart version 2.6.0 which
> has this bug fixed. 4.8 branch uses KDChart version 2.4.3 which is broken in
> this area.

Then I would suggest either fix it or mark as resolved-wontfix, so that this bug doesn't pollute bug list.
Comment 10 Ralf Habacker 2017-08-03 19:32:12 UTC
(In reply to NSLW from comment #9)
> (In reply to Ralf Habacker from comment #8)
> > (In reply to NSLW from comment #7)
> > > No problem here. Aren't the graphs look correct?
> > 
> > This is because kmymoney from master branch uses KDChart version 2.6.0 which
> > has this bug fixed. 4.8 branch uses KDChart version 2.4.3 which is broken in
> > this area.
> 
> Then I would suggest either fix it 

I'm mostly ready with this - see at https://github.com/rhabacker/kmymoney/commits/4.8-kdchart-2.6. 
Should I open a review request for this or simply merge into 4.8 ?
Comment 11 NSLW 2017-08-04 14:09:47 UTC
(In reply to Ralf Habacker from comment #10)
> (In reply to NSLW from comment #9)
> > (In reply to Ralf Habacker from comment #8)
> > > (In reply to NSLW from comment #7)
> > > > No problem here. Aren't the graphs look correct?
> > > 
> > > This is because kmymoney from master branch uses KDChart version 2.6.0 which
> > > has this bug fixed. 4.8 branch uses KDChart version 2.4.3 which is broken in
> > > this area.
> > 
> > Then I would suggest either fix it 
> 
> I'm mostly ready with this - see at
> https://github.com/rhabacker/kmymoney/commits/4.8-kdchart-2.6. 
> Should I open a review request for this or simply merge into 4.8 ?

Maybe Thomas would like to review this, but if you just removed old one version and pasted new one version and it works for you, then I think there is nothing to review.
Comment 12 Ralf Habacker 2017-08-05 07:47:21 UTC
(In reply to NSLW from comment #11)
> (In reply to Ralf Habacker from comment #10)
> > (In reply to NSLW from comment #9)
> > > (In reply to Ralf Habacker from comment #8)
> > > > (In reply to NSLW from comment #7)
> > > > > No problem here. Aren't the graphs look correct?
> > > > 
> > > > This is because kmymoney from master branch uses KDChart version 2.6.0 which
> > > > has this bug fixed. 4.8 branch uses KDChart version 2.4.3 which is broken in
> > > > this area.
> > > 
> > > Then I would suggest either fix it 
> > 
> > I'm mostly ready with this - see at
> > https://github.com/rhabacker/kmymoney/commits/4.8-kdchart-2.6. 
> > Should I open a review request for this or simply merge into 4.8 ?
> 
> Maybe Thomas would like to review this, but if you just removed old one
> version and pasted new one version 
yes 
> and it works for you, then I think there is nothing to review.
I did not changed any major things. From the git log mentioned above you can see that there is one commit with the update, two commits with fixed bugs I was faced on testing (may be useful for master too) and one commit which adds a simple chart test dialog, which makes it easier to debug chart issues in future.
Talking about the bugs I guess it would be better to open separate bug reports for it  to be able to have it public.

Also I will merge the two remaining commits - the update and the test dialog into one commit to solve this bug.
Comment 13 Ralf Habacker 2017-08-05 08:41:19 UTC
Git commit cb841612e11349aca26b6c4af4ff7dced9a95357 by Ralf Habacker.
Committed on 05/08/2017 at 08:35.
Pushed by habacker into branch '4.8'.

Update to KDChart 2.6

Also add test report chart.

FIXED-IN:4.8.1

https://commits.kde.org/kmymoney/cb841612e11349aca26b6c4af4ff7dced9a95357
Comment 14 Ralf Habacker 2017-08-05 08:44:43 UTC
Git commit 51529f756008f2c879a92b0b3fef72a716e51e2b by Ralf Habacker.
Committed on 05/08/2017 at 08:44.
Pushed by habacker into branch '4.8'.

Add missing copyright header

bugzilla complaints about this.

M  +16   -0    kmymoney/reports/reportcharttest.cpp

https://commits.kde.org/kmymoney/51529f756008f2c879a92b0b3fef72a716e51e2b
Comment 15 Ralf Habacker 2017-08-05 09:03:56 UTC
*** Bug 383110 has been marked as a duplicate of this bug. ***