Merge lp:~stefan-schwarzburg/qreator/touch_qrcodesize into lp:qreator/touch

Proposed by Schwarzburg on 2013-05-21
Status: Merged
Approved by: David Planella on 2013-05-24
Approved revision: 16
Merged at revision: 14
Proposed branch: lp:~stefan-schwarzburg/qreator/touch_qrcodesize
Merge into: lp:qreator/touch
Diff against target: 201 lines (+182/-2)
1 file modified
js/qrcode.js (+182/-2)
To merge this branch: bzr merge lp:~stefan-schwarzburg/qreator/touch_qrcodesize
Reviewer Review Type Date Requested Status
David Planella 2013-05-21 Approve on 2013-05-22
Review via email: mp+164960@code.launchpad.net

Description of the change

Hi David,
Since you did so much great work on releasing the python based version, we are now at a point were we can continue with the qml version, I guess.

So: this is the first change I would like to input.

It is an hopefully uncontroversionl change, it extends the possible qrcode size of the javascript library to version 40 (previously the max size was version 10). The only reason why we might not want this here is that it could also go into the upstream version. However, there will be other and deeper changes to the qrcode.js library in the future, which will simplify calculating the best size before creating the code... So I think qreator is the right place for this patch.

To post a comment you must log in.
David Planella (dpm) wrote :

Thanks Stefan, indeed, time to start the fun with the qml version ;)

The change looks good to me. However, may I ask you to fix just one thing? The modification includes 2 changes:

1) Addition of the new lookup tables from 11 to 40
2) Whitespace change from tabs to spaces under RS_BLOCK_TABLE

The reason I'm asking, apart from nitpicking (sorry) is because this way the change is much more easy to read. I assume you haven't changed the arrays from 1 to 10, but that's not obvious from the diff, as it's quite a long list of values to check one by one. Also, although I prefer spaces than tabs myself, for consistency's sake we should stick to the whitespace used originally rather than mixing spaces and tabs.

It will also help if upstream wants to cherrypick this one change but not more intrusive ones.

Thanks!

review: Needs Fixing
15. By Schwarzburg on 2013-05-22

replaced the whitespace changes to mach the upstream indentation.

16. By Schwarzburg on 2013-05-22

replaced the last remaining line with 4 spaces instead of a tab to match the upstream indentation.

Schwarzburg (stefan-schwarzburg) wrote :

OK, I think I got all the lines with 4 spaces instead of tabs :-)

Sorry for your extra work on my merge requests, I will try to pay more attention to details...

David Planella (dpm) wrote :

No worries, thank you for the work and the fixes!

Ok, this looks good to me now. I must admit I haven't checked the values in the arrays, but I trust you :)

Also, before you merge into trunk, just two things:

1) Can you add an empty line after this line:

18 + [6, 43, 15, 2, 44, 16],

This way the comment is separated from the previous array as in the rest of the file.

2) Can you add a brief note on the commit message about how you calculated the values in the arrays?

Thanks!

review: Approve
17. By Schwarzburg on 2013-05-25

Changed more whitespace to fit the original style.

Note on the values in RS_BLOCK_TABLE table:

The table has 4 entries per 'version' (in the range 1 - 40, corresponding to
different sizes of the qr code).
Each entry correspsonds to a different error corection level (L, M, Q, H).
In each entry, there are sets of 3 numbers:
number-of-error-correction-blocks,total-number-of-codewords,number-of-data-codewords

While it is certainly possible to calculate these numbers, I have not done so, but instead
I have consulted the ISO standard: the values are noted in table 9 of the ISO/IEC 18004:2006
document in the last two columns.

18. By Schwarzburg on 2013-05-25

