Differential D13450 Diff 37963 src/QuickEditor/EditorRoot.qml

Changeset View

Standalone View

View Options

src/QuickEditor/EditorRoot.qml

Show All 32 Lines25Item {
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 int largeChange: 15 / Screen.devicePixelRatio;
rkflxUnsubmitted
Done

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.)

rkflx: That does not seem right, because this will result in slower movement on a HiDPI screen…
sharveyAuthorUnsubmitted
Done

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 int smallChange: 1 / Screen.devicePixelRatio
rkflxUnsubmitted
Done

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​ });
rkflxUnsubmitted
Done

Unrelated whitespace change, please revert.

rkflx: Unrelated whitespace change, please revert.
57 59​
abalajiUnsubmitted
Done

Are rightX and bottomY used anywhere

abalaji: Are `rightX` and `bottomY` used anywhere
sharveyAuthorUnsubmitted
Done

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.
58 cropDisplayCanvas.requestPaint(); 60​ cropDisplayCanvas.requestPaint();
59 } 61​ }
60 62​
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 delta; // valid for either direction
rkflxUnsubmitted
Done

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) {
rkflxUnsubmitted
Done

Insert space after switch, please. Repeat for all other occurrences.

rkflx: Insert space after `switch`, please. Repeat for all other occurrences.
rkflxUnsubmitted
Not Done

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​ case Qt.Key_Left:
97​ delta = checkBounds(-smallChange, 0, "left");
abalajiUnsubmitted
Done

Change -1 to -1 / Screen.devicePixelRatio

abalaji: Change `-1` to `-1 / Screen.devicePixelRatio`
98​ selection.x = selection.x + delta;
rkflxUnsubmitted
Done

Do you know about the += operator, which would be more concise?

rkflx: Do you know about the `+=` operator, which would be more concise?
99​ break;
100​ case Qt.Key_Right:
101​ delta = checkBounds(smallChange, 0, "right");
abalajiUnsubmitted
Done

Same

abalaji: Same
102​ selection.x = selection.x + delta;
103​ break;
104​ case Qt.Key_Up:
105​ delta = checkBounds(0, -smallChange, "up");
abalajiUnsubmitted
Done

Same

abalaji: Same
106​ selection.y = selection.y + delta;
107​ break;
108​ case Qt.Key_Down:
109​ delta = checkBounds(0, smallChange, "down");
abalajiUnsubmitted
Done

Same

abalaji: Same
110​ selection.y = selection.y + delta;
111​ break;
112​ }
113​
114​ break; // end no modifier (just arrows)
115​
116​ case Qt.ShiftModifier:
117​ switch(event.key) {
118​ case Qt.Key_Left:
119​ delta = checkBounds(-largeChange, 0, "left");
abalajiUnsubmitted
Done

Change -largeChange to -largeChange / Screen.devicePixelRatio

abalaji: Change `-largeChange` to `-largeChange / Screen.devicePixelRatio`
120​ selection.x = selection.x + delta;
121​ break;
122​
123​ case Qt.Key_Right:
124​ delta = checkBounds(largeChange, 0, "right");
abalajiUnsubmitted
Done

I think you get the idea

abalaji: I think you get the idea
125​ selection.x = selection.x + delta;
126​ break;
127​
128​ case Qt.Key_Up:
129​ delta = checkBounds(0, -largeChange, "up");
130​ selection.y = selection.y + delta;
131​ break;
132​
133​ case Qt.Key_Down:
134​ delta = checkBounds(0, largeChange, "down");
135​ selection.y = selection.y + delta;
136​ break;
137​ }
rkflxUnsubmitted
Not Done

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​
139​ break; // end Shift + arrows (large move)
140​
141​ case Qt.AltModifier:
142​ switch(event.key) {
143​ case Qt.Key_Left:
144​ delta = checkBounds(-smallChange, 0, "left");
145​ if (selection.width + delta <= 0) {
146​ selection.width = 0;
147​ } else {
148​ selection.width = selection.width + delta;
149​ }
150​ break;
151​
152​ case Qt.Key_Right:
153​ delta = checkBounds(smallChange, 0, "right");
154​ selection.width = selection.width + delta;
155​ break;
156​
157​ case Qt.Key_Up:
158​ delta = checkBounds(0, -smallChange, "up");
159​ if (selection.height + delta <= 0) {
160​ selection.height = 0;
161​ } else {
162​ selection.height = selection.height + delta;
163​ }
164​ break;
165​
166​ case Qt.Key_Down:
167​ delta = checkBounds(0, smallChange, "down");
168​ selection.height = selection.height + delta;
169​ break;
rkflxUnsubmitted
Done

Alt+↓ is broken (should be fast, not slow)

rkflx: {key Alt down} is broken (should be fast, not slow)
170​ }
171​ break; // end ALT + arrows (resize rectangle)
172​
173​ case(Qt.ShiftModifier + Qt.AltModifier):
rkflxUnsubmitted
Done

