Add Cursor
ClosedPublic

Authored by Murmele on Jun 4 2019, 1:56 PM.

Details

Summary

This patch adds the ability to create cursors to measure values in the curves (x, y)

TODO:

  • icon for the cursor action
  • when clicking on the selection tool, the cursor disappear. Don't understand why

Diff Detail

Repository
R262 LabPlot
Lint
Lint Skipped
Unit
Unit Tests Skipped
Murmele created this revision.Jun 4 2019, 1:56 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJun 4 2019, 1:56 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
Murmele requested review of this revision.Jun 4 2019, 1:56 PM
Murmele updated this revision to Diff 59226.Jun 5 2019, 8:26 PM
  • fix bugs
asemke added a comment.Jun 8 2019, 7:43 PM

The class name TreeModel sounds very generic but this model will most probably only used for cursors only. Can we call it CursorTreeModel similar to how have AspectTreeModel already?

The are several code style glitches. Please execute astyle with the options in admin/README.astyle.

src/backend/core/AbstractAspect.cpp
339 ↗(On Diff #59226)

remove empty line.

src/backend/core/column/ColumnPrivate.cpp
1289 ↗(On Diff #59226)

std::isnan(value)

1299 ↗(On Diff #59226)

std::isnan(value)

1301 ↗(On Diff #59226)

small typo - leave, not leaf.

src/backend/worksheet/TreeModel.cpp
36 ↗(On Diff #59226)

use constructor member initializer list.

src/backend/worksheet/TreeModel.h
63 ↗(On Diff #59226)

TreeItem* parentItem{nullptr};

74 ↗(On Diff #59226)

TreeModel(const QStringList& headers, QObject* parent = nullptr);

106 ↗(On Diff #59226)

TreeItem* rootItem{nullptr};

src/backend/worksheet/Worksheet.cpp
427 ↗(On Diff #59226)

remove empty line.

429 ↗(On Diff #59226)

no brackets for one-liners.

976 ↗(On Diff #59226)

In curveDataChanged() and in the following functions there is a lot of logic handling the tree model. Can me move this code into TreeModel and have in Worksheet only some abstract interface of the model hiding the technical details?

1226 ↗(On Diff #59226)

add i18n for "Plot/Curve".

src/backend/worksheet/plots/cartesian/CartesianCoordinateSystem.cpp
250 ↗(On Diff #59226)

add spaces between the operators here.

src/backend/worksheet/plots/cartesian/CartesianPlot.cpp
1069 ↗(On Diff #59226)

do we really need to retransform the scale if we set the pen for the cursor?

1359 ↗(On Diff #59226)

"signal" is not the terminology used in the code. Let's stick to "curve".

3050 ↗(On Diff #59226)

use WRITE_QPEN macro.

src/commonfrontend/ProjectExplorer.cpp
629 ↗(On Diff #59226)

use directly selectedAspects << static_cast<AbstractAspect*>(index.internalPointer());, no need for an extra variable and this assignment in the loop.

src/kdefrontend/MainWin.cpp
179 ↗(On Diff #59226)

i18n("Cursor")

190 ↗(On Diff #59226)

isn't this obsolete since you set the title in the constructor already?

src/kdefrontend/dockwidgets/CartesianPlotDock.h
181 ↗(On Diff #59226)

void plotCursorPenChanged(QPen);

Murmele marked 18 inline comments as done.Jun 20 2019, 12:50 PM
Murmele added inline comments.
src/backend/worksheet/plots/cartesian/CartesianPlot.cpp
1069 ↗(On Diff #59226)

No, think update should do the same job

Murmele marked 3 inline comments as done.Jun 20 2019, 1:16 PM
Murmele updated this revision to Diff 61288.Jul 7 2019, 4:50 PM
Murmele updated this revision to Diff 61289.
sgerlach added inline comments.
src/backend/worksheet/TreeModel.cpp
149 ↗(On Diff #59226)

use range-based loop

165 ↗(On Diff #59226)

indentation?

src/backend/worksheet/Worksheet.cpp
1334 ↗(On Diff #59226)

auto* sender = ...

1356 ↗(On Diff #59226)

auto* plot = ...

1480 ↗(On Diff #59226)

auto*

1519 ↗(On Diff #59226)

auto*

1563 ↗(On Diff #59226)

auto*

src/backend/worksheet/plots/cartesian/CartesianCoordinateSystem.cpp
609 ↗(On Diff #59226)

define temp vars for pageRect.x() and pageRect.y() too?

src/backend/worksheet/plots/cartesian/CartesianPlot.cpp
2575 ↗(On Diff #59226)

same as
if (cursorPenWidth2 < 10) cursorPenWidth2 = 10;

2689 ↗(On Diff #59226)

translation?

Murmele marked 9 inline comments as done.Jul 18 2019, 8:21 PM
Murmele updated this revision to Diff 62011.Jul 18 2019, 8:33 PM
Murmele marked an inline comment as done.
sgerlach added inline comments.Jul 19 2019, 3:46 AM
src/backend/worksheet/plots/cartesian/CartesianCoordinateSystem.cpp
711 ↗(On Diff #59226)

use temp vars here too?

src/backend/worksheet/plots/cartesian/CartesianPlot.cpp
2641 ↗(On Diff #59226)

use spaces: } else {

2661 ↗(On Diff #59226)

space after "if": if (...)

2690 ↗(On Diff #59226)

maybe better:
cursorNumber == 0 ? cursor0Enable = true : cursor1Enable = true;

2804 ↗(On Diff #59226)

maybe better:
cursorNumber == 0 ? cursor0Pos = p1 : cursor1Pos = p1;

2972 ↗(On Diff #59226)

same as
if (cursorPenWidth2 < 10)
cursorPenWidth2 = 10;

src/backend/worksheet/plots/cartesian/XYCurve.cpp
2314 ↗(On Diff #59226)

do you really want abs() defined as
int abs(int)
or fabs() defined as
double fabs(double)
or even qAbs() defined as
T qAbs(const T&)

please check all places where abs() is used

src/commonfrontend/worksheet/WorksheetView.cpp
1816 ↗(On Diff #59226)

no brackets for one-liners

src/kdefrontend/dockwidgets/CartesianPlotDock.cpp
103 ↗(On Diff #59226)

This is Qt::PenStyle, right?
Can we use a global const QStringList containing the names? Also translation would be good :-)

src/kdefrontend/dockwidgets/XYCurveDock.cpp
513 ↗(On Diff #62011)

space before {

There are couple of easy-to-fix compiler warnings

/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:738:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotActionMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:752:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotActionMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:766:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotCursorMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:783:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotActionMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:797:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotActionMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:811:32: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if (cartesianPlotCursorMode() == WorksheetView::ApplyActionToAll) {
            ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:843:31: warning: comparison of two values with different enumeration types ('Worksheet::CartesianPlotActionMode' and 'WorksheetView::CartesianPlotActionMode') [-Wenum-compare]
        if(cartesianPlotCursorMode() == WorksheetView::ApplyActionToAll){
           ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:997:7: warning: unused variable 'curveCount' [-Wunused-variable]
                int curveCount = treeModel->rowCount(plotIndex);
                    ^
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:1037:7: warning: unused variable 'curveCount' [-Wunused-variable]
                int curveCount = treeModel->rowCount(plotIndex);
                    ^
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:1208:8: warning: unused variable 'rowCount' [-Wunused-variable]
                        int rowCount = treeModel->rowCount(plotName);
                            ^
/Users/d058339/Projekte/labplot/src/backend/worksheet/Worksheet.cpp:1209:9: warning: unused variable 'success' [-Wunused-variable]
                        bool success = treeModel->insertRows(treeModel->rowCount(plotName), 1, plotName);

/Users/d058339/Projekte/labplot/src/backend/worksheet/TreeModel.cpp:36:2: warning: field 'parentItem' will be initialized after field 'itemData' [-Wreorder]
        parentItem(parent),
        ^
/Users/d058339/Projekte/labplot/src/backend/worksheet/TreeModel.cpp:315:19: warning: code will never be executed [-Wunreachable-code]
    bool result = rootItem->setData(section, value);
                  ^~~~~~~~
/Users/d058339/Projekte/labplot/src/backend/worksheet/TreeModel.cpp:312:60: warning: code will never be executed [-Wunreachable-code]
    if (role != Qt::EditRole || role != Qt::DisplayRole || orientation != Qt::Horizontal)

The new cursor dock needs to be cleared and hidden when we close the project or create a new one.

Murmele updated this revision to Diff 62072.Jul 19 2019, 6:52 PM
Murmele marked 9 inline comments as done.Jul 19 2019, 7:24 PM
Murmele added inline comments.
src/kdefrontend/dockwidgets/CartesianPlotDock.cpp
103 ↗(On Diff #59226)

Jop it is :)

Can we use a global const QStringList containing the names? Also translation would be good :-)

Jep

Murmele marked 3 inline comments as done.Jul 19 2019, 7:25 PM

The new cursor dock needs to be cleared and hidden when we close the project or create a new one.

Done

Murmele updated this revision to Diff 62079.Jul 19 2019, 7:44 PM
Murmele updated this revision to Diff 62164.Jul 21 2019, 7:40 AM
asemke accepted this revision.Jul 21 2019, 7:07 PM
This revision is now accepted and ready to land.Jul 21 2019, 7:07 PM
Murmele updated this revision to Diff 62230.Jul 21 2019, 7:40 PM
This revision was automatically updated to reflect the committed changes.