Click Update Manager

Merge lp:~diegosarmentero/click-update-manager/bytes-to-size into lp:click-update-manager

  1. bytes-to-size
  2. Merge into trunk
Proposed by Diego Sarmentero on 2013-10-09
Status: Merged
Approved by: Diego Sarmentero on 2013-10-16
Approved revision: 44
Merged at revision: 36
Proposed branch: lp:~diegosarmentero/click-update-manager/bytes-to-size
Merge into: lp:click-update-manager
Diff against target: 105 lines (+47/-2)
5 files modified
CMakeLists.txt (+1/-0)
Components/PageUpdate.qml (+2/-1)
js/utils.js (+39/-0)
tests/unit/tst_pageupdate.qml (+1/-1)
updatemanager.qmlproject (+4/-0)
To merge this branch: bzr merge lp:~diegosarmentero/click-update-manager/bytes-to-size
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2013-10-16
Mike McCracken (community) 2013-10-09 Approve on 2013-10-16
dobey (community) Approve on 2013-10-16
Roberto Alsina (community) Approve on 2013-10-16
Review via email: mp+190149@code.launchpad.net

Commit message

- Show app size in the proper way (BUG: #1232256).

To post a comment you must log in.
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-09
37. By Diego Sarmentero on 2013-10-09

fix if comparison

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Mike McCracken (mikemc) wrote :

In utils.js, the calculation "parseFloat(bytes / 1024).toFixed(2)" is strange.
per IRC, bytes will be an int, so we're dividing two ints then apparently implicitly converting them to strings before passing them to parseFloat(), which generates a float.

If you just remove "parseFloat" and divide by a float, it works the same but makes more sense to me.

like this:

var size = (bytes / 1024.0).toFixed(2);

review: Needs Fixing
Mike McCracken (mikemc) wrote :

http://paste.ubuntu.com/6214313/ is the QML I was using to poke at the convert_bytes_to_size function

lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-11
38. By Diego Sarmentero on 2013-10-11

removing parseFloat

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-14
39. By Diego Sarmentero on 2013-10-14

update cmake

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-15
40. By Diego Sarmentero on 2013-10-15

revert desktop file name

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-16
41. By Diego Sarmentero on 2013-10-16

adding desktop file

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Roberto Alsina (ralsina) wrote :

Asked for GiB to be added just in case.

review: Approve
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-16
42. By Diego Sarmentero on 2013-10-16

adding GiB to convert function

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
dobey (dobey) wrote :

> === added file 'click-update-manager.desktop'
> === removed file 'click-update-manager.desktop'

Huh? How can it be both added and removed in the same diff? And which one is correct?

82 +function convert_bytes_to_size(bytes) {
83 + var result = "";
84 + var size = (bytes / 1024.0).toFixed(2);
85 + if(size < 1024) {
86 + result = size + " KiB";
87 + } else if(size < 1048576) {
88 + size = (size / 1024.0).toFixed(2);
89 + result = size + " MiB";
90 + } else {
91 + size = (size / 1048576.0).toFixed(2);
92 + result = size + " GiB";
93 + }
94 +
95 + return result;
96 +}

This code is really hard to read, because the checks are inconsistent, and you're converting bytes to KiB from the start, but use "size" as a variable name. Why not just use the input "bytes" variable across the board for checking, and only place the reduced size in the result string? Something like this perhaps:

var SIZE_IN_GIB = 1024.0 * 1024.0 * 1024.0;
var SIZE_IN_MIB = 1024.0 * 1024.0;
var SIZE_IN_KIB = 1024.0;

var result = "";
var size = 0;
if (bytes / SIZE_IN_KIB < SIZE_IN_KIB) {
    size = bytes / SIZE_IN_KIB;
    result = size + " KiB";
} else if (bytes / SIZE_IN_MIB < SIZE_IN_KIB) {
    size = bytes / SIZE_IN_MIB;
    result = size + " MiB";
} else if (bytes / SIZE_IN_GIB < SIZE_IN_KIB) {
    size = bytes / SIZE_IN_GIB;
    result = size + " GiB";
} else {
    result = bytes + " bytes";
}

return result;

review: Needs Fixing
lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-16
43. By Diego Sarmentero on 2013-10-16

improve convert function

Diego Sarmentero (diegosarmentero) wrote :

> > === added file 'click-update-manager.desktop'
> > === removed file 'click-update-manager.desktop'
>
> Huh? How can it be both added and removed in the same diff? And which one is
> correct?
>
> 82 +function convert_bytes_to_size(bytes) {
> 83 + var result = "";
> 84 + var size = (bytes / 1024.0).toFixed(2);
> 85 + if(size < 1024) {
> 86 + result = size + " KiB";
> 87 + } else if(size < 1048576) {
> 88 + size = (size / 1024.0).toFixed(2);
> 89 + result = size + " MiB";
> 90 + } else {
> 91 + size = (size / 1048576.0).toFixed(2);
> 92 + result = size + " GiB";
> 93 + }
> 94 +
> 95 + return result;
> 96 +}
>
> This code is really hard to read, because the checks are inconsistent, and
> you're converting bytes to KiB from the start, but use "size" as a variable
> name. Why not just use the input "bytes" variable across the board for
> checking, and only place the reduced size in the result string? Something like
> this perhaps:
>
> var SIZE_IN_GIB = 1024.0 * 1024.0 * 1024.0;
> var SIZE_IN_MIB = 1024.0 * 1024.0;
> var SIZE_IN_KIB = 1024.0;
>
> var result = "";
> var size = 0;
> if (bytes / SIZE_IN_KIB < SIZE_IN_KIB) {
> size = bytes / SIZE_IN_KIB;
> result = size + " KiB";
> } else if (bytes / SIZE_IN_MIB < SIZE_IN_KIB) {
> size = bytes / SIZE_IN_MIB;
> result = size + " MiB";
> } else if (bytes / SIZE_IN_GIB < SIZE_IN_KIB) {
> size = bytes / SIZE_IN_GIB;
> result = size + " GiB";
> } else {
> result = bytes + " bytes";
> }
>
> return result;

Fixed

lp:~diegosarmentero/click-update-manager/bytes-to-size updated on 2013-10-16
44. By Diego Sarmentero on 2013-10-16

revert renaming desktop file

dobey (dobey) :
review: Approve
Mike McCracken (mikemc) :
review: Approve
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2013-10-03 02:35:10 +0000
3+++ CMakeLists.txt 2013-10-16 18:03:12 +0000
4@@ -8,6 +8,7 @@
5 file(GLOB QML_JS_FILES *.qml)
6 install(FILES ${QML_JS_FILES} DESTINATION ${CLICK_UPDATE_MANAGER_DIR})
7 install(DIRECTORY Components DESTINATION ${CLICK_UPDATE_MANAGER_DIR})
8+install(DIRECTORY js DESTINATION ${CLICK_UPDATE_MANAGER_DIR})
9
10 install(FILES click-update-manager.desktop DESTINATION ${CMAKE_INSTALL_DATADIR}/applications)
11
12
13=== modified file 'Components/PageUpdate.qml'
14--- Components/PageUpdate.qml 2013-10-08 13:15:38 +0000
15+++ Components/PageUpdate.qml 2013-10-16 18:03:12 +0000
16@@ -18,6 +18,7 @@
17 import Ubuntu.Components 0.1
18 import Ubuntu.Components.ListItems 0.1 as ListItem
19 import com.ubuntu.click 0.1
20+import "../js/utils.js" as Utils
21
22 Page {
23 id: root
24@@ -160,7 +161,7 @@
25 }
26 }
27 Label {
28- text: parseFloat((modelData.binaryFilesize / 1024) / 1024).toFixed(2) + " Mb"
29+ text: Utils.convert_bytes_to_size(modelData.binaryFilesize)
30 height: textArea.height / 2
31 anchors.right: parent.right
32 }
33
34=== added directory 'js'
35=== added file 'js/utils.js'
36--- js/utils.js 1970-01-01 00:00:00 +0000
37+++ js/utils.js 2013-10-16 18:03:12 +0000
38@@ -0,0 +1,39 @@
39+/*
40+ * Copyright (C) 2013 Canonical, Ltd.
41+ *
42+ * This program is free software; you can redistribute it and/or modify
43+ * it under the terms of the GNU General Public License as published by
44+ * the Free Software Foundation; version 3.
45+ *
46+ * This program is distributed in the hope that it will be useful,
47+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
48+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
49+ * GNU General Public License for more details.
50+ *
51+ * You should have received a copy of the GNU General Public License
52+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
53+ */
54+.pragma library
55+
56+function convert_bytes_to_size(bytes) {
57+ var SIZE_IN_GIB = 1024.0 * 1024.0 * 1024.0;
58+ var SIZE_IN_MIB = 1024.0 * 1024.0;
59+ var SIZE_IN_KIB = 1024.0;
60+
61+ var result = "";
62+ var size = 0;
63+ if (bytes / SIZE_IN_KIB < SIZE_IN_KIB) {
64+ size = (bytes / SIZE_IN_KIB).toFixed(2);
65+ result = size + " KiB";
66+ } else if (bytes / SIZE_IN_MIB < SIZE_IN_KIB) {
67+ size = (bytes / SIZE_IN_MIB).toFixed(2);
68+ result = size + " MiB";
69+ } else if (bytes / SIZE_IN_GIB < SIZE_IN_KIB) {
70+ size = (bytes / SIZE_IN_GIB).toFixed(2);
71+ result = size + " GiB";
72+ } else {
73+ result = bytes + " bytes";
74+ }
75+
76+ return result;
77+}
78
79=== modified file 'tests/unit/tst_pageupdate.qml'
80--- tests/unit/tst_pageupdate.qml 2013-10-08 13:15:38 +0000
81+++ tests/unit/tst_pageupdate.qml 2013-10-16 18:03:12 +0000
82@@ -154,7 +154,7 @@
83 compare(colLeft.children[1].visible, true, "Should be visible");
84 compare(colLeft.children[1].text, "Version: " + fakeModel[i].remoteVersion, "Remote versions don't match");
85 var colRight = UT.findChild(obj, "colRight");
86- var mbvalue = parseFloat((fakeModel[i].binaryFilesize / 1024) / 1024).toFixed(2) + " Mb";
87+ var mbvalue = "4.88 KiB";
88 compare(colRight.children[1].text, mbvalue, "Other Mb size expected");
89 }
90 }
91
92=== modified file 'updatemanager.qmlproject'
93--- updatemanager.qmlproject 2013-09-20 20:04:44 +0000
94+++ updatemanager.qmlproject 2013-10-16 18:03:12 +0000
95@@ -26,6 +26,10 @@
96 filter: "*"
97 }
98 Files {
99+ directory: "js/"
100+ filter: "*"
101+ }
102+ Files {
103 directory: "img/"
104 filter: "*"
105 }

Subscribers

People subscribed via source and target branches