Insert space after case.

rkflx: Insert space after `case`.
rkflxUnsubmitted
Not Done

Not yet done.

rkflx: Not yet done.
174​ switch(event.key) {
175​ case Qt.Key_Left:
176​ delta = checkBounds(-largeChange, 0, "left");
177​ if (selection.width + delta <= 0) {
178​ selection.width = 0;
179​ } else {
180​ selection.width = selection.width + delta;
181​ }
182​ break;
183​
184​ case Qt.Key_Right:
185​ delta = checkBounds(largeChange, 0, "right");
186​ selection.width = selection.width + delta;
187​ break;
188​
189​ case Qt.Key_Up:
190​ delta = checkBounds(0, -largeChange, "up");
191​ if (selection.height + delta <= 0) {
192​ selection.height = 0;
193​ } else {
194​ selection.height = selection.height + delta
195​ }
196​ break;
197​
198​ case Qt.Key_Down:
199​ delta = checkBounds(0, largeChange, "down");
200​ selection.height = selection.height + delta;
201​ break;
202​ }
203​ break; // end Shift + ALT + arrows (large resize rectangle)
204​
205​ }
206​
207​ // all switches done; repaint on any keypress
83 cropDisplayCanvas.requestPaint(); 208​ cropDisplayCanvas.requestPaint();
209​
210​ function checkBounds(deltaX, deltaY, direction) {
rkflxUnsubmitted
Not Done

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​ var leftEdge = selection.x;
213​ var rightEdge = selection.x + selection.width;
214​ var topEdge = selection.y;
215​ var bottomEdge = selection.y + selection.height;
216​
217​ const screenMaxX = cropDisplayCanvas.width;
218​ const screenMaxY = cropDisplayCanvas.height;
219​
220​ var newX;
221​ var newY;
222​
223​ var overlap;
224​
225​ newX = selection.x + deltaX;
226​ newY = selection.y + deltaY;
227​
228​ switch (direction) {
229​
230​ case "left":
231​ if (leftEdge + deltaX >= 0) {
232​ return deltaX;
84 } 233​ }
234​
235​ if (leftEdge + deltaX <= 0) {
236​ return -leftEdge;
85 } 237​ }
238​ break;
239​
240​ case "right":
241​ newX = newX + selection.width;
242​ if (newX <= screenMaxX) {
243​ return deltaX;
244​ }
245​ if (newX >= screenMaxX) {
246​ overlap = newX - screenMaxX;
247​ return deltaX - overlap;
248​ }
249​ break;
250​
251​ case "up":
252​ if (topEdge + deltaY >= 0) {
253​ return deltaY;
254​ } else {
255​ return -topEdge;
256​ }
257​
rkflxUnsubmitted
Not Done

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​ case "down":
259​ newY = newY + selection.height;
260​ if (newY <= screenMaxY) {
261​ return deltaY;
262​ }
263​ if (newY >= screenMaxY) {
264​ overlap = newY - screenMaxY;
265​ return deltaY - overlap;
266​ }
267​ break;
268​ }
269​ }
270​ } // end Keys.onPressed
271​
86 272​
87 Keys.onReleased: { 273​ Keys.onReleased: {
88 if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) { 274​ if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) {
89 toggleMagnifier = false; 275​ toggleMagnifier = false;
90 cropDisplayCanvas.requestPaint(); 276​ cropDisplayCanvas.requestPaint();
91 } 277​ }
92 } 278​ }
93 279​
▲ Show 20 Lines • Show All 155 Lines • ▼ Show 20 Line(s)297 Canvas {
249 TextMetrics { 435​ TextMetrics {
250 id: selectionTextMetrics 436​ id: selectionTextMetrics
251 } 437​ }
252 438​
253 Rectangle { 439​ Rectangle {
254 id: midHelpText; 440​ id: midHelpText;
255 objectName: "midHelpText"; 441​ objectName: "midHelpText";
256 442​
257 height: midHelpTextElement.height + 40; 443​ height: midHelpTextElement.height + 20;
258 width: midHelpTextElement.width + 40; 444​ width: midHelpTextElement.width + 20;
259 radius: 4; 445​ radius: 4;
260 border.color: systemPalette.windowText; 446​ border.color: systemPalette.windowText;
261 color: labelBackgroundColour; 447​ color: labelBackgroundColour;
262 448​
263 visible: false; 449​ visible: false;
264 anchors.centerIn: parent; 450​ anchors.centerIn: parent;
265 451​
266 Label { 452​ Label {
267 id: midHelpTextElement; 453​ id: midHelpTextElement;
268 text: i18n("Click anywhere to start drawing a selection rectangle,\n" + 454​ text: i18n("Click anywhere to start drawing a selection rectangle,\n" +
269 "or press Esc to cancel."); 455​ "or press Esc to cancel.");
270 font.pixelSize: Qt.application.font.pixelSize * 1.2; 456​ font.pixelSize: Qt.application.font.pixelSize * 1.2;
271 457​
272 anchors.centerIn: parent; 458​ anchors.centerIn: parent;
273 } 459​ }
274 } 460​ }
275 461​
276 Rectangle { 462​ Rectangle {
277 id: bottomHelpText; 463​ id: bottomHelpText;
278 objectName: "bottomHelpText"; 464​ objectName: "bottomHelpText";
279 465​
280 height: bottomHelpTextElement.height + 20; 466​ height: bottomHelpTextElement.height
281 width: bottomHelpTextElement.width + 20; 467​ width: bottomHelpTextElement.width
282 radius: 4; 468​ radius: 4;
283 border.color: systemPalette.windowText; 469​ border.color: systemPalette.windowText;
284 color: labelBackgroundColour; 470​ color: labelBackgroundColour;
285 471​
286 visible: false; 472​ visible: false;
287 anchors.bottom: parent.bottom; 473​ anchors.bottom: parent.bottom;
288 anchors.horizontalCenter: parent.horizontalCenter; 474​ anchors.horizontalCenter: parent.horizontalCenter;
289 475​
476​ RowLayout {
477​ id: bottomHelpTextElement
478​
290 GridLayout { 479​ GridLayout {
291 id: bottomHelpTextElement; ​
292 columns: 2 480​ columns: 2
293 anchors.centerIn: parent; 481​ Layout.alignment: Qt.AlignLeft && Qt.AlignHCenter
482​ Layout.leftMargin: 10
483​ Layout.topMargin: 5
484​ Layout.bottomMargin: 5
485​ Layout.rightMargin: 10
294 486​
295 Label { 487​ Label {
296 text: i18n("Enter, double-click:"); 488​ text: i18n("Enter, double-click:");
297 Layout.alignment: Qt.AlignRight; 489​ Layout.alignment: Qt.AlignLeft;
298 } 490​ }
299 Label { text: i18n("Take screenshot"); } 491​ Label { text: i18n("Take screenshot"); }
300 492​
301 Label { 493​ Label {
302 text: i18n("Shift:"); 494​ text: i18n("Shift:");
303 Layout.alignment: Qt.AlignRight; 495​ Layout.alignment: Qt.AlignLeft;
304 } 496​ }
305 Label { text: i18n("Hold to toggle magnifier"); } 497​ Label { text: i18n("Hold to toggle magnifier"); }
306 498​
307 Label { 499​ Label {
308 text: i18n("Right-click:"); 500​ text: i18n("Right-click:");
309 Layout.alignment: Qt.AlignRight; 501​ Layout.alignment: Qt.AlignLeft;
310 } 502​ }
311 Label { text: i18n("Reset selection"); } 503​ Label { text: i18n("Reset selection"); }
ngrahamUnsubmitted
Done

"Hove"? :-)

How about "Move selection rectangle"

ngraham: "Hove"? :-) How about "Move selection rectangle"
312 504​
313 Label { 505​ Label {
314 text: i18n("Esc:"); 506​ text: i18n("Esc:");
315 Layout.alignment: Qt.AlignRight; 507​ Layout.alignment: Qt.AlignLeft;
rkflxUnsubmitted
Done

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

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

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 } 508​ }
317 Label { text: i18n("Cancel"); } 509​ Label { text: i18n("Cancel"); }
ngrahamUnsubmitted
Done

How about "Move selection rectangle <number> pixels"?

ngraham: How about "Move selection rectangle <number> pixels"?
ngrahamUnsubmitted
Done

How about taking the value from largeChange above?

ngraham: How about taking the value from `largeChange` above?
510​
511​ Label {
512​ text: i18n("Arrow Keys:");
rkflxUnsubmitted
Done

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"?
513​ Layout.alignment: Qt.AlignLeft && Qt.AlignTop;
rkflxUnsubmitted
Done

Do you mean | instead of && by any chance?

rkflx: Do you mean `|` instead of `&&` by any chance?
514​ }
515​ Label { text: i18n("Move selection rectangle \n" +
ngrahamUnsubmitted
Done

etc.

ngraham: etc.
ngrahamUnsubmitted
Done

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…
516​ "Alt to resize, Shift for %1px change", largeChange); }
ngrahamUnsubmitted
Done

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

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

"Arrow keys" could go after "Shift"

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

"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…
318 } 517​ }
319 } 518​ }
519​ }
520​
320 521​
ngrahamUnsubmitted
Done

