Add time line to X axis
ClosedPublic

Authored by alexde on Aug 28 2019, 8:51 AM.

Details

Summary

The current energy plot does not display any time line. This patch adds a grid and time stamps to the X axis.
It is assumed that the X axis of the plot ranges over [Now - Timespan, Now].

The timestamps for the timespans greater than "1 hour" are aligned to either half hours or full hours.
The resolution respectively the divisions count or number of ticks is made dependend on the timespan.
Dates are only shown if the date changes in the timespan.

If there are more or equal 10 divisions, only every second grid line gets a label to avoid that
the labels pile up, when the window size becomes small.

This patch may be only of temporary nature until kf5quickcharts becomes production ready.

BUG: 376940
FIXED-IN: 5.17.0

As QML was absolutely new to me, I am looking forward to your review.

Test Plan



Diff Detail

Repository
R102 KInfoCenter
Branch
energy-timeline (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16531
Build 16549: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
alexde edited the summary of this revision. (Show Details)Aug 28 2019, 8:59 AM
alexde edited the summary of this revision. (Show Details)Aug 28 2019, 12:05 PM
alexde updated this revision to Diff 64840.Aug 28 2019, 12:56 PM

Set more sane left/right margins

Nice!

Only showing the date once per day is clever, but it can be even more clever: when all of the times in the graph correspond to the current day, there's no need to show it at all. You can see this issue in this screenshot:

As you can see, it's kind of funny that only one of the labels has a date under it. This is fine for the multi-day graphs, but for the graphs that are entirely within the current day, I would recommend omitting the date.

Code-wise, there are some formatting issues:

  • Don't add semicolons
  • Use 4 spaces for indentation
  • Clean up indentation and make it consistent
  • Add spaces between operators (e.g. diff / 1000)
This comment was removed by alexde.
alexde updated this revision to Diff 64889.Aug 28 2019, 8:40 PM

Resolve formatting issues

  • remove semicolons
  • replace tab indention by 4 spaces
  • add space between operator and arguments
alexde updated this revision to Diff 64890.Aug 28 2019, 8:52 PM

Omit the date if date equals today and the graph does not range over 24 hours.

alexde updated this revision to Diff 64892.Aug 28 2019, 9:07 PM

Update comment

alexde updated this revision to Diff 64896.Aug 28 2019, 9:37 PM

Improve the formatting's consistancy

Wuh-oh, there's something wrong with the date display now:

ngraham edited the summary of this revision. (Show Details)Aug 29 2019, 2:53 AM
ngraham edited the test plan for this revision. (Show Details)
alexde updated this revision to Diff 64920.Aug 29 2019, 7:39 AM

Fix displaying wrong variable.
Add postfix "Str" to indicate that those vars are explicitely strings.
Make sure that the date is only displayed when date has changed for <= 24h graphs.

Right now I only display the date under the tick mark if the date has changed and the tickmark number is odd, which I think is already good enough for this graph.
However, indicating a date change at midnight (12 pm / 0 am) would make sense as well. If you prefer that and find a suitable position for the indicator label, I may implemement it.

ngraham accepted this revision.Aug 29 2019, 4:49 PM

Much better. The UI and functionality look great now.

I'll hand this over to the Plasma people for code review now.

This revision is now accepted and ready to land.Aug 29 2019, 4:49 PM
This comment was removed by bcooksley.
bcooksley removed a subscriber: fuksitter.
alexde updated this revision to Diff 65320.Sep 3 2019, 1:54 PM
  • Set valid defaults.
alexde updated this revision to Diff 65321.Sep 3 2019, 1:55 PM

Set valid defaults.

Plasma people, could we get a code review on this?

alexde updated this revision to Diff 65407.Sep 5 2019, 8:04 AM

Align grid to 10 minutes for the 1h graph.

alexde updated this revision to Diff 65409.Sep 5 2019, 9:28 AM

Use dashed grid lines instead of solid lines. Grid lines with tick labels are less faint.
Rename variable to make its function clearer.
Show grid lines at all five minutes for the 1 hour graph.

alexde updated this revision to Diff 65410.Sep 5 2019, 10:24 AM

Fix issue that a date may be displayed unintentionally.
Shrink right margin for grid lines and try to add an additional grid line
on the right side if reasonable.
Add more divisions for 2h and 24h graphs.

alexde updated this revision to Diff 65413.Sep 5 2019, 11:09 AM

Rename variables to make their function clearer.
Decrease the lower loop border by 1 to make sure that all
possible grid lines are drawn.

alexde requested review of this revision.Sep 5 2019, 11:12 AM
alexde edited the test plan for this revision. (Show Details)

I have changed some stuff, can you please review it again.

For some reason the xGridOffset seems sometimes to be wrong, and I have no explanation why. It especially happens for 1 hour graphs.

alexde updated this revision to Diff 65423.Sep 5 2019, 12:50 PM

Shrink plotWidth a little bit so the earliest date on the right side may fit.
Change the way the division widths are defined to hopefully make the code easier to understand
and to maintain.

alexde updated this revision to Diff 65428.Sep 5 2019, 1:12 PM

Move stroke() out of the loop.
Reduce line with by pixel.

alexde edited the test plan for this revision. (Show Details)Sep 5 2019, 1:15 PM
alexde updated this revision to Diff 65457.Sep 5 2019, 4:32 PM

Set correct right border for the grid lines and ticks.

*Friendly ping to the reviewers.*

meven added a subscriber: meven.Sep 14 2019, 8:41 AM
meven added inline comments.
Modules/energy/package/contents/ui/Graph.qml
181

A switch statement should be clearer here.

alexde updated this revision to Diff 66031.Sep 14 2019, 10:14 AM

Use a switch statement for xTicksAt

alexde updated this revision to Diff 66034.Sep 14 2019, 10:18 AM

Remove redundant semicolons and move comment about unit (ms) to the declaration of the var

alexde updated this revision to Diff 66044.Sep 14 2019, 1:02 PM

The former scaling of the x-Positions of the data points did not fit to the time axis.
Now the x-values of the points, i. e. their timestamps, are correctly aligned.

For some unknown reasons, all data points, which come before/after a certain,
abitrary timestamp, seem to have a non-valid timestamp = 0.
That's why I have added a if-statement to ignore those values.

This feels more like a workaround and the underlying issue should be fixed instead in the future.

ngraham requested changes to this revision.Sep 14 2019, 2:24 PM

Hmm, now the X axis labels have disappeared for "Last hour", "Last 2 hours" and "Last 12 hours".

This revision now requires changes to proceed.Sep 14 2019, 2:24 PM
alexde updated this revision to Diff 66055.Sep 14 2019, 2:44 PM

Fix typo in variable and by that make all X axis labels appear again.

Thanks, that's fixed now. However the "Last hour" graph still looks a little odd...

alexde added a comment.EditedSep 14 2019, 3:04 PM

Thanks, that's fixed now. However the "Last hour" graph still looks a little odd...

Mm, I cannot reproduce this issue on my side and I cannot explain why you may experience it as the code behaves all the same of for all timespans. Very odd.

Edit: I can reproduce it, if the time is at xx-30 - xx:59. Going to investigate...

alexde updated this revision to Diff 66062.Sep 14 2019, 3:45 PM

Fix offset calculation for the one hour graph

alexde updated this revision to Diff 66063.Sep 14 2019, 4:11 PM

Remove redundant and now wrong comment

ngraham accepted this revision.Sep 14 2019, 7:19 PM
This revision is now accepted and ready to land.Sep 14 2019, 7:19 PM
ngraham edited the summary of this revision. (Show Details)Sep 16 2019, 5:05 PM
This revision was automatically updated to reflect the committed changes.