ubuntu-system-settings

Merge lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460 into lp:ubuntu-system-settings

battery-draw-x-text-1289460
Merge into trunk

Proposed by Sebastien Bacher on 2014-07-07

Status:

Merged

Approved by:

Iain Lane on 2014-07-09

Approved revision:

780

Merged at revision:

787

Proposed branch:

lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460

Merge into:

lp:ubuntu-system-settings

Diff against target:

130 lines (+59/-15)

1 file modified

plugins/battery/PageComponent.qml (+59/-15)

To merge this branch:

bzr merge lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2014-07-09
Iain Lane		2014-07-07	Approve on 2014-07-09
Review via email: mp+225856@code.launchpad.net

Commit message

battery, draw the x axis labels

Description of the change

battery, draw the x axis labels

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-07-07:

The code might look a bit complex, but turns out it's not trivial to write on a canvas. The results to be decent on desktop and phone configs, from my testing

Revision history for this message

Iain Lane (laney) wrote on 2014-07-07:

Thanks.

I think you could have avoided doing so many manipulations involving the margins if you had used a combination of translating and scaling to avoid them. But it's done now so don't bother changing.

- On N4, I made the "Today" label go off the end by setting the time to 01:53 (ish).
- Please wrap long lines.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/936/rebuild

review: Approve (continuous-integration)

lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460 updated on 2014-07-08

776. By Sebastien Bacher on 2014-07-08: correctly calculate the space for the text
777. By Sebastien Bacher on 2014-07-08: Wrap some long lines
778. By Sebastien Bacher on 2014-07-08: don't check for 0 value, we start at 1

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/944/rebuild

review: Approve (continuous-integration)

lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460 updated on 2014-07-08

779. By Sebastien Bacher on 2014-07-08: draw the first mark as well

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/945/rebuild

review: Approve (continuous-integration)

Revision history for this message

Iain Lane (laney) wrote on 2014-07-09:

I ran it (possibly with more data this time than last). There's a problem with the x coordinate for the previous day's data.

http://people.canonical.com/~laney/weird-things/battery.png

And do you know if it's possible to smooth the text? If you size that image to 100% you can see that the text is jagged and not very nice looking.

review: Needs Fixing

lp:~seb128/ubuntu-system-settings/battery-draw-x-text-1289460 updated on 2014-07-09

780. By Sebastien Bacher on 2014-07-09: revert change that seemed logical but is creating issues

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-07-09:

> I ran it (possibly with more data this time than last). There's a problem with the x coordinate for the previous day's data.
> http://people.canonical.com/~laney/weird-things/battery.png

Ok, I reverted the change that created that. The formular there looks weird but it works, we can fix in another change

> And do you know if it's possible to smooth the text? If you size that image to 100% you can see that the text is jagged and not very nice looking.

I didn't find any while looking in the canvas api, not sure what to do there ...

Revision history for this message

Iain Lane (laney) wrote on 2014-07-09:

Alright, looks fine for now.

I think the line drawing code is a bit suspicious but that's pre-existing.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-09:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/953/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Sebastien Bacher

Ubuntu Touch System Settings

 === modified file 'plugins/battery/PageComponent.qml'
 --- plugins/battery/PageComponent.qml	2014-01-27 18:12:16 +0000
 +++ plugins/battery/PageComponent.qml	2014-07-09 15:03:35 +0000
@@ -127,43 +127,84 @@
                  id: canvas
                  width:parent.width - units.gu(4)
                  anchors.horizontalCenter: parent.horizontalCenter
--                height: units.gu(15)
++                height: units.gu(23)
                  antialiasing: true
