Differential D11599 Diff 32359 src/QuickEditor/EditorRoot.qml

Changeset View

Standalone View

View Options

src/QuickEditor/EditorRoot.qml

Show All 23 Lines23Item {
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 bool showMagnifier: false;
34​ property bool toggleMagnifier: false;
35​ property int magZoom: 5;
36​ property int magPixels: 16;
alexeyminUnsubmitted
Done

can these magZoom, magPixels, magOffset have type int instead of var? Just asking, why var?

alexeymin: can these `magZoom`, `magPixels`, `magOffset` have type `int` instead of `var`? Just asking…
37​ property int magOffset: 32;
38​
32 SystemPalette { 39​ SystemPalette {
33 id: systemPalette; 40​ id: systemPalette;
34 } 41​ }
35 42​
36 function setInitialSelection(xx, yy, ww, hh) { 43​ function setInitialSelection(xx, yy, ww, hh) {
37 if (selection) { 44​ if (selection) {
38 selection.destroy(); 45​ selection.destroy();
39 } 46​ }
40 47​
41 selection = cropRectangle.createObject(parent, { 48​ selection = cropRectangle.createObject(parent, {
42 "x": xx, 49​ "x": xx,
43 "y": yy, 50​ "y": yy,
44 "height": hh, 51​ "height": hh,
45 "width": ww 52​ "width": ww
46 }); 53​ });
47 54​
48 cropDisplayCanvas.requestPaint(); 55​ cropDisplayCanvas.requestPaint();
49 } 56​ }
50 57​
51 function accept() { 58​ function accept() {
52 if (selection) { 59​ if (selection) {
53 acceptImage(selection.x, selection.y, selection.width, selection.height); 60​ acceptImage(selection.x, selection.y, selection.width, selection.height);
54 } else { 61​ } else {
alexeyminUnsubmitted
Done

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); 62​ acceptImage(-1, -1, -1, -1);
56 } 63​ }
57 } 64​ }
58 65​
59 // key handlers 66​ // key handlers
60 67​
61 focus: true; 68​ focus: true;
62 69​
63 Keys.onReturnPressed: accept() 70​ Keys.onReturnPressed: accept()
64 Keys.onEnterPressed: accept() 71​ Keys.onEnterPressed: accept()
65 72​
66 Keys.onEscapePressed: { 73​ Keys.onEscapePressed: {
67 cancelImage(); 74​ cancelImage();
68 } 75​ }
69 76​
77​ Keys.onPressed: {
78​ if (event.modifiers | Qt.AltModifier) {
rkflxUnsubmitted
Done

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…
guotaoAuthorUnsubmitted
Done

my bad, wrong logic

guotao: my bad, wrong logic
79​ toggleMagnifier = true;
80​ cropDisplayCanvas.requestPaint();
81​ }
82​ }
83​
84​ Keys.onReleased: {
85​ if (toggleMagnifier && (event.modifiers | Qt.AltModifier)) {
86​ toggleMagnifier = false;
87​ cropDisplayCanvas.requestPaint();
88​ }
89​ }
90​
70 // signals 91​ // signals
71 92​
72 signal acceptImage(int x, int y, int width, int height); 93​ signal acceptImage(int x, int y, int width, int height);
73 signal cancelImage(); 94​ signal cancelImage();
74 95​
75 Image { 96​ Image {
76 id: imageBackground; 97​ id: imageBackground;
77 objectName: "imageBackground"; 98​ objectName: "imageBackground";
▲ Show 20 Lines • Show All 105 Lines • ▼ Show 20 Line(s)128 if (selection) {
183 } 204​ }
184 // Now do the actual box, border and text drawing 205​ // Now do the actual box, border and text drawing
185 ctx.fillStyle = systemPalette.window; 206​ ctx.fillStyle = systemPalette.window;
186 ctx.strokeStyle = systemPalette.windowText; 207​ ctx.strokeStyle = systemPalette.windowText;
187 ctx.fillRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8); 208​ 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); 209​ ctx.strokeRect(selectionBoxX - 4, selectionBoxY - selectionTextRect.height - 2, selectionTextRect.width + 10, selectionTextRect.height + 8);
189 ctx.fillStyle = systemPalette.windowText; 210​ ctx.fillStyle = systemPalette.windowText;
190 ctx.fillText(selectionText, selectionBoxX, selectionBoxY); 211​ ctx.fillText(selectionText, selectionBoxX, selectionBoxY);
212​ if (selection.zoomCenterX >= 0 && selection.zoomCenterY >= 0 && (showMagnifier ^ toggleMagnifier)) {
213​ var offsetX = magOffset;
214​ var offsetY = magOffset;
215​ var magX = selection.zoomCenterX;
216​ var magY = selection.zoomCenterY;
217​ var magWidth = (magPixels * 2 + 1) * magZoom;
218​ var magHeight = (magPixels * 2 + 1) * magZoom;
ngrahamUnsubmitted
Done

Make all of these ints, please. vars are much slower.

ngraham: Make all of these `int`s, please. `var`s are much slower.
alexeyminUnsubmitted
Done

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 (
guotaoAuthorUnsubmitted
Done

@alexeymin, so it's doesn't need change here, right?

guotao: @alexeymin, so it's doesn't need change here, right?
alexeyminUnsubmitted
Done

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…
rkflxUnsubmitted
Done

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…
219​
220​ if (magX + offsetX + magWidth >= Window.width / Screen.devicePixelRatio) {
221​ offsetX -= offsetX * 2 + magWidth;
222​ }
223​
224​ if (magY + offsetY + magHeight >= Window.height / Screen.devicePixelRatio) {
225​ offsetY -= offsetY * 2 + magHeight;
226​ }
227​
228​ magX += offsetX;
229​ magY += offsetY;
230​ crossMagnifier.visible = true;
231​ crossMagnifier.x = magX;
232​ crossMagnifier.y = magY;
233​ crossBackground.x = -selection.zoomCenterX * Screen.devicePixelRatio * magZoom + magPixels * magZoom;
234​ crossBackground.y = -selection.zoomCenterY * Screen.devicePixelRatio * magZoom + magPixels * magZoom;
235​ ctx.strokeRect(magX, magY, magWidth, magHeight);
236​ } else {
237​ crossMagnifier.visible = false;
238​ }
191 } else { 239​ } else {
192 midHelpText.visible = true; 240​ midHelpText.visible = true;
193 bottomHelpText.visible = false; 241​ bottomHelpText.visible = false;
242​ crossMagnifier.visible = false;
194 } 243​ }
195 } 244​ }
196 245​
197 TextMetrics { 246​ TextMetrics {
198 id: selectionTextMetrics 247​ id: selectionTextMetrics
199 } 248​ }
200 249​
201 Rectangle { 250​ Rectangle {
Show All 22 Lines272 Rectangle {
224 id: bottomHelpText; 273​ id: bottomHelpText;
225 objectName: "bottomHelpText"; 274​ objectName: "bottomHelpText";
226 275​
227 height: bottomHelpTextElement.height + 16; 276​ height: bottomHelpTextElement.height + 16;
228 width: bottomHelpTextElement.width + 24; 277​ width: bottomHelpTextElement.width + 24;
229 border.width: 1; 278​ border.width: 1;
230 border.color: Qt.rgba(0, 0, 0, 1); 279​ border.color: Qt.rgba(0, 0, 0, 1);
231 color: Qt.rgba(1, 1, 1, 0.85); 280​ color: Qt.rgba(1, 1, 1, 0.85);
281​ visible: false;
rkflxUnsubmitted
Done

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?
guotaoAuthorUnsubmitted
Done

I change this because the help text show and hide very quickly.
I think the default value should be false.

guotao: I change this because the help text show and hide very quickly. I think the default value…
rkflxUnsubmitted
Done

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 282​
233 anchors.bottom: parent.bottom; 283​ anchors.bottom: parent.bottom;
234 anchors.horizontalCenter: parent.horizontalCenter; 284​ anchors.horizontalCenter: parent.horizontalCenter;
235 285​
236 Text { 286​ Text {
237 id: bottomHelpTextElement; 287​ 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"); 288​ 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; 289​ font.pointSize: 9;
240 290​
241 anchors.centerIn: parent; 291​ anchors.centerIn: parent;
242 } 292​ }
243 } 293​ }
294​
295​ Rectangle {
296​ id: crossMagnifier;
297​ objectName: "crossMagnifier";
298​
299​ height: (magPixels * 2 + 1) * magZoom;
300​ width: (magPixels * 2 + 1) * magZoom;
rkflxUnsubmitted
Done

Why not use height here?

rkflx: Why not use `height` here?
guotaoAuthorUnsubmitted
Done

sorry, what you mean?

guotao: sorry, what you mean?
rkflxUnsubmitted
Done

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…
301​ border.width: 0;
302​ visible: false;
ngrahamUnsubmitted
Done

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…
guotaoAuthorUnsubmitted
Done

What Item? Sorry I am newbie for QML.
I tried Item, Object and all of them not work

guotao: What Item? Sorry I am newbie for QML. I tried Item, Object and all of them not work
rkflxUnsubmitted
Done

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…
rkflxUnsubmitted
Done

@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…
303​ clip: true
304​
305​ Image {
306​ id: crossBackground;
307​ objectName: "crossBackground";
308​ source: "image://snapshot/rawimage";
309​ cache: true;
rkflxUnsubmitted
Done

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…
guotaoAuthorUnsubmitted
Done

I just follow the exist code.
After set to true, it seem works well.

guotao: I just follow the exist code. After set to true, it seem works well.
rkflxUnsubmitted
Done

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.
rkflxUnsubmitted
Done

Why is this marked as "Done" when the line is still there?

rkflx: Why is this marked as "Done" when the line is still there?
310​ smooth: false;
311​ height: Window.height * magZoom;
312​ width: Window.width * magZoom;
313​ }
rkflxUnsubmitted
Done

Is this needed?

rkflx: Is this needed?
guotaoAuthorUnsubmitted
Done

It seems my debug code but not removed.
after remove, works well.

guotao: It seems my debug code but not removed. after remove, works well.
314​
315​ Rectangle {
316​ x: magPixels * magZoom;
317​ y: 0;
318​ width: magZoom;
319​ height: magPixels * magZoom;
320​ color: crossColour;
321​ }
322​
323​ Rectangle {
324​ x: magPixels * magZoom;
325​ y: (magPixels + 1) * magZoom;
326​ width: magZoom;
327​ height: magPixels * magZoom;
328​ color: crossColour;
329​ }
330​
331​ Rectangle {
332​ x: 0;
333​ y: magPixels * magZoom;
334​ width: magPixels * magZoom;
335​ height: magZoom;
336​ color: crossColour;
337​ }
338​
339​ Rectangle {
340​ x: (magPixels + 1) * magZoom;
341​ y: magPixels * magZoom;
342​ width: magPixels * magZoom;
343​ height: magZoom;
344​ color: crossColour;
345​ }
346​ }
347​
244 } 348​ }
245 349​
246 MouseArea { 350​ MouseArea {
247 anchors.fill: imageBackground; 351​ anchors.fill: imageBackground;
248 352​
249 property int startx: 0; 353​ property int startx: 0;
250 property int starty: 0; 354​ property int starty: 0;
251 355​
Show All 16 Lines359 onPressed: {
268 }); 372​ });
269 } 373​ }
270 374​
271 onPositionChanged: { 375​ onPositionChanged: {
272 selection.x = Math.min(startx, mouse.x); 376​ selection.x = Math.min(startx, mouse.x);
273 selection.y = Math.min(starty, mouse.y); 377​ selection.y = Math.min(starty, mouse.y);
274 selection.width = Math.abs(startx - mouse.x) + 1; 378​ selection.width = Math.abs(startx - mouse.x) + 1;
275 selection.height = Math.abs(starty - mouse.y) + 1; 379​ selection.height = Math.abs(starty - mouse.y) + 1;
276 380​ selection.zoomCenterX = mouse.x;
381​ selection.zoomCenterY = mouse.y;
277 cropDisplayCanvas.requestPaint(); 382​ cropDisplayCanvas.requestPaint();
278 } 383​ }
279 384​
280 onClicked: { 385​ onClicked: {
281 if ((mouse.button == Qt.RightButton) && (selection)) { 386​ if ((mouse.button == Qt.RightButton) && (selection)) {
282 selection.destroy(); 387​ selection.destroy();
283 cropDisplayCanvas.requestPaint(); 388​ cropDisplayCanvas.requestPaint();
284 } 389​ }
285 } 390​ }
391​
392​ onReleased: {
393​ selection.zoomCenterX = -1;
394​ selection.zoomCenterY = -1;
395​ cropDisplayCanvas.requestPaint();
396​ }
286 } 397​ }
287 398​
288 Component { 399​ Component {
289 id: cropRectangle; 400​ id: cropRectangle;
290 401​
291 SelectionRectangle { 402​ SelectionRectangle {
292 drawCanvas: cropDisplayCanvas; 403​ drawCanvas: cropDisplayCanvas;
293 imageElement: imageBackground; 404​ imageElement: imageBackground;
294 405​
295 onDoubleClicked: editorRoot.accept() 406​ onDoubleClicked: editorRoot.accept()
296 } 407​ }
297 } 408​ }
298} 409​}