Differential D11599 Diff 30281 src/QuickEditor/EditorRoot.qml

Changeset View

Standalone View

View Options

src/QuickEditor/EditorRoot.qml

Context not available.
188 ctx.strokeRect(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);
189 ctx.fillStyle = systemPalette.windowText; 189​ ctx.fillStyle = systemPalette.windowText;
190 ctx.fillText(selectionText, selectionBoxX, selectionBoxY); 190​ ctx.fillText(selectionText, selectionBoxX, selectionBoxY);
191​ if (selection.zx >= 0 && selection.zy >= 0) {
192​ var zx = selection.zx + 15;
193​ var zy = selection.zy + 15;
194​ if (zx + 72 >= Window.width) {
195​ zx -= 102;
196​ }
197​ if (zy + 72 >= Window.height) {
198​ zy -= 102;
199​ }
200​ crossMagnifier.visible = true;
201​ crossMagnifier.x = zx;
202​ crossMagnifier.y = zy;
203​ crossBackground.x = -selection.zx * 8 + 32;
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…
204​ crossBackground.y = -selection.zy * 8 + 32;
205​ ctx.strokeRect(zx, zy, 72, 72);
206​ } else {
207​ crossMagnifier.visible = false;
208​ }
191 } else { 209​ } else {
192 midHelpText.visible = true; 210​ midHelpText.visible = true;
193 bottomHelpText.visible = false; 211​ bottomHelpText.visible = false;
212​ crossMagnifier.visible = false;
194 } 213​ }
195 } 214​ }
196 215​
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…
Context not available.
229 border.width: 1; 248​ border.width: 1;
230 border.color: Qt.rgba(0, 0, 0, 1); 249​ border.color: Qt.rgba(0, 0, 0, 1);
231 color: Qt.rgba(1, 1, 1, 0.85); 250​ color: Qt.rgba(1, 1, 1, 0.85);
251​ visible: false;
232 252​
233 anchors.bottom: parent.bottom; 253​ anchors.bottom: parent.bottom;
234 anchors.horizontalCenter: parent.horizontalCenter; 254​ anchors.horizontalCenter: parent.horizontalCenter;
Context not available.
241 anchors.centerIn: parent; 261​ anchors.centerIn: parent;
242 } 262​ }
243 } 263​ }
264​
265​ Rectangle {
266​ id: crossMagnifier;
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…
267​ objectName: "crossMagnifier";
268​
269​ height: 72;
270​ width: 72;
271​ border.width: 1;
272​ border.color: Qt.rgba(0, 0, 0, 1);
273​ visible: false;
274​ clip: true
275​
276​ Image {
277​ id: crossBackground;
278​ objectName: "crossBackground";
279​ source: "image://snapshot/rawimage";
280​ cache: false;
281​ smooth: false;
282​ height: Window.height / Screen.devicePixelRatio * 8;
283​ width: Window.width / Screen.devicePixelRatio * 8;
284​ fillMode: Image.PreserveAspectCrop;
285​ }
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…
286​
287​ Rectangle {
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…
288​ x: 32;
289​ y: 0;
290​ width: 8;
291​ height: 72;
292​ color: Qt.rgba(0.114, 0.6, 0.953, 0.5);
293​ }
294​
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?
295​ Rectangle {
296​ x: 0;
297​ y: 32;
298​ width: 72;
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.
299​ height: 8;
300​ color: Qt.rgba(0.114, 0.6, 0.953, 0.5);
301​ }
302​ }
303​
244 } 304​ }
245 305​
246 MouseArea { 306​ MouseArea {
Context not available.
269 } 329​ }
270 330​
271 onPositionChanged: { 331​ onPositionChanged: {
332​ selection.zx = mouse.x;
333​ selection.zy = mouse.y;
272 selection.x = Math.min(startx, mouse.x); 334​ selection.x = Math.min(startx, mouse.x);
273 selection.y = Math.min(starty, mouse.y); 335​ selection.y = Math.min(starty, mouse.y);
274 selection.width = Math.abs(startx - mouse.x); 336​ selection.width = Math.abs(startx - mouse.x);
Context not available.
283 cropDisplayCanvas.requestPaint(); 345​ cropDisplayCanvas.requestPaint();
284 } 346​ }
285 } 347​ }
348​
349​ onReleased: {
350​ selection.zx = -1;
351​ selection.zy = -1;
352​ cropDisplayCanvas.requestPaint();
353​ }
286 } 354​ }
287 355​
288 Component { 356​ Component {
Context not available.