--                function drawAxes(ctx, axisWidth, axisHeight) {
++                function drawAxes(ctx, axisWidth, axisHeight, bottomMargin, rightMargin) {
++
++                    var currentHour = Qt.formatDateTime(new Date(), "h")
++                    var currentMinutes = Qt.formatDateTime(new Date(), "m")
++                    var displayHour
++                    var labelWidth
++                    var zeroMark
++
                      ctx.save()
                      ctx.beginPath()
                      ctx.strokeStyle = UbuntuColors.lightAubergine
                      ctx.lineWidth = units.dp(2)
++                    var fontHeight = FontUtils.sizeToPixels("small")
++                    ctx.font="%1px Ubuntu".arg(fontHeight)
++
                      ctx.translate(0, 1)
                      // 11 ticks with 0, 5, 10 being big
                      for (var i = 0; i <= 10; i++) {
                          var x = (i % 5 == 0) ? 0 : Math.floor(axisWidth / 2)
--                        var y = (i / 10) * (height - axisHeight - ctx.lineWidth)
++                        var y = (i / 10) * (height - axisHeight - bottomMargin - ctx.lineWidth)
                          ctx.moveTo(x, y)
                          ctx.lineTo(axisWidth, y)
+                     }
                      ctx.translate(axisWidth + ctx.lineWidth / 2,
--                                  height - axisHeight - ctx.lineWidth / 2)
++                                  height - axisHeight - bottomMargin - ctx.lineWidth / 2)
                      ctx.moveTo(0, 0)
                      ctx.lineTo(0, -ctx.lineWidth)
--                    // 25 ticks with 0, 6, 12, 18, 24 being big
++                    // 24 ticks with 6, 12, 18, 24 being big
                      for (i = 0; i <= 24; i++) {
--                        x = (i / 24) * (width - axisWidth - ctx.lineWidth)
++                        /* the marks need to be shifted on the hours */
++                        x = ((i - currentMinutes / 60) / 24) * (width - axisWidth - ctx.lineWidth - rightMargin)
++                        if (x < 0)
++                            continue
                          y = (i % 6 == 0) ? axisHeight : axisHeight -
                                              Math.floor(axisHeight / 2)
                          ctx.moveTo(x, 0)
                          ctx.lineTo(x, y)
++
++                        /* Determine the hour to display */
++                        displayHour = (currentHour - (24-i))
++                        if (displayHour < 0)
++                            displayHour = displayHour + 24
++                        /* Store the x for the day change line */
++                        if (displayHour === 0)
++                            zeroMark = x
++
++                        /* Write the x-axis legend */
++                        if (i % 6 == 0) {
++                            labelWidth = context.measureText("%1".arg(displayHour)).width;
++                            ctx.fillText("%1".arg(displayHour),
++                                         x - labelWidth/2,
++                                         axisHeight + units.dp(1) + fontHeight)
++                        }
+                     }
--                    ctx.translate(0, 0)
++                    labelWidth = context.measureText(i18n.tr("Yesterday")).width;
++                    if(labelWidth < zeroMark)
++                        ctx.fillText(i18n.tr("Yesterday"),
++                                     (zeroMark - labelWidth)/2,
++                                     axisHeight + units.dp(6) + 2*fontHeight)
++
++                    ctx.fillText("|", zeroMark, axisHeight + units.dp(6) + 2*fontHeight)
++
++                    labelWidth = context.measureText(i18n.tr("Today")).width;
++                    if(labelWidth < (width - zeroMark - rightMargin - axisWidth - ctx.lineWidth))
++                        ctx.fillText(i18n.tr("Today"),
++                                     zeroMark + (width - zeroMark - labelWidth)/2,
++                                     axisHeight + units.dp(6) + 2*fontHeight)
                      ctx.stroke()
                      ctx.restore()
@@ -174,12 +215,14 @@
                      ctx.save();
                      ctx.clearRect(0, 0, canvas.width, canvas.height)
--                    var axisWidth = 10
--                    var axisHeight = 10
--
--                    var graphHeight = height - axisHeight
--
--                    drawAxes(ctx, axisWidth, axisHeight)
++                    var axisWidth = units.gu(1)
++                    var axisHeight = units.gu(1)
++
++                    /* Space to write the legend */
++                    var bottomMargin = units.gu(6)
++                    var rightMargin = units.gu(1)
++
++                    drawAxes(ctx, axisWidth, axisHeight, bottomMargin, rightMargin)
                      /* Display the charge history */
                      ctx.beginPath();
@@ -191,10 +234,11 @@
                      // Invert the y axis so we draw from the bottom left
                      ctx.scale(1, -1)
                      // Move the origin to just above the axes
--                    ctx.translate(axisWidth, axisHeight)
++                    ctx.translate(axisWidth, axisHeight + bottomMargin)
                      // Scale to avoid the axes so we can draw as if they aren't
                      // there
--                    ctx.scale(1 - (axisWidth / width), 1 - axisHeight / height)
++                    ctx.scale(1 - ((axisWidth + rightMargin) / width),
++                              1 - (axisHeight + bottomMargin) / height)
                      var gradient = ctx.createLinearGradient(0, 0, 0, height);
                      gradient.addColorStop(1, "green");