Changeset View
Standalone View
src/QuickEditor/EditorRoot.qml
Show All 23 Lines | 23 | Item { | |||
---|---|---|---|---|---|
24 | id: editorRoot; | 24 | id: editorRoot; | ||
25 | objectName: "editorRoot"; | 25 | objectName: "editorRoot"; | ||
26 | 26 | | |||
27 | // properties and setters | 27 | // properties and setters | ||
28 | 28 | | |||
29 | property var selection: undefined; | 29 | property var selection: undefined; | ||
30 | property color maskColour: Qt.rgba(0, 0, 0, 0.15); | 30 | property color maskColour: Qt.rgba(0, 0, 0, 0.15); | ||
31 | property color strokeColour: Qt.rgba(0.114, 0.6, 0.953, 1); | 31 | property color strokeColour: Qt.rgba(0.114, 0.6, 0.953, 1); | ||
32 | property color crossColour: Qt.rgba(0.114, 0.6, 0.953, 0.5); | ||||
33 | property var magZoom: 5; | ||||
34 | property var magPixels: 16; | ||||
35 | property var magOffset: 16; | ||||
36 | | ||||
alexeymin: can these `magZoom`, `magPixels`, `magOffset` have type `int` instead of `var`? Just asking… | |||||
32 | SystemPalette { | 37 | SystemPalette { | ||
33 | id: systemPalette; | 38 | id: systemPalette; | ||
34 | } | 39 | } | ||
35 | 40 | | |||
36 | function setInitialSelection(xx, yy, ww, hh) { | 41 | function setInitialSelection(xx, yy, ww, hh) { | ||
37 | if (selection) { | 42 | if (selection) { | ||
38 | selection.destroy(); | 43 | selection.destroy(); | ||
39 | } | 44 | } | ||
40 | 45 | | |||
41 | selection = cropRectangle.createObject(parent, { | 46 | selection = cropRectangle.createObject(parent, { | ||
42 | "x": xx, | 47 | "x": xx, | ||
43 | "y": yy, | 48 | "y": yy, | ||
44 | "height": hh, | 49 | "height": hh, | ||
45 | "width": ww | 50 | "width": ww | ||
46 | }); | 51 | }); | ||
47 | 52 | | |||
48 | cropDisplayCanvas.requestPaint(); | 53 | cropDisplayCanvas.requestPaint(); | ||
49 | } | 54 | } | ||
50 | 55 | | |||
51 | function accept() { | 56 | function accept() { | ||
52 | if (selection) { | 57 | if (selection) { | ||
53 | acceptImage(selection.x, selection.y, selection.width, selection.height); | 58 | acceptImage(selection.x, selection.y, selection.width, selection.height); | ||
54 | } else { | 59 | } else { | ||
This function setShowMagnifier() is probably not needed at all. You can set context property directly from C++, without invokeMethod, see below alexeymin: This function `setShowMagnifier()` is probably not needed at all. You can set context property… | |||||
55 | acceptImage(-1, -1, -1, -1); | 60 | acceptImage(-1, -1, -1, -1); | ||
56 | } | 61 | } | ||
57 | } | 62 | } | ||
58 | 63 | | |||
59 | // key handlers | 64 | // key handlers | ||
60 | 65 | | |||
61 | focus: true; | 66 | focus: true; | ||
62 | 67 | | |||
63 | Keys.onReturnPressed: accept() | 68 | Keys.onReturnPressed: accept() | ||
64 | Keys.onEnterPressed: accept() | 69 | Keys.onEnterPressed: accept() | ||
65 | 70 | | |||
66 | Keys.onEscapePressed: { | 71 | Keys.onEscapePressed: { | ||
67 | cancelImage(); | 72 | cancelImage(); | ||
68 | } | 73 | } | ||
69 | 74 | | |||
70 | // signals | 75 | // signals | ||
71 | 76 | | |||
For me this triggers for every key on the keyboard. Could you make it so that only ⇧+Alt+Ctrl affect the magnifier? rkflx: For me this triggers for every key on the keyboard. Could you make it so that only {key Shift… | |||||
guotao: my bad, wrong logic | |||||
72 | signal acceptImage(int x, int y, int width, int height); | 77 | signal acceptImage(int x, int y, int width, int height); | ||
73 | signal cancelImage(); | 78 | signal cancelImage(); | ||
74 | 79 | | |||
75 | Image { | 80 | Image { | ||
76 | id: imageBackground; | 81 | id: imageBackground; | ||
77 | objectName: "imageBackground"; | 82 | objectName: "imageBackground"; | ||
78 | 83 | | |||
79 | source: "image://snapshot/rawimage"; | 84 | source: "image://snapshot/rawimage"; | ||
▲ Show 20 Lines • Show All 103 Lines • ▼ Show 20 Line(s) | 112 | if (selection) { | |||
183 | } | 188 | } | ||
184 | // Now do the actual box, border and text drawing | 189 | // Now do the actual box, border and text drawing | ||
185 | ctx.fillStyle = systemPalette.window; | 190 | ctx.fillStyle = systemPalette.window; | ||
186 | ctx.strokeStyle = systemPalette.windowText; | 191 | ctx.strokeStyle = systemPalette.windowText; | ||
187 | ctx.fillRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8); | 192 | ctx.fillRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8); | ||
188 | ctx.strokeRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8); | 193 | ctx.strokeRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8); | ||
189 | ctx.fillStyle = systemPalette.windowText; | 194 | ctx.fillStyle = systemPalette.windowText; | ||
190 | ctx.fillText(selectionText, selectionBoxX, selectionBoxY); | 195 | ctx.fillText(selectionText, selectionBoxX, selectionBoxY); | ||
196 | if (selection.zx >= 0 && selection.zy >= 0) { | ||||
197 | var offsetX = magOffset; | ||||
198 | var offsetY = magOffset; | ||||
199 | var magX = selection.zx; | ||||
200 | var magY = selection.zy; | ||||
201 | var magWidth = (magPixels * 2 + 1) * magZoom; | ||||
202 | var magHeight = (magPixels * 2 + 1) * magZoom; | ||||
ngraham: Make all of these `int`s, please. `var`s are much slower. | |||||
Javascript only has var type for variables. Not a property declaration here ( alexeymin: Javascript only has `var` type for variables. Not a property declaration here ( | |||||
@alexeymin, so it's doesn't need change here, right? guotao: @alexeymin, so it's doesn't need change here, right? | |||||
Yes, in property declarations you can say property int x, property double y, but variable declarations inside JS code can only use var or let. double offsetX will produce unexpected token errors here. alexeymin: Yes, in property declarations you can say `property int x`, `property double y`, but variable… | |||||
You could be more DRY here: var magWidth = crossMagnifier.width; var magHeight = crossMagnifier.height; rkflx: You could be more [DRY](https://en.wikipedia.org/wiki/Don't_repeat_yourself) here:
```
var… | |||||
203 | | ||||
204 | if (magX + offsetX + magWidth >= Window.width / Screen.devicePixelRatio) { | ||||
205 | offsetX -= offsetX * 2 + magWidth; | ||||
206 | } | ||||
207 | | ||||
208 | if (magY + offsetY + magHeight >= Window.height / Screen.devicePixelRatio) { | ||||
209 | offsetY -= offsetY * 2 + magHeight; | ||||
210 | } | ||||
211 | | ||||
212 | magX += offsetX; | ||||
213 | magY += offsetY; | ||||
214 | crossMagnifier.visible = true; | ||||
215 | crossMagnifier.x = magX; | ||||
216 | crossMagnifier.y = magY; | ||||
217 | crossBackground.x = -selection.zx * Screen.devicePixelRatio * magZoom + magPixels * magZoom; | ||||
218 | crossBackground.y = -selection.zy * Screen.devicePixelRatio * magZoom + magPixels * magZoom; | ||||
219 | ctx.strokeRect(magX, magY, magWidth, magHeight); | ||||
220 | } else { | ||||
221 | crossMagnifier.visible = false; | ||||
222 | } | ||||
191 | } else { | 223 | } else { | ||
192 | midHelpText.visible = true; | 224 | midHelpText.visible = true; | ||
193 | bottomHelpText.visible = false; | 225 | bottomHelpText.visible = false; | ||
226 | crossMagnifier.visible = false; | ||||
194 | } | 227 | } | ||
195 | } | 228 | } | ||
196 | 229 | | |||
197 | TextMetrics { | 230 | TextMetrics { | ||
198 | id: selectionTextMetrics | 231 | id: selectionTextMetrics | ||
199 | } | 232 | } | ||
200 | 233 | | |||
201 | Rectangle { | 234 | Rectangle { | ||
Show All 22 Lines | 256 | Rectangle { | |||
224 | id: bottomHelpText; | 257 | id: bottomHelpText; | ||
225 | objectName: "bottomHelpText"; | 258 | objectName: "bottomHelpText"; | ||
226 | 259 | | |||
227 | height: bottomHelpTextElement.height + 16; | 260 | height: bottomHelpTextElement.height + 16; | ||
228 | width: bottomHelpTextElement.width + 24; | 261 | width: bottomHelpTextElement.width + 24; | ||
229 | border.width: 1; | 262 | border.width: 1; | ||
230 | border.color: Qt.rgba(0, 0, 0, 1); | 263 | border.color: Qt.rgba(0, 0, 0, 1); | ||
231 | color: Qt.rgba(1, 1, 1, 0.85); | 264 | color: Qt.rgba(1, 1, 1, 0.85); | ||
265 | visible: false; | ||||
Maybe there's a reason you have to add this, but currently I don't see it. Could you explain? rkflx: Maybe there's a reason you have to add this, but currently I don't see it. Could you explain? | |||||
I change this because the help text show and hide very quickly. guotao: I change this because the help text show and hide very quickly.
I think the default value… | |||||
Ah, I see, makes sense. I was looking for an explanation connected to the magnifier, but apparently there is none to find ;) Could you split out this change into a separate Diff, then? (Could even go to the Applications/18.04 branch, so users will get the fix earlier than the magnifier, which would have to wait until 18.08…) rkflx: Ah, I see, makes sense. I was looking for an explanation connected to the magnifier, but… | |||||
232 | 266 | | |||
233 | anchors.bottom: parent.bottom; | 267 | anchors.bottom: parent.bottom; | ||
234 | anchors.horizontalCenter: parent.horizontalCenter; | 268 | anchors.horizontalCenter: parent.horizontalCenter; | ||
235 | 269 | | |||
236 | Text { | 270 | Text { | ||
237 | id: bottomHelpTextElement; | 271 | id: bottomHelpTextElement; | ||
238 | text: i18n("To take the screenshot, double-click or press Enter. Right-click to reset the selection, or press Esc to quit"); | 272 | text: i18n("To take the screenshot, double-click or press Enter. Right-click to reset the selection, or press Esc to quit"); | ||
239 | font.pointSize: 9; | 273 | font.pointSize: 9; | ||
240 | 274 | | |||
241 | anchors.centerIn: parent; | 275 | anchors.centerIn: parent; | ||
242 | } | 276 | } | ||
243 | } | 277 | } | ||
278 | | ||||
279 | Rectangle { | ||||
280 | id: crossMagnifier; | ||||
281 | objectName: "crossMagnifier"; | ||||
282 | | ||||
283 | height: ( magPixels * 2 + 1) * magZoom; | ||||
284 | width: ( magPixels * 2 + 1) * magZoom; | ||||
rkflx: Why not use `height` here? | |||||
guotao: sorry, what you mean? | |||||
You are duplicating the calculation for height also for width. As you simply want both to be the same, you could write: width: height; rkflx: You are duplicating the calculation for `height` also for `width`. As you simply want both to… | |||||
285 | border.width: 0; | ||||
286 | visible: false; | ||||
Instead of giving this poor Rectangle an invisible border and making it invisible, perhaps it should be a simple Item instead. ngraham: Instead of giving this poor `Rectangle` an invisible border and making it invisible, perhaps it… | |||||
What Item? Sorry I am newbie for QML. guotao: What Item? Sorry I am newbie for QML.
I tried Item, Object and all of them not work
| |||||
@ngraham Ping? rkflx: > What Item? Sorry I am newbie for QML.
> I tried Item, Object and all of them not work… | |||||
@ngraham When changing to Item, for me the background of the magnifier got transparent instead of white when operating near the edges of the screen. Is there a better way you have in mind? @guotao I guess you should add a comment to that effect to the code, so future readers will also understand why you chose a Rectangle. rkflx: @ngraham When changing to `Item`, for me the background of the magnifier got transparent… | |||||
287 | clip: true | ||||
288 | | ||||
289 | Image { | ||||
290 | id: crossBackground; | ||||
291 | objectName: "crossBackground"; | ||||
292 | source: "image://snapshot/rawimage"; | ||||
293 | cache: false; | ||||
Are you sure false is the right choice here? The docs say "at the expense of small 'ui element' images", but I don't see any of those. IIUC this is about caching the image on the GPU, which to me sounds sensible in this case… rkflx: Are you sure `false` is the right choice here? The docs say "at the expense of small 'ui… | |||||
guotao: I just follow the exist code.
After set to true, it seem works well. | |||||
Okay. But if it works well for you on real hardware, I'd tend to go with enabling the cache. Let me investigate this more and possibly change it in the other place too. For crossBackground, you can remove the line for now, because the default value is already true. rkflx: Okay. But if it works well for you on real hardware, I'd tend to go with enabling the cache. | |||||
rkflx: Why is this marked as "Done" when the line is still there? | |||||
294 | smooth: false; | ||||
295 | height: Window.height * magZoom; | ||||
296 | width: Window.width * magZoom; | ||||
297 | fillMode: Image.PreserveAspectCrop; | ||||
rkflx: Is this needed? | |||||
guotao: It seems my debug code but not removed.
after remove, works well. | |||||
298 | } | ||||
299 | | ||||
300 | Rectangle { | ||||
301 | x: magPixels * magZoom; | ||||
302 | y: 0; | ||||
303 | width: magZoom; | ||||
304 | height: magPixels * magZoom; | ||||
305 | color: crossColour; | ||||
306 | } | ||||
307 | | ||||
308 | Rectangle { | ||||
309 | x: magPixels * magZoom; | ||||
310 | y: (magPixels + 1) * magZoom; | ||||
311 | width: magZoom; | ||||
312 | height: magPixels * magZoom; | ||||
313 | color: crossColour; | ||||
314 | } | ||||
315 | | ||||
316 | Rectangle { | ||||
317 | x: 0; | ||||
318 | y: magPixels * magZoom; | ||||
319 | width: magPixels * magZoom; | ||||
320 | height: magZoom; | ||||
321 | color: crossColour; | ||||
322 | } | ||||
323 | | ||||
324 | Rectangle { | ||||
325 | x: (magPixels + 1) * magZoom; | ||||
326 | y: magPixels * magZoom; | ||||
327 | width: magPixels * magZoom; | ||||
328 | height: magZoom; | ||||
329 | color: crossColour; | ||||
330 | } | ||||
331 | } | ||||
332 | | ||||
244 | } | 333 | } | ||
245 | 334 | | |||
246 | MouseArea { | 335 | MouseArea { | ||
247 | anchors.fill: imageBackground; | 336 | anchors.fill: imageBackground; | ||
248 | 337 | | |||
249 | property int startx: 0; | 338 | property int startx: 0; | ||
250 | property int starty: 0; | 339 | property int starty: 0; | ||
251 | 340 | | |||
Show All 16 Lines | 344 | onPressed: { | |||
268 | }); | 357 | }); | ||
269 | } | 358 | } | ||
270 | 359 | | |||
271 | onPositionChanged: { | 360 | onPositionChanged: { | ||
272 | selection.x = Math.min(startx, mouse.x); | 361 | selection.x = Math.min(startx, mouse.x); | ||
273 | selection.y = Math.min(starty, mouse.y); | 362 | selection.y = Math.min(starty, mouse.y); | ||
274 | selection.width = Math.abs(startx - mouse.x) + 1; | 363 | selection.width = Math.abs(startx - mouse.x) + 1; | ||
275 | selection.height = Math.abs(starty - mouse.y) + 1; | 364 | selection.height = Math.abs(starty - mouse.y) + 1; | ||
276 | 365 | selection.zx = mouse.x; | |||
366 | selection.zy = mouse.y; | ||||
277 | cropDisplayCanvas.requestPaint(); | 367 | cropDisplayCanvas.requestPaint(); | ||
278 | } | 368 | } | ||
279 | 369 | | |||
280 | onClicked: { | 370 | onClicked: { | ||
281 | if ((mouse.button == Qt.RightButton) && (selection)) { | 371 | if ((mouse.button == Qt.RightButton) && (selection)) { | ||
282 | selection.destroy(); | 372 | selection.destroy(); | ||
283 | cropDisplayCanvas.requestPaint(); | 373 | cropDisplayCanvas.requestPaint(); | ||
284 | } | 374 | } | ||
285 | } | 375 | } | ||
376 | | ||||
377 | onReleased: { | ||||
378 | selection.zx = -1; | ||||
379 | selection.zy = -1; | ||||
380 | cropDisplayCanvas.requestPaint(); | ||||
381 | } | ||||
286 | } | 382 | } | ||
287 | 383 | | |||
288 | Component { | 384 | Component { | ||
289 | id: cropRectangle; | 385 | id: cropRectangle; | ||
290 | 386 | | |||
291 | SelectionRectangle { | 387 | SelectionRectangle { | ||
292 | drawCanvas: cropDisplayCanvas; | 388 | drawCanvas: cropDisplayCanvas; | ||
293 | imageElement: imageBackground; | 389 | imageElement: imageBackground; | ||
294 | 390 | | |||
295 | onDoubleClicked: editorRoot.accept() | 391 | onDoubleClicked: editorRoot.accept() | ||
296 | } | 392 | } | ||
297 | } | 393 | } | ||
298 | } | 394 | } |
can these magZoom, magPixels, magOffset have type int instead of var? Just asking, why var?