etc.

ngraham: etc.
321 // Use Rectangle so that the background is white when cursor nearby edge 522​ // Use Rectangle so that the background is white when cursor nearby edge
322 Rectangle { 523​ Rectangle {
323 id: crossMagnifier; 524​ id: crossMagnifier;
324 525​
325 height: (magPixels * 2 + 1) * magZoom; 526​ height: (magPixels * 2 + 1) * magZoom;
326 width: height; 527​ width: height;
327 border.width: 0; 528​ border.width: 0;
328 visible: false; 529​ visible: false;
Show All 34 Lines
363 Rectangle { 564​ Rectangle {
364 x: (magPixels + 1) * magZoom; 565​ x: (magPixels + 1) * magZoom;
365 y: magPixels * magZoom; 566​ y: magPixels * magZoom;
366 width: magPixels * magZoom; 567​ width: magPixels * magZoom;
367 height: magZoom; 568​ height: magZoom;
368 color: crossColour; 569​ color: crossColour;
369 } 570​ }
370 } 571​ }
371 ​
rkflxUnsubmitted
Not Done

Unrelated whitespace change.

rkflx: Unrelated whitespace change.
372 } 572​ }
373 573​
374 MouseArea { 574​ MouseArea {
375 anchors.fill: imageBackground; 575​ anchors.fill: imageBackground;
376 576​
377 property int startx: 0; 577​ property int startx: 0;
378 property int starty: 0; 578​ property int starty: 0;
379 579​
▲ Show 20 Lines • Show All 54 Lines • Show Last 20 Lines