Fixing
ClosedPublic

Authored by Murmele on Jan 23 2019, 5:21 PM.

Details

Reviewers
asemke
Summary
  • add DateTimeSpinbox and normal qspinbox instead of lineEdit
  • move corner to tick position if abs(angle) >= 0.01°
  • fix axis label moving in x direction with offset
  • don't move axistitle, if the axislabels are inside the plot

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Murmele created this revision.Jan 23 2019, 5:21 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJan 23 2019, 5:21 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
Murmele requested review of this revision.Jan 23 2019, 5:21 PM

Thanks in advance for fixing a minor typo.

src/commonfrontend/widgets/DateTimeSpinBox.h
9 ↗(On Diff #50129)

Typo: alway -> always

sgerlach added inline comments.
src/commonfrontend/widgets/DateTimeSpinBox.cpp
14 ↗(On Diff #50129)

warning: unknown escape sequence: '\.'

23 ↗(On Diff #50129)

warning: unused variable ‘type’

201 ↗(On Diff #50129)

warning: unused parameter ‘nextType’

204 ↗(On Diff #50129)

warning: enumeration value ‘millisecond’ not handled in switch

src/commonfrontend/widgets/DateTimeSpinBox.h
29 ↗(On Diff #50129)

warning: 'stepEnabled' overrides a member function but is not marked 'override'

DateTimeSpinBox is a useful widget. I think we can use it at some other places, too. Thanks for this contribution! Can you please add license headers to the new files?

I played a bit with the data from intraday.json. I added "1. open" to the y-values. The y-axis ranges in this case from 101.27 to 101.375. The paging step in the DoubleSpinBox for the tick increment is set to 1. With this, when changing the value once, we increment by 1 and all the ticks disappear. Can we adjust this paging step automatically to the current ranges of the axis somehow? This would further improve the user experience.

Murmele marked 6 inline comments as done.Jan 24 2019, 10:38 AM
Murmele added inline comments.
src/commonfrontend/widgets/DateTimeSpinBox.cpp
14 ↗(On Diff #50129)

I tried in a qt validator program and there \\. did not work, but in labplot it seems to work

src/commonfrontend/widgets/DateTimeSpinBox.h
9 ↗(On Diff #50129)

thanks :)

Murmele updated this revision to Diff 50174.Jan 24 2019, 10:39 AM
Murmele marked 2 inline comments as done.
  • determine decimals and steps for the qdoubleSpinbox
  • removed not used variables
  • update comments
Murmele updated this revision to Diff 50176.Jan 24 2019, 10:43 AM

add comments

asemke added inline comments.Jan 24 2019, 9:02 PM
src/backend/worksheet/plots/cartesian/Axis.cpp
1569–1570 ↗(On Diff #50129)

cosinus and sinus only used in the definition of diffx and diffy and can be used there directly.

1572–1581 ↗(On Diff #50129)

why do you need to check orientation here?

1599 ↗(On Diff #50129)

improve the wording a bit. Maybe something like

"for rotated labels (angle is not zero), align label's corner at the position of the tick"

1600 ↗(On Diff #50129)

if I set Rotation=90° and Offset=0pt, I'd expect the label to be positioned right next to the tick and aligned at the center of the rotated label. This is not the case with this logic.

src/kdefrontend/dockwidgets/AxisDock.cpp
72 ↗(On Diff #50129)

let's use hard coded row numbers here instead of this for loop with all the logic that is not so easy to grasp.

849 ↗(On Diff #50129)

this check is not required. An axis has always a plot as the parent.

911 ↗(On Diff #50129)

same as above.

919 ↗(On Diff #50129)

no brackets for one-liners.

931 ↗(On Diff #50129)

no brackets for one-liners.

1874 ↗(On Diff #50129)

why this change?

1882 ↗(On Diff #50129)

why this change?

Murmele marked 5 inline comments as done.Jan 25 2019, 6:57 AM
Murmele added inline comments.
src/backend/worksheet/plots/cartesian/Axis.cpp
1569–1570 ↗(On Diff #50129)

otherwise sinus and cosinus will be calculated every loop cycle

1572–1581 ↗(On Diff #50129)

before there was only
labelsFormat == Axis::FormatDecimal || labelsFormat == Axis::FormatScientificE

but the labelsformat for datetime is different and it cannot be checked, if the labelsFormat == Axis::FormatDecimal ...

1600 ↗(On Diff #50129)

Thats correct :) I have to check.

Murmele updated this revision to Diff 50245.Jan 25 2019, 12:56 PM
Murmele marked 6 inline comments as done.
Murmele marked an inline comment as done.Jan 25 2019, 1:02 PM
Murmele updated this revision to Diff 50247.Jan 25 2019, 1:09 PM
Murmele edited the summary of this revision. (Show Details)
Murmele updated this revision to Diff 50249.Jan 25 2019, 1:39 PM
Murmele updated this revision to Diff 50250.
Murmele updated this revision to Diff 50312.Jan 26 2019, 11:46 AM
Murmele updated this revision to Diff 50318.Jan 26 2019, 1:39 PM
Murmele updated this revision to Diff 50319.

add DateTimeSpinbox.cpp to cmake file

asemke accepted this revision.Jan 26 2019, 4:59 PM

Looks good now. I'll add additional escaping in QRegularExpression in DateTimeSpinBox to get rid of compiler warnings and push your fix.

This revision is now accepted and ready to land.Jan 26 2019, 4:59 PM
asemke closed this revision.Dec 6 2019, 8:42 PM