Changeset View
Standalone View
src/QuickEditor/EditorRoot.qml
Show All 32 Lines | 25 | Item { | |||
---|---|---|---|---|---|
33 | property color strokeColour: systemPalette.highlight; | 33 | property color strokeColour: systemPalette.highlight; | ||
34 | property color crossColour: Qt.rgba(strokeColour.r, strokeColour.g, strokeColour.b, 0.7); | 34 | property color crossColour: Qt.rgba(strokeColour.r, strokeColour.g, strokeColour.b, 0.7); | ||
35 | property color labelBackgroundColour: Qt.rgba(systemPalette.light.r, systemPalette.light.g, systemPalette.light.b, 0.85); | 35 | property color labelBackgroundColour: Qt.rgba(systemPalette.light.r, systemPalette.light.g, systemPalette.light.b, 0.85); | ||
36 | property bool showMagnifier: false; | 36 | property bool showMagnifier: false; | ||
37 | property bool toggleMagnifier: false; | 37 | property bool toggleMagnifier: false; | ||
38 | property int magZoom: 5; | 38 | property int magZoom: 5; | ||
39 | property int magPixels: 16; | 39 | property int magPixels: 16; | ||
40 | property int magOffset: 32; | 40 | property int magOffset: 32; | ||
41 | property double largeChange: 15; | ||||
rkflx: That does not seem right, because this will result in slower movement on a HiDPI screen… | |||||
This was suggested by @abalaji based on his work on reworking the selection rectangle. Should it be removed? sharvey: This was suggested by @abalaji based on his work on reworking the selection rectangle. Should… | |||||
42 | property double smallChange: 1 / Screen.devicePixelRatio; | ||||
That looks suspicious: How will you be able to store the result accurately in an int? rkflx: That looks suspicious: How will you be able to store the result accurately in an `int`? | |||||
41 | 43 | | |||
42 | SystemPalette { | 44 | SystemPalette { | ||
43 | id: systemPalette; | 45 | id: systemPalette; | ||
44 | } | 46 | } | ||
45 | 47 | | |||
46 | function setInitialSelection(xx, yy, ww, hh) { | 48 | function setInitialSelection(xx, yy, ww, hh) { | ||
47 | if (selection) { | 49 | if (selection) { | ||
48 | selection.destroy(); | 50 | selection.destroy(); | ||
49 | } | 51 | } | ||
50 | 52 | | |||
51 | selection = cropRectangle.createObject(parent, { | 53 | selection = cropRectangle.createObject(parent, { | ||
52 | "x": xx, | 54 | "x": xx, | ||
53 | "y": yy, | 55 | "y": yy, | ||
54 | "height": hh, | 56 | "height": hh, | ||
55 | "width": ww | 57 | "width": ww | ||
56 | }); | 58 | }); | ||
rkflx: Unrelated whitespace change, please revert. | |||||
57 | 59 | | |||
58 | cropDisplayCanvas.requestPaint(); | 60 | cropDisplayCanvas.requestPaint(); | ||
59 | } | 61 | } | ||
60 | 62 | | |||
abalaji: Are `rightX` and `bottomY` used anywhere | |||||
Hm. Probably not. Probably seemed like a valid idea at one point. Scheduled for removal. sharvey: Hm. Probably not. Probably seemed like a valid idea at one point. Scheduled for removal. | |||||
61 | function accept() { | 63 | function accept() { | ||
62 | if (selection) { | 64 | if (selection) { | ||
63 | acceptImage(selection.x, selection.y, selection.width, selection.height); | 65 | acceptImage(selection.x, selection.y, selection.width, selection.height); | ||
64 | } else { | 66 | } else { | ||
65 | acceptImage(-1, -1, -1, -1); | 67 | acceptImage(-1, -1, -1, -1); | ||
66 | } | 68 | } | ||
67 | } | 69 | } | ||
68 | 70 | | |||
69 | // key handlers | 71 | // key handlers | ||
70 | 72 | | |||
71 | focus: true; | 73 | focus: true; | ||
72 | 74 | | |||
73 | Keys.onReturnPressed: accept() | 75 | Keys.onReturnPressed: accept() | ||
74 | Keys.onEnterPressed: accept() | 76 | Keys.onEnterPressed: accept() | ||
75 | 77 | | |||
76 | Keys.onEscapePressed: { | 78 | Keys.onEscapePressed: { | ||
77 | cancelImage(); | 79 | cancelImage(); | ||
78 | } | 80 | } | ||
79 | 81 | | |||
80 | Keys.onPressed: { | 82 | Keys.onPressed: { | ||
83 | | ||||
84 | var change; | ||||
Without the context of the patch in mind, this is a bit hard to understand what it is about when you stumble upon this while reading the code. Could you come up with a better name and/or comment? rkflx: Without the context of the patch in mind, this is a bit hard to understand what it is about… | |||||
85 | | ||||
86 | // shift key alone = magnifier toggle | ||||
81 | if (event.modifiers & Qt.ShiftModifier) { | 87 | if (event.modifiers & Qt.ShiftModifier) { | ||
82 | toggleMagnifier = true; | 88 | toggleMagnifier = true; | ||
89 | } | ||||
90 | | ||||
91 | // nested switches for arrow keys based on modifier keys | ||||
92 | switch(event.modifiers) { | ||||
rkflx: Insert space after `switch`, please. Repeat for all other occurrences. | |||||
Not done, but maybe I should have given you an example. This is how I meant it: switch (event.modifiers) { IOW, this should be consistent with how the spacing works for if. rkflx: Not done, but maybe I should have given you an example. This is how I meant it:
switch (event. | |||||
93 | | ||||
94 | case Qt.NoModifier: | ||||
95 | switch (event.key) { | ||||
96 | | ||||
97 | case Qt.Key_Left: | ||||
abalaji: Change `-1` to `-1 / Screen.devicePixelRatio` | |||||
98 | change = checkBounds(-largeChange, 0.0, "left"); | ||||
rkflx: Do you know about the `+=` operator, which would be more concise? | |||||
99 | selection.x += change; | ||||
100 | break; | ||||
101 | case Qt.Key_Right: | ||||
abalaji: Same | |||||
102 | change = checkBounds(largeChange, 0.0, "right"); | ||||
103 | selection.x += change; | ||||
104 | break; | ||||
105 | case Qt.Key_Up: | ||||
abalaji: Same | |||||
106 | change = checkBounds(0.0, -largeChange, "up"); | ||||
107 | selection.y += change; | ||||
108 | break; | ||||
109 | case Qt.Key_Down: | ||||
abalaji: Same | |||||
110 | change = checkBounds(0.0, largeChange, "down"); | ||||
111 | selection.y += change; | ||||
112 | break; | ||||
113 | } | ||||
114 | | ||||
115 | break; // end no modifier (just arrows) | ||||
116 | | ||||
117 | case Qt.ShiftModifier: | ||||
118 | switch (event.key) { | ||||
119 | | ||||
abalaji: Change `-largeChange` to `-largeChange / Screen.devicePixelRatio` | |||||
120 | case Qt.Key_Left: | ||||
121 | change = checkBounds(-smallChange, 0.0, "left"); | ||||
122 | selection.x += change; | ||||
123 | break; | ||||
124 | | ||||
abalaji: I think you get the idea | |||||
125 | case Qt.Key_Right: | ||||
126 | change = checkBounds(smallChange, 0.0, "right"); | ||||
127 | selection.x += change; | ||||
128 | break; | ||||
129 | | ||||
130 | case Qt.Key_Up: | ||||
131 | change = checkBounds(0.0, -smallChange, "up"); | ||||
132 | selection.y += change; | ||||
133 | break; | ||||
134 | | ||||
135 | case Qt.Key_Down: | ||||
136 | change = checkBounds(0.0, smallChange, "down"); | ||||
137 | selection.y += change; | ||||
This duplicates most of the code for Qt.NoModifier. Could you instead assign largeChange or smallChange to a variable based on the modifier, which you then pass to checkBounds? Also this is likely independent from whether you are moving or resizing (where all of this is duplicated again currently). rkflx: This duplicates most of the code for `Qt.NoModifier`. Could you instead assign `largeChange` or… | |||||
138 | break; | ||||
139 | } | ||||
140 | | ||||
141 | break; // end Shift + arrows (large move) | ||||
142 | | ||||
143 | case Qt.AltModifier: | ||||
144 | switch (event.key) { | ||||
145 | | ||||
146 | case Qt.Key_Left: | ||||
147 | change = checkBounds(-largeChange, 0.0, "left"); | ||||
148 | if (selection.width + change <= 0.0) { | ||||
149 | selection.width = 0.0; | ||||
150 | } else { | ||||
151 | selection.width += change; | ||||
152 | } | ||||
153 | break; | ||||
154 | | ||||
155 | case Qt.Key_Right: | ||||
156 | change = checkBounds(largeChange, 0.0, "right"); | ||||
157 | selection.width += change; | ||||
158 | break; | ||||
159 | | ||||
160 | case Qt.Key_Up: | ||||
161 | change = checkBounds(0.0, -largeChange, "up"); | ||||
162 | if (selection.height + change <= 0.0) { | ||||
163 | selection.height = 0.0; | ||||
164 | } else { | ||||
165 | selection.height += change; | ||||
166 | } | ||||
167 | break; | ||||
168 | | ||||
169 | case Qt.Key_Down: | ||||
rkflx: {key Alt down} is broken (should be fast, not slow) | |||||
170 | change = checkBounds(0.0, largeChange, "down"); | ||||
171 | selection.height = selection.height + change; | ||||
172 | break; | ||||
173 | } | ||||
rkflx: Insert space after `case`. | |||||
rkflx: Not yet done. | |||||
174 | | ||||
175 | break; // end ALT + arrows (resize rectangle) | ||||
176 | | ||||
177 | case (Qt.ShiftModifier + Qt.AltModifier): | ||||
178 | switch (event.key) { | ||||
179 | | ||||
180 | case Qt.Key_Left: | ||||
181 | change = checkBounds(-smallChange, 0.0, "left"); | ||||
182 | if (selection.width + change <= 0.0) { | ||||
183 | selection.width = 0.0; | ||||
184 | } else { | ||||
185 | selection.width += change; | ||||
186 | } | ||||
187 | break; | ||||
188 | | ||||
189 | case Qt.Key_Right: | ||||
190 | change = checkBounds(smallChange, 0.0, "right"); | ||||
191 | selection.width += change; | ||||
192 | break; | ||||
193 | | ||||
194 | case Qt.Key_Up: | ||||
195 | change = checkBounds(0.0, -smallChange, "up"); | ||||
196 | if (selection.height + change <= 0.0) { | ||||
197 | selection.height = 0.0; | ||||
198 | } else { | ||||
199 | selection.height += change | ||||
200 | } | ||||
201 | break; | ||||
202 | | ||||
203 | case Qt.Key_Down: | ||||
204 | change = checkBounds(0.0, smallChange, "down"); | ||||
205 | selection.height += change; | ||||
206 | break; | ||||
207 | } | ||||
208 | | ||||
209 | break; // end Shift + ALT + arrows (large resize rectangle) | ||||
210 | | ||||
As the mapping between Qt.Key_* and "*" is static, for direction you could pass the key directly instead of creating an extra string. rkflx: As the mapping between `Qt.Key_*` and `"*"` is static, for `direction` you could pass the key… | |||||
211 | } | ||||
212 | | ||||
213 | // all switches done; repaint on any keypress | ||||
83 | cropDisplayCanvas.requestPaint(); | 214 | cropDisplayCanvas.requestPaint(); | ||
215 | | ||||
216 | function checkBounds(changeX, changeY, direction) { | ||||
217 | | ||||
218 | var leftEdge = selection.x; | ||||
219 | var rightEdge = selection.x + selection.width; | ||||
220 | var topEdge = selection.y; | ||||
221 | var bottomEdge = selection.y + selection.height; | ||||
222 | | ||||
223 | const screenMaxX = cropDisplayCanvas.width; | ||||
224 | const screenMaxY = cropDisplayCanvas.height; | ||||
225 | | ||||
226 | var newX; | ||||
227 | var newY; | ||||
228 | | ||||
229 | var overlap; | ||||
230 | | ||||
231 | newX = selection.x + changeX; | ||||
232 | newY = selection.y + changeY; | ||||
233 | | ||||
234 | switch (direction) { | ||||
235 | | ||||
236 | case "left": | ||||
237 | if (leftEdge + changeX > 0.0) { | ||||
238 | return changeX; | ||||
239 | } | ||||
240 | | ||||
241 | if (leftEdge + changeX < 0.0) { | ||||
242 | return -leftEdge; | ||||
243 | } | ||||
244 | break; | ||||
245 | | ||||
246 | case "right": | ||||
247 | newX = newX + selection.width; | ||||
248 | if (newX < screenMaxX) { | ||||
249 | return changeX; | ||||
250 | } | ||||
251 | if (newX > screenMaxX) { | ||||
252 | overlap = newX - screenMaxX; | ||||
253 | return changeX - overlap; | ||||
84 | } | 254 | } | ||
255 | break; | ||||
256 | | ||||
257 | case "up": | ||||
Missing break (not that it would matter with those returns, but there should be at least a consistent style). rkflx: Missing `break` (not that it would matter with those `returns`, but there should be at least a… | |||||
258 | if (topEdge + changeY > 0.0) { | ||||
259 | return changeY; | ||||
260 | } else { | ||||
261 | return -topEdge; | ||||
85 | } | 262 | } | ||
86 | 263 | | |||
264 | case "down": | ||||
265 | newY = newY + selection.height; | ||||
266 | if (newY < screenMaxY) { | ||||
267 | return changeY; | ||||
268 | } | ||||
269 | if (newY > screenMaxY) { | ||||
270 | overlap = newY - screenMaxY; | ||||
271 | return changeY - overlap; | ||||
272 | } | ||||
273 | break; | ||||
274 | | ||||
275 | } | ||||
276 | } | ||||
277 | } // end Keys.onPressed | ||||
278 | | ||||
279 | | ||||
87 | Keys.onReleased: { | 280 | Keys.onReleased: { | ||
88 | if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) { | 281 | if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) { | ||
89 | toggleMagnifier = false; | 282 | toggleMagnifier = false; | ||
90 | cropDisplayCanvas.requestPaint(); | 283 | cropDisplayCanvas.requestPaint(); | ||
91 | } | 284 | } | ||
92 | } | 285 | } | ||
93 | 286 | | |||
94 | // signals | 287 | // signals | ||
▲ Show 20 Lines • Show All 155 Lines • ▼ Show 20 Line(s) | 442 | TextMetrics { | |||
250 | id: selectionTextMetrics | 443 | id: selectionTextMetrics | ||
251 | } | 444 | } | ||
252 | 445 | | |||
253 | Rectangle { | 446 | Rectangle { | ||
254 | id: midHelpText; | 447 | id: midHelpText; | ||
255 | objectName: "midHelpText"; | 448 | objectName: "midHelpText"; | ||
256 | 449 | | |||
257 | height: midHelpTextElement.height + 40; | 450 | height: midHelpTextElement.height + 40; | ||
258 | width: midHelpTextElement.width + 40; | 451 | width: midHelpTextElement.width + 40; | ||
259 | radius: 4; | 452 | radius: 4; | ||
260 | border.color: systemPalette.windowText; | 453 | border.color: systemPalette.windowText; | ||
261 | color: labelBackgroundColour; | 454 | color: labelBackgroundColour; | ||
262 | 455 | | |||
263 | visible: false; | 456 | visible: false; | ||
264 | anchors.centerIn: parent; | 457 | anchors.centerIn: parent; | ||
265 | 458 | | |||
266 | Label { | 459 | Label { | ||
267 | id: midHelpTextElement; | 460 | id: midHelpTextElement; | ||
268 | text: i18n("Click anywhere to start drawing a selection rectangle,\n" + | 461 | text: i18n("Click anywhere to start drawing a selection rectangle,\n" + | ||
269 | "or press Esc to cancel."); | 462 | "or press Esc to cancel."); | ||
270 | font.pixelSize: Qt.application.font.pixelSize * 1.2; | 463 | font.pixelSize: Qt.application.font.pixelSize * 1.2; | ||
271 | 464 | | |||
272 | anchors.centerIn: parent; | 465 | anchors.centerIn: parent; | ||
273 | } | 466 | } | ||
274 | } | 467 | } | ||
275 | 468 | | |||
276 | Rectangle { | 469 | Rectangle { | ||
277 | id: bottomHelpText; | 470 | id: bottomHelpText; | ||
278 | objectName: "bottomHelpText"; | 471 | objectName: "bottomHelpText"; | ||
279 | 472 | | |||
280 | height: bottomHelpTextElement.height + 20; | 473 | height: bottomHelpTextElement.height + 20; | ||
281 | width: bottomHelpTextElement.width + 20; | 474 | width: bottomHelpTextElement.width + 20; | ||
282 | radius: 4; | 475 | radius: 4; | ||
283 | border.color: systemPalette.windowText; | 476 | border.color: systemPalette.windowText; | ||
284 | color: labelBackgroundColour; | 477 | color: labelBackgroundColour; | ||
285 | 478 | | |||
286 | visible: false; | 479 | visible: false; | ||
287 | anchors.bottom: parent.bottom; | 480 | anchors.bottom: parent.bottom; | ||
288 | anchors.horizontalCenter: parent.horizontalCenter; | 481 | anchors.horizontalCenter: parent.horizontalCenter; | ||
289 | 482 | | |||
290 | GridLayout { | 483 | GridLayout { | ||
291 | id: bottomHelpTextElement; | 484 | id: bottomHelpTextElement; | ||
292 | columns: 2 | 485 | columns: 2 | ||
293 | anchors.centerIn: parent; | 486 | anchors.centerIn: parent; | ||
294 | 487 | | |||
295 | Label { | 488 | Label { | ||
296 | text: i18n("Enter, double-click:"); | 489 | text: i18n("Enter, double-click:"); | ||
297 | Layout.alignment: Qt.AlignRight; | 490 | Layout.alignment: Qt.AlignRight; | ||
298 | } | 491 | } | ||
299 | Label { text: i18n("Take screenshot"); } | 492 | Label { text: i18n("Take screenshot"); } | ||
300 | 493 | | |||
301 | Label { | 494 | Label { | ||
302 | text: i18n("Shift:"); | 495 | text: i18n("Shift:"); | ||
303 | Layout.alignment: Qt.AlignRight; | 496 | Layout.alignment: Qt.AlignRight; | ||
304 | } | 497 | } | ||
305 | Label { text: i18n("Hold to toggle magnifier"); } | 498 | Label { text: i18n("Hold to toggle magnifier"); } | ||
306 | 499 | | |||
307 | Label { | 500 | Label { | ||
501 | text: i18n("Arrow keys:"); | ||||
502 | Layout.alignment: Qt.AlignRight | Qt.AlignTop; | ||||
503 | } | ||||
504 | Label { text: i18n("Move selection rectangle \n" + | ||||
ngraham: "Hove"? :-)
How about "Move selection rectangle" | |||||
505 | "Hold Alt to resize, Shift to fine-tune"); } | ||||
506 | | ||||
507 | Label { | ||||
308 | text: i18n("Right-click:"); | 508 | text: i18n("Right-click:"); | ||
309 | Layout.alignment: Qt.AlignRight; | 509 | Layout.alignment: Qt.AlignRight; | ||
310 | } | 510 | } | ||
ngraham: How about "Move selection rectangle <number> pixels"? | |||||
ngraham: How about taking the value from `largeChange` above? | |||||
311 | Label { text: i18n("Reset selection"); } | 511 | Label { text: i18n("Reset selection"); } | ||
312 | 512 | | |||
313 | Label { | 513 | Label { | ||
314 | text: i18n("Esc:"); | 514 | text: i18n("Esc:"); | ||
315 | Layout.alignment: Qt.AlignRight; | 515 | Layout.alignment: Qt.AlignRight; | ||
Currently I don't understand what all those changes are for and how they relate to the topic of the patch (and I would disagree with some). Contrary to @ngraham I'd say this actually is a visual regression. rkflx: Currently I don't understand what all those changes are for and how they relate to the topic of… | |||||
These are remnants (which will be reverted) from when the help text box was two columns wide. sharvey: These are remnants (which will be reverted) from when the help text box was two columns wide. | |||||
Not done. While you fixed the alignment, there are still a couple of extra changes (lines 448 to 490, line 525). You might want to look at the Diff again, either locally or in Phab. rkflx: Not done. While you fixed the alignment, there are still a couple of extra changes (lines 448… | |||||
316 | } | 516 | } | ||
ngraham: etc. | |||||
317 | Label { text: i18n("Cancel"); } | 517 | Label { text: i18n("Cancel"); } | ||
318 | } | 518 | } | ||
319 | } | 519 | } | ||
320 | 520 | | |||
Would it make sense to use the same capitalization as above, i.e. "keys" instead of "Keys"? rkflx: Would it make sense to use the same capitalization as above, i.e. "keys" instead of "Keys"? | |||||
321 | // Use Rectangle so that the background is white when cursor nearby edge | 521 | // Use Rectangle so that the background is white when cursor nearby edge | ||
rkflx: Do you mean `|` instead of `&&` by any chance? | |||||
322 | Rectangle { | 522 | Rectangle { | ||
ngraham: etc. | |||||
323 | id: crossMagnifier; | 523 | id: crossMagnifier; | ||
Let's change the first line to just "Move selection rectangle", since that's all it does without a modifier key. The next line that introduces the modifier keys for resizing is enough, I think. ngraham: Let's change the first line to just "Move selection rectangle", since that's all it does… | |||||
324 | 524 | | |||
I'd recommend consistency with the terminology used in menus and instead say, "Alt". ngraham: I'd recommend consistency with the terminology used in menus and instead say, "Alt". | |||||
I would suggest to keep "Esc" at the bottom, to make it easier to find. "Arrow keys" could go after "Shift", so "Right-click" would still be next to "Esc" to which it belongs. rkflx: I would suggest to keep "Esc" at the bottom, to make it easier to find. "Arrow keys" could go… | |||||
Not done. Why not move the new entry up one more line? IIRC I specifically put "Reset" next to "Cancel" when reorganizing this recently. rkflx: > "Arrow keys" could go after "Shift"
Not done. Why not move the new entry up one more line? | |||||
"15px change" does sound a bit strange: What is "change", and why do "15px" matter? Let me repeat my suggestion from above: Hold Alt to resize, fine-tune with Shift rkflx: "15px change" does sound a bit strange: What is "change", and why do "15px" matter? Let me… | |||||
325 | height: (magPixels * 2 + 1) * magZoom; | 525 | height: (magPixels * 2 + 1) * magZoom; | ||
326 | width: height; | 526 | width: height; | ||
327 | border.width: 0; | 527 | border.width: 0; | ||
328 | visible: false; | 528 | visible: false; | ||
329 | clip: true | 529 | clip: true | ||
330 | 530 | | |||
331 | Image { | 531 | Image { | ||
332 | id: crossBackground; | 532 | id: crossBackground; | ||
Show All 30 Lines | |||||
363 | Rectangle { | 563 | Rectangle { | ||
364 | x: (magPixels + 1) * magZoom; | 564 | x: (magPixels + 1) * magZoom; | ||
365 | y: magPixels * magZoom; | 565 | y: magPixels * magZoom; | ||
366 | width: magPixels * magZoom; | 566 | width: magPixels * magZoom; | ||
367 | height: magZoom; | 567 | height: magZoom; | ||
368 | color: crossColour; | 568 | color: crossColour; | ||
369 | } | 569 | } | ||
370 | } | 570 | } | ||
371 | | ||||
rkflx: Unrelated whitespace change. | |||||
372 | } | 571 | } | ||
373 | 572 | | |||
374 | MouseArea { | 573 | MouseArea { | ||
375 | anchors.fill: imageBackground; | 574 | anchors.fill: imageBackground; | ||
376 | 575 | | |||
377 | property int startx: 0; | 576 | property int startx: 0; | ||
378 | property int starty: 0; | 577 | property int starty: 0; | ||
379 | 578 | | |||
▲ Show 20 Lines • Show All 54 Lines • Show Last 20 Lines |
That does not seem right, because this will result in slower movement on a HiDPI screen compared to a regular screen of the same size. (It's needed for the 1px case, of course.)