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
=== modified file 'js/qrcode.js'
--- js/qrcode.js 2013-02-08 03:16:44 +0000
+++ js/qrcode.js 2013-05-25 09:00:37 +0000
@@ -23,7 +23,7 @@
2323
24 /**24 /**
25 * qrcode25 * qrcode
26 * @param typeNumber 1 to 1026 * @param typeNumber 1 to 40
27 * @param errorCorrectLevel 'L','M','Q','H'27 * @param errorCorrectLevel 'L','M','Q','H'
28 */28 */
29 var qrcode = function(typeNumber, errorCorrectLevel) {29 var qrcode = function(typeNumber, errorCorrectLevel) {
@@ -1048,7 +1048,187 @@
1048 [2, 86, 68, 2, 87, 69],1048 [2, 86, 68, 2, 87, 69],
1049 [4, 69, 43, 1, 70, 44],1049 [4, 69, 43, 1, 70, 44],
1050 [6, 43, 19, 2, 44, 20],1050 [6, 43, 19, 2, 44, 20],
1051 [6, 43, 15, 2, 44, 16]1051 [6, 43, 15, 2, 44, 16],
1052
1053 // 11
1054 [4, 101, 81],
1055 [1, 80, 50, 4, 81, 51],
1056 [4, 50, 22, 4, 51, 23],
1057 [3, 36, 12, 8, 37, 13],
1058
1059 // 12
1060 [2, 116, 92, 2, 117, 93],
1061 [6, 58, 36, 2, 59, 37],
1062 [4, 46, 20, 6, 47, 21],
1063 [7, 42, 14, 4, 43, 15],
1064
1065 // 13
1066 [4, 133, 107],
1067 [8, 59, 37, 1, 60, 38],
1068 [8, 44, 20, 4, 45, 21],
1069 [12, 33, 11, 4, 34, 12],
1070
1071 // 14
1072 [3, 145, 115, 1, 146, 116],
1073 [4, 64, 40, 5, 65, 41],
1074 [11, 36, 16, 5, 37, 17],
1075 [11, 36, 12, 5, 37, 13],
1076
1077 // 15
1078 [5, 109, 87, 1, 110, 88],
1079 [5, 65, 41, 5, 66, 42],
1080 [5, 54, 24, 7, 55, 25],
1081 [11, 36, 12],
1082
1083 // 16
1084 [5, 122, 98, 1, 123, 99],
1085 [7, 73, 45, 3, 74, 46],
1086 [15, 43, 19, 2, 44, 20],
1087 [3, 45, 15, 13, 46, 16],
1088
1089 // 17
1090 [1, 135, 107, 5, 136, 108],
1091 [10, 74, 46, 1, 75, 47],
1092 [1, 50, 22, 15, 51, 23],
1093 [2, 42, 14, 17, 43, 15],
1094
1095 // 18
1096 [5, 150, 120, 1, 151, 121],
1097 [9, 69, 43, 4, 70, 44],
1098 [17, 50, 22, 1, 51, 23],
1099 [2, 42, 14, 19, 43, 15],
1100
1101 // 19
1102 [3, 141, 113, 4, 142, 114],
1103 [3, 70, 44, 11, 71, 45],
1104 [17, 47, 21, 4, 48, 22],
1105 [9, 39, 13, 16, 40, 14],
1106
1107 // 20
1108 [3, 135, 107, 5, 136, 108],
1109 [3, 67, 41, 13, 68, 42],
1110 [15, 54, 24, 5, 55, 25],
1111 [15, 43, 15, 10, 44, 16],
1112
1113 // 21
1114 [4, 144, 116, 4, 145, 117],
1115 [17, 68, 42],
1116 [17, 50, 22, 6, 51, 23],
1117 [19, 46, 16, 6, 47, 17],
1118
1119 // 22
1120 [2, 139, 111, 7, 140, 112],
1121 [17, 74, 46],
1122 [7, 54, 24, 16, 55, 25],
1123 [34, 37, 13],
1124
1125 // 23
1126 [4, 151, 121, 5, 152, 122],
1127 [4, 75, 47, 14, 76, 48],
1128 [11, 54, 24, 14, 55, 25],
1129 [16, 45, 15, 14, 46, 16],
1130
1131 // 24
1132 [6, 147, 117, 4, 148, 118],
1133 [6, 73, 45, 14, 74, 46],
1134 [11, 54, 24, 16, 55, 25],
1135 [30, 46, 16, 2, 47, 17],
1136
1137 // 25
1138 [8, 132, 106, 4, 133, 107],
1139 [8, 75, 47, 13, 76, 48],
1140 [7, 54, 24, 22, 55, 25],
1141 [22, 45, 15, 13, 46, 16],
1142
1143 // 26
1144 [10, 142, 114, 2, 143, 115],
1145 [19, 74, 46, 4, 75, 47],
1146 [28, 50, 22, 6, 51, 23],
1147 [33, 46, 16, 4, 47, 17],
1148
1149 // 27
1150 [8, 152, 122, 4, 153, 123],
1151 [22, 73, 45, 3, 74, 46],
1152 [8, 53, 23, 26, 54, 24],
1153 [12, 45, 15, 28, 46, 16],
1154
1155 // 28
1156 [3, 147, 117, 10, 148, 118],
1157 [3, 73, 45, 23, 74, 46],
1158 [4, 54, 24, 31, 55, 25],
1159 [11, 45, 15, 31, 46, 16],
1160
1161 // 29
1162 [7, 146, 116, 7, 147, 117],
1163 [21, 73, 45, 7, 74, 46],
1164 [1, 53, 23, 37, 54, 24],
1165 [19, 45, 15, 26, 46, 16],
1166
1167 // 30
1168 [5, 145, 115, 10, 146, 116],
1169 [19, 75, 47, 10, 76, 48],
1170 [15, 54, 24, 25, 55, 25],
1171 [23, 45, 15, 25, 46, 16],
1172
1173 // 31
1174 [13, 145, 115, 3, 146, 116],
1175 [2, 74, 46, 29, 75, 47],
1176 [42, 54, 24, 1, 55, 25],
1177 [23, 45, 15, 28, 46, 16],
1178
1179 // 32
1180 [17, 145, 115],
1181 [10, 74, 46, 23, 75, 47],
1182 [10, 54, 24, 35, 55, 25],
1183 [19, 45, 15, 35, 46, 16],
1184
1185 // 33
1186 [17, 145, 115, 1, 146, 116],
1187 [14, 74, 46, 21, 75, 47],
1188 [29, 54, 24, 19, 55, 25],
1189 [11, 45, 15, 46, 46, 16],
1190
1191 // 34
1192 [13, 145, 115, 6, 146, 116],
1193 [14, 74, 46, 23, 75, 47],
1194 [44, 54, 24, 7, 55, 25],
1195 [59, 46, 16, 1, 47, 17],
1196
1197 // 35
1198 [12, 151, 121, 7, 152, 122],
1199 [12, 75, 47, 26, 76, 48],
1200 [39, 54, 24, 14, 55, 25],
1201 [22, 45, 15, 41, 46, 16],
1202
1203 // 36
1204 [6, 151, 121, 14, 152, 122],
1205 [6, 75, 47, 34, 76, 48],
1206 [46, 54, 24, 10, 55, 25],
1207 [2, 45, 15, 64, 46, 16],
1208
1209 // 37
1210 [17, 152, 122, 4, 153, 123],
1211 [29, 74, 46, 14, 75, 47],
1212 [49, 54, 24, 10, 55, 25],
1213 [24, 45, 15, 46, 46, 16],
1214
1215 // 38
1216 [4, 152, 122, 18, 153, 123],
1217 [13, 74, 46, 32, 75, 47],
1218 [48, 54, 24, 14, 55, 25],
1219 [42, 45, 15, 32, 46, 16],
1220
1221 // 39
1222 [20, 147, 117, 4, 148, 118],
1223 [40, 75, 47, 7, 76, 48],
1224 [43, 54, 24, 22, 55, 25],
1225 [10, 45, 15, 67, 46, 16],
1226
1227 // 40
1228 [19, 148, 118, 6, 149, 119],
1229 [18, 75, 47, 31, 76, 48],
1230 [34, 54, 24, 34, 55, 25],
1231 [20, 45, 15, 61, 46, 16]
1052 ];1232 ];
10531233
1054 var qrRSBlock = function(totalCount, dataCount) {1234 var qrRSBlock = function(totalCount, dataCount) {

Subscribers

People subscribed via source and target branches

to all changes: