Bug 72536 - Eliminate calls to RegressionCurveHelper::getFirstCurveNotMeanValueLine (and company)
Summary: Eliminate calls to RegressionCurveHelper::getFirstCurveNotMeanValueLine (and ...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Chart (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: low normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyMedium, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Chart
  Show dependency tree / graph
 
Reported: 2013-12-09 19:40 UTC by Tomaz Vajngerl
Modified: 2020-07-22 15:02 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tomaz Vajngerl 2013-12-09 19:40:07 UTC 
Calls to RegressionCurveHelper::getFirstCurveNotMeanValueLine in [1] (and calls to other similar methods on the same class) mostly assume that only one trendline / regression curve per data series can be present. With 4.2 (and ODF 1.2) this is no longer true as there can be multiple trendlines for one data series. This calls must be inspected and changed to act appropriately in those scenarios, the method getFirstCurveNotMeanValueLine (and similar) must be removed.

[1] chart2/source/inc/RegressionCurveHelper.hxx

Regards,
Tomaž
Comment 1 QA Administrators 2015-04-19 03:22:06 UTC Comment hidden (obsolete)
Comment 2 QA Administrators 2016-09-20 09:24:40 UTC Comment hidden (obsolete)
Comment 3 Xisco Faulí 2017-07-13 12:37:48 UTC 
Setting Assignee back to default. Please assign it back to yourself if you're
still working on this issue
Comment 4 QA Administrators 2018-07-14 02:47:01 UTC Comment hidden (obsolete)
Comment 5 Roman Kuznetsov 2019-03-18 22:00:52 UTC 
Tomaž, is it still actual now?
Comment 6 Tomaz Vajngerl 2019-03-20 13:09:34 UTC 
AFAIK nothing changed there regarding this, yes. This method still exists and is used. 

OTOH, this bug could be an easyhack (and it already has a code pointer).
Comment 7 Xisco Faulí 2019-03-20 13:12:41 UTC 
Let make it an easyhack then...
Comment 8 Mehmet Sait Gülmez 2019-04-23 19:11:16 UTC 
I'm currently starting work on this bug.
Comment 9 Xisco Faulí 2019-08-06 14:33:22 UTC 
Dear Mehmet Sait Gülmez,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assigned it back to yourself if you're still working on this.
Comment 10 Shivam Kumar Singh 2020-01-19 08:22:38 UTC 
Assigned to shivamhere247@gmail.com
Comment 11 Shivam Kumar Singh 2020-01-19 08:31:18 UTC 
> This calls must be inspected and changed to act
> appropriately in those scenarios, the method getFirstCurveNotMeanValueLine
> (and similar) must be removed.


I don't understand how removing getFirstCurveNotMeanValueLine will fix the bug.
Someone please mentor me on this.
Comment 12 Buovjaga 2020-01-20 13:20:07 UTC 
(In reply to Shivam Kumar Singh from comment #11)
> > This calls must be inspected and changed to act
> > appropriately in those scenarios, the method getFirstCurveNotMeanValueLine
> > (and similar) must be removed.
> 
> 
> I don't understand how removing getFirstCurveNotMeanValueLine will fix the
> bug.
> Someone please mentor me on this.

Notice the two points:
1. The calls must be inspected and changed to act appropriately in those scenarios
2. The method getFirstCurveNotMeanValueLine (and similar) must be removed
Comment 13 Shivam Kumar Singh 2020-01-20 19:10:09 UTC 
Hi ,

> Notice the two points:
> 1. The calls must be inspected and changed to act appropriately in those
> scenarios

Calling getAllRegressionCurvesNotMeanValueLine instead of getFirstCurveNotMeanValueLine should do the task , because we need a whole vector of curves/trendlines and not just a single one .
Replacing the previous function calls with this is what's required . What do you think ?


std::vector< Reference< chart2::XRegressionCurve > > aRegressionCurves(
RegressionCurveHelper::getAllRegressionCurvesNotMeanValueLine( xDiagram ));
bool bHasEquation = false;
for( const auto& xCurve : aRegressionCurves )
        bHasEquation = bHasEquation || 
                                   RegressionCurveHelper::hasEquation(xCurve);

> 2. The method getFirstCurveNotMeanValueLine (and similar) must be removed

This is easy once 1 is done
Comment 14 Shivam Kumar Singh 2020-01-21 20:04:50 UTC 
I have submitted a patch relating to this bug, I was not able to fix the bug but I wanted to do some progress, as it is mentioned there can be more than one regression curves corresponding to data series , so I replaced getFirstCurveNotMeanValueLine with getAllregressionCurvesNotMeanValueLine and removed the function from RegressionCurveHelper.hxx and RegressionCurveHelper.cxx

This is my patch 
https://gerrit.libreoffice.org/c/core/+/87161
Please discuss furthur steps , to complete it .
Comment 15 Tomaz Vajngerl 2020-03-27 10:27:39 UTC 
Hi, sorry - somehow I missed this. 

Just changing one for another is not the solution. What is the correct way here is to inspect where getFirstCurveNotMeanValueLine is called and figure out what that does. 

For example if a menu action is called from the UI and you don't have a regression curve selected, then that menu action should be disabled and it shouldn't take the first regression curve. 

So the task is to search for uses of getFirstCurveNotMeanValueLine, then get all the way to where it is triggered in the UI and determine if the UI does what it should do, then work yourself all the way back to he call.

As said in the description - previously we only supported one regression curve and which had partial code overlapping with the mean curve. Now there can be multiple regression curves and we practically never should just assume to take the first regression curve (with getFirstCurveNotMeanValueLine), but the one that is selected or the action is not possible.

I think that in most cases you will get to a dead end when inspecting those calls to the UI, so they would be safe to remove.
Comment 16 Xisco Faulí 2020-07-22 15:02:21 UTC 
Dear Shivam Kumar Singh,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assign it back to yourself if you're still working on this.