Bug 420767

Summary: Incorrect ordinate axis labels when zooming a chart
Product: [Applications] kmymoney Reporter: Robert Szczesiak <dev.rszczesiak>
Component: reportsAssignee: KMyMoney Devel Mailing List <kmymoney-devel>
Status: RESOLVED FIXED    
Severity: normal CC: dev.rszczesiak, me
Priority: NOR    
Version: git (master)   
Target Milestone: ---   
Platform: Manjaro   
OS: Linux   
Latest Commit: Version Fixed In: 5.1.0
Attachments: Zoomed account balance chart
Newly opened account balance chart before zooming

Description Robert Szczesiak 2020-04-29 19:39:33 UTC
Created attachment 128010 [details]
Zoomed account balance chart

SUMMARY
When one zooms a chart (using so-called rubber band zooming feature from KDiagram project) axis ticks and labels are recalculated but they are overridden with customized "hard labels". The number of axis ticks is increasing and becomes grater than the count of hard labels, so the labels are repeated in cycle. In and of itself, it is really a feature rather than a bug but the feature works to our disadvantage in this case producing incorrect ordinate axis labels.

STEPS TO REPRODUCE
1. Open any account ledger and click on "Show balance chart" button at the top.
2. Click and drag your left mouse button to zoom boxed area.

OBSERVED RESULT
Ordinate axis labels are incorrect and repeated in cycle.

EXPECTED RESULT
Ordinate axis labels should match zoomed chart's data.

SOFTWARE/OS VERSIONS
KDE Plasma Version: 5.18.4
KDE Frameworks Version: 5.69.0
Qt Version: 5.14.2

ADDITIONAL INFORMATION
Please find attached a screenshot presenting the issue.
Comment 1 Robert Szczesiak 2020-04-29 19:54:36 UTC
Created attachment 128011 [details]
Newly opened account balance chart before zooming
Comment 2 Thomas Baumgart 2020-05-02 05:12:58 UTC
Git commit 5e4c8322fbefcb6cdaf1c51f6204986946ca0ba4 by Thomas Baumgart, on behalf of Robert Szczesiak.
Committed on 02/05/2020 at 05:11.
Pushed by tbaumgart into branch 'master'.

Fix ordinate axis labels when zooming

Summary:
If I understand the intent of the author correctly, the sole purpose of
`KReportChartView::slotNeedUpdate()` was one-time customization of
ordinate axis labels at the time of chart creation. The fact that signal
is disconnected immediately after execution of slot starts seems to
confirm my understanding.

The problem begins when one zooms the chart (using so-called rubber band
zooming feature from KDiagram project). Axis ticks and labels are
recalculated but still they are overridden with "hard labels" defined in
the slot body. The number of axis ticks increases and is grater than the
number of hard labels, so the labels are repeated in cycle. In and of
itself it is really a feature rather than a bug but the feature works to
our disadvantage in this case.
Please find below a quote from AbstractAxis Class documentation:
> void AbstractAxis::setLabels( const QStringList & list )
> Use this to specify your own set of strings, to be used as axis
labels.
> Labels specified via setLabels take precedence: If a non-empty list is
passed, KD Chart will use these strings as axis labels, **instead of
calculating them.**
> **If you pass a smaller number of strings than the number of labels
drawn at this axis, KD Chart will repeat the strings until all labels
are drawn**. As an example you could specify the seven days of the week
as abscissa labels, which would be repeatedly used then.

Unfortunately, mere removal of disconnect method call does not solve the
problem but the same documentation provides a way to customize axis
labels when needed. It is done through overriding of `const QString
AbstractAxis::customizedLabel( const QString & label ) const`

My solution proposed in attached patch goes this way and also simplifies
calculations where they seemed over-complicated to me. Please correct my
understanding if i missed the point of removed lines.

Please find attached screenshots before and after the fix.
Balance history chart before fix with no zoom:
{F8268351}
Balance history chart before fix with zoom::
{F8268353}
Balance history chart after fix with no zoom:
{F8268359}
Balance history chart after fix with zoom::
{F8268358}

As a footnote, I would like to sincerely thank to all the people who
have contributed to this project. I started to use KMyMoney recently and
I am surprised how good it is, especially for an open-source project run
by the community. It suits my need for personal finance and
investment-supporting software amazingly well. If you find my
contribution valuable, I would love to provide more patches in the
future if I find more bugs or feel that some feature is missing. In
fact, I have little to no industrial experience in C++ as I work mostly
in Java. Please let me know if I broke any C++ coding rules.

Reviewers: #kmymoney, tbaumgart

Reviewed By: #kmymoney, tbaumgart

Subscribers: tbaumgart

Tags: #kmymoney, #kdiagram

Differential Revision: https://phabricator.kde.org/D29255

M  +1    -0    kmymoney/plugins/views/reports/core/CMakeLists.txt
A  +57   -0    kmymoney/plugins/views/reports/core/kreportcartesianaxis.cpp     [License: GPL (v2+)]
A  +48   -0    kmymoney/plugins/views/reports/core/kreportcartesianaxis.h     [License: GPL (v2+)]
M  +26   -56   kmymoney/plugins/views/reports/core/kreportchartview.cpp
M  +6    -2    kmymoney/plugins/views/reports/core/kreportchartview.h
M  +1    -1    kmymoney/plugins/views/reports/reporttabimpl.cpp

https://commits.kde.org/kmymoney/5e4c8322fbefcb6cdaf1c51f6204986946ca0ba4
Comment 3 Thomas Baumgart 2020-05-02 07:57:34 UTC
Git commit 332b888252b4bd8e08b9d21e21d34e5be3ac25b9 by Thomas Baumgart.
Committed on 02/05/2020 at 07:57.
Pushed by tbaumgart into branch '5.0'.

Fix ordinate axis labels when zooming

Summary:
If I understand the intent of the author correctly, the sole purpose of
`KReportChartView::slotNeedUpdate()` was one-time customization of
ordinate axis labels at the time of chart creation. The fact that signal
is disconnected immediately after execution of slot starts seems to
confirm my understanding.

The problem begins when one zooms the chart (using so-called rubber band
zooming feature from KDiagram project). Axis ticks and labels are
recalculated but still they are overridden with "hard labels" defined in
the slot body. The number of axis ticks increases and is grater than the
number of hard labels, so the labels are repeated in cycle. In and of
itself it is really a feature rather than a bug but the feature works to
our disadvantage in this case.
Please find below a quote from AbstractAxis Class documentation:
> void AbstractAxis::setLabels( const QStringList & list )
> Use this to specify your own set of strings, to be used as axis
labels.
> Labels specified via setLabels take precedence: If a non-empty list is
passed, KD Chart will use these strings as axis labels, **instead of
calculating them.**
> **If you pass a smaller number of strings than the number of labels
drawn at this axis, KD Chart will repeat the strings until all labels
are drawn**. As an example you could specify the seven days of the week
as abscissa labels, which would be repeatedly used then.

Unfortunately, mere removal of disconnect method call does not solve the
problem but the same documentation provides a way to customize axis
labels when needed. It is done through overriding of `const QString
AbstractAxis::customizedLabel( const QString & label ) const`

My solution proposed in attached patch goes this way and also simplifies
calculations where they seemed over-complicated to me. Please correct my
understanding if i missed the point of removed lines.

Please find attached screenshots before and after the fix.
Balance history chart before fix with no zoom:
{F8268351}
Balance history chart before fix with zoom::
{F8268353}
Balance history chart after fix with no zoom:
{F8268359}
Balance history chart after fix with zoom::
{F8268358}

As a footnote, I would like to sincerely thank to all the people who
have contributed to this project. I started to use KMyMoney recently and
I am surprised how good it is, especially for an open-source project run
by the community. It suits my need for personal finance and
investment-supporting software amazingly well. If you find my
contribution valuable, I would love to provide more patches in the
future if I find more bugs or feel that some feature is missing. In
fact, I have little to no industrial experience in C++ as I work mostly
in Java. Please let me know if I broke any C++ coding rules.

Reviewers: #kmymoney, tbaumgart

Reviewed By: #kmymoney, tbaumgart

Subscribers: tbaumgart

Tags: #kmymoney, #kdiagram

Differential Revision: https://phabricator.kde.org/D29255

(cherry picked from commit 5e4c8322fbefcb6cdaf1c51f6204986946ca0ba4)

M  +1    -0    kmymoney/plugins/views/reports/core/CMakeLists.txt
A  +57   -0    kmymoney/plugins/views/reports/core/kreportcartesianaxis.cpp     [License: GPL (v2+)]
A  +48   -0    kmymoney/plugins/views/reports/core/kreportcartesianaxis.h     [License: GPL (v2+)]
M  +26   -56   kmymoney/plugins/views/reports/core/kreportchartview.cpp
M  +6    -2    kmymoney/plugins/views/reports/core/kreportchartview.h
M  +1    -1    kmymoney/plugins/views/reports/reporttabimpl.cpp

https://commits.kde.org/kmymoney/332b888252b4bd8e08b9d21e21d34e5be3ac25b9