more whitespace changes... (I need to switch of some auto-indentation and auto-whitespace features in emacs... Sorry for the noise)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'js/qrcode.js'
2--- js/qrcode.js 2013-02-08 03:16:44 +0000
3+++ js/qrcode.js 2013-05-25 09:00:37 +0000
4@@ -23,7 +23,7 @@
5
6 /**
7 * qrcode
8- * @param typeNumber 1 to 10
9+ * @param typeNumber 1 to 40
10 * @param errorCorrectLevel 'L','M','Q','H'
11 */
12 var qrcode = function(typeNumber, errorCorrectLevel) {
13@@ -1048,7 +1048,187 @@
14 [2, 86, 68, 2, 87, 69],
15 [4, 69, 43, 1, 70, 44],
16 [6, 43, 19, 2, 44, 20],
17- [6, 43, 15, 2, 44, 16]
18+ [6, 43, 15, 2, 44, 16],
19+
20+ // 11
21+ [4, 101, 81],
22+ [1, 80, 50, 4, 81, 51],
23+ [4, 50, 22, 4, 51, 23],
24+ [3, 36, 12, 8, 37, 13],
25+
26+ // 12
27+ [2, 116, 92, 2, 117, 93],
28+ [6, 58, 36, 2, 59, 37],
29+ [4, 46, 20, 6, 47, 21],
30+ [7, 42, 14, 4, 43, 15],
31+
32+ // 13
33+ [4, 133, 107],
34+ [8, 59, 37, 1, 60, 38],
35+ [8, 44, 20, 4, 45, 21],
36+ [12, 33, 11, 4, 34, 12],
37+
38+ // 14
39+ [3, 145, 115, 1, 146, 116],
40+ [4, 64, 40, 5, 65, 41],
41+ [11, 36, 16, 5, 37, 17],
42+ [11, 36, 12, 5, 37, 13],
43+
44+ // 15
45+ [5, 109, 87, 1, 110, 88],
46+ [5, 65, 41, 5, 66, 42],
47+ [5, 54, 24, 7, 55, 25],
48+ [11, 36, 12],
49+
50+ // 16
51+ [5, 122, 98, 1, 123, 99],
52+ [7, 73, 45, 3, 74, 46],
53+ [15, 43, 19, 2, 44, 20],
54+ [3, 45, 15, 13, 46, 16],
55+
56+ // 17
57+ [1, 135, 107, 5, 136, 108],
58+ [10, 74, 46, 1, 75, 47],
59+ [1, 50, 22, 15, 51, 23],
60+ [2, 42, 14, 17, 43, 15],
61+
62+ // 18
63+ [5, 150, 120, 1, 151, 121],
64+ [9, 69, 43, 4, 70, 44],
65+ [17, 50, 22, 1, 51, 23],
66+ [2, 42, 14, 19, 43, 15],
67+
68+ // 19
69+ [3, 141, 113, 4, 142, 114],
70+ [3, 70, 44, 11, 71, 45],
71+ [17, 47, 21, 4, 48, 22],
72+ [9, 39, 13, 16, 40, 14],
73+
74+ // 20
75+ [3, 135, 107, 5, 136, 108],
76+ [3, 67, 41, 13, 68, 42],
77+ [15, 54, 24, 5, 55, 25],
78+ [15, 43, 15, 10, 44, 16],
79+
80+ // 21
81+ [4, 144, 116, 4, 145, 117],
82+ [17, 68, 42],
83+ [17, 50, 22, 6, 51, 23],
84+ [19, 46, 16, 6, 47, 17],
85+
86+ // 22
87+ [2, 139, 111, 7, 140, 112],
88+ [17, 74, 46],
89+ [7, 54, 24, 16, 55, 25],
90+ [34, 37, 13],
91+
92+ // 23
93+ [4, 151, 121, 5, 152, 122],
94+ [4, 75, 47, 14, 76, 48],
95+ [11, 54, 24, 14, 55, 25],
96+ [16, 45, 15, 14, 46, 16],
97+
98+ // 24
99+ [6, 147, 117, 4, 148, 118],
100+ [6, 73, 45, 14, 74, 46],
101+ [11, 54, 24, 16, 55, 25],
102+ [30, 46, 16, 2, 47, 17],
103+
104+ // 25
105+ [8, 132, 106, 4, 133, 107],
106+ [8, 75, 47, 13, 76, 48],
107+ [7, 54, 24, 22, 55, 25],
108+ [22, 45, 15, 13, 46, 16],
109+
110+ // 26
111+ [10, 142, 114, 2, 143, 115],
112+ [19, 74, 46, 4, 75, 47],
113+ [28, 50, 22, 6, 51, 23],
114+ [33, 46, 16, 4, 47, 17],
115+
116+ // 27
117+ [8, 152, 122, 4, 153, 123],
118+ [22, 73, 45, 3, 74, 46],
119+ [8, 53, 23, 26, 54, 24],
120+ [12, 45, 15, 28, 46, 16],
121+
122+ // 28
123+ [3, 147, 117, 10, 148, 118],
124+ [3, 73, 45, 23, 74, 46],
125+ [4, 54, 24, 31, 55, 25],
126+ [11, 45, 15, 31, 46, 16],
127+
128+ // 29
129+ [7, 146, 116, 7, 147, 117],
130+ [21, 73, 45, 7, 74, 46],
131+ [1, 53, 23, 37, 54, 24],
132+ [19, 45, 15, 26, 46, 16],
133+
134+ // 30
135+ [5, 145, 115, 10, 146, 116],
136+ [19, 75, 47, 10, 76, 48],
137+ [15, 54, 24, 25, 55, 25],
138+ [23, 45, 15, 25, 46, 16],
139+
140+ // 31
141+ [13, 145, 115, 3, 146, 116],
142+ [2, 74, 46, 29, 75, 47],
143+ [42, 54, 24, 1, 55, 25],
144+ [23, 45, 15, 28, 46, 16],
145+
146+ // 32
147+ [17, 145, 115],
148+ [10, 74, 46, 23, 75, 47],
149+ [10, 54, 24, 35, 55, 25],
150+ [19, 45, 15, 35, 46, 16],
151+
152+ // 33
153+ [17, 145, 115, 1, 146, 116],
154+ [14, 74, 46, 21, 75, 47],
155+ [29, 54, 24, 19, 55, 25],
156+ [11, 45, 15, 46, 46, 16],
157+
158+ // 34
159+ [13, 145, 115, 6, 146, 116],
160+ [14, 74, 46, 23, 75, 47],
161+ [44, 54, 24, 7, 55, 25],
162+ [59, 46, 16, 1, 47, 17],
163+
164+ // 35
165+ [12, 151, 121, 7, 152, 122],
166+ [12, 75, 47, 26, 76, 48],
167+ [39, 54, 24, 14, 55, 25],
168+ [22, 45, 15, 41, 46, 16],
169+
170+ // 36
171+ [6, 151, 121, 14, 152, 122],
172+ [6, 75, 47, 34, 76, 48],
173+ [46, 54, 24, 10, 55, 25],
174+ [2, 45, 15, 64, 46, 16],
175+
176+ // 37
177+ [17, 152, 122, 4, 153, 123],
178+ [29, 74, 46, 14, 75, 47],
179+ [49, 54, 24, 10, 55, 25],
180+ [24, 45, 15, 46, 46, 16],
181+
182+ // 38
183+ [4, 152, 122, 18, 153, 123],
184+ [13, 74, 46, 32, 75, 47],
185+ [48, 54, 24, 14, 55, 25],
186+ [42, 45, 15, 32, 46, 16],
187+
188+ // 39
189+ [20, 147, 117, 4, 148, 118],
190+ [40, 75, 47, 7, 76, 48],
191+ [43, 54, 24, 22, 55, 25],
192+ [10, 45, 15, 67, 46, 16],
193+
194+ // 40
195+ [19, 148, 118, 6, 149, 119],
196+ [18, 75, 47, 31, 76, 48],
197+ [34, 54, 24, 34, 55, 25],
198+ [20, 45, 15, 61, 46, 16]
199 ];
200
201 var qrRSBlock = function(totalCount, dataCount) {

Subscribers

People subscribed via source and target branches

to all changes: