Differential D13450 Diff 38014 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 double largeChange: 15;
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 double 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​
58 cropDisplayCanvas.requestPaint(); 60​ cropDisplayCanvas.requestPaint();
59 } 61​ }
60 62​
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.
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;
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​ change = checkBounds(-largeChange, 0.0, "left");
abalajiUnsubmitted
Done

Change -1 to -1 / Screen.devicePixelRatio

abalaji: Change `-1` to `-1 / Screen.devicePixelRatio`
98​ selection.x += change;
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​ change = checkBounds(largeChange, 0.0, "right");
abalajiUnsubmitted
Done

Same

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

Same

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

Same

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

Change -largeChange to -largeChange / Screen.devicePixelRatio

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

I think you get the idea

abalaji: I think you get the idea
125​ change = checkBounds(smallChange, 0.0, "right");
126​ selection.x += change;
127​ break;
128​
129​ case Qt.Key_Up:
130​ change = checkBounds(0.0, -smallChange, "up");
131​ selection.y += change;
132​ break;
133​
134​ case Qt.Key_Down:
135​ change = checkBounds(0.0, smallChange, "down");
136​ selection.y += change;
137​ break;
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​
140​ break; // end Shift + arrows (large move)
141​
142​ case Qt.AltModifier:
143​ switch(event.key) {
144​
145​ case Qt.Key_Left:
146​ change = checkBounds(-largeChange, 0.0, "left");
147​ if (selection.width + change <= 0.0) {
148​ selection.width = 0.0;
149​ } else {
150​ selection.width += change;
151​ }
152​ break;
153​
154​ case Qt.Key_Right:
155​ change = checkBounds(largeChange, 0.0, "right");
156​ selection.width += change;
157​ break;
158​
159​ case Qt.Key_Up:
160​ change = checkBounds(0.0, -largeChange, "up");
161​ if (selection.height + change <= 0.0) {
162​ selection.height = 0.0;
163​ } else {
164​ selection.height += change;
165​ }
166​ break;
167​
168​ case Qt.Key_Down:
169​ change = checkBounds(0.0, smallChange, "down");
rkflxUnsubmitted
Done

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

rkflx: {key Alt down} is broken (should be fast, not slow)
170​ selection.height = selection.height + change;
171​ break;
172​ }
173​
rkflxUnsubmitted
Done

Insert space after case.

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

Not yet done.

rkflx: Not yet done.
174​ break; // end ALT + arrows (resize rectangle)
175​
176​ case(Qt.ShiftModifier + Qt.AltModifier):
177​ switch(event.key) {
178​ case Qt.Key_Left:
179​ change = checkBounds(-smallChange, 0.0, "left");
180​ if (selection.width + change <= 0.0) {
181​ selection.width = 0.0;
182​ } else {
183​ selection.width += change;
184​ }
185​ break;
186​
187​ case Qt.Key_Right:
188​ change = checkBounds(smallChange, 0.0, "right");
189​ selection.width += change;
190​ break;
191​
192​ case Qt.Key_Up:
193​ change = checkBounds(0.0, -smallChange, "up");
194​ if (selection.height + change <= 0.0) {
195​ selection.height = 0.0;
196​ } else {
197​ selection.height += change
198​ }
199​ break;
200​
201​ case Qt.Key_Down:
202​ change = checkBounds(0.0, smallChange, "down");
203​ selection.height += change;
204​ break;
205​ }
206​
207​ break; // end Shift + ALT + arrows (large resize rectangle)
208​
209​ }
210​
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​ // all switches done; repaint on any keypress
83 cropDisplayCanvas.requestPaint(); 212​ cropDisplayCanvas.requestPaint();
213​
214​ function checkBounds(changeX, changeY, direction) {
215​
216​ var leftEdge = selection.x;
217​ var rightEdge = selection.x + selection.width;
218​ var topEdge = selection.y;
219​ var bottomEdge = selection.y + selection.height;
220​
221​ const screenMaxX = cropDisplayCanvas.width;
222​ const screenMaxY = cropDisplayCanvas.height;
223​
224​ var newX;
225​ var newY;
226​
227​ var overlap;
228​
229​ newX = selection.x + changeX;
230​ newY = selection.y + changeY;
231​
232​ switch (direction) {
233​
234​ case "left":
235​ if (leftEdge + changeX > 0.0) {
236​ return changeX;
237​ }
238​
239​ if (leftEdge + changeX < 0.0) {
240​ return -leftEdge;
84 } 241​ }
242​ break;
243​
244​ case "right":
245​ newX = newX + selection.width;
246​ if (newX < screenMaxX) {
247​ return changeX;
248​ }
249​ if (newX > screenMaxX) {
250​ overlap = newX - screenMaxX;
251​ return changeX - overlap;
85 } 252​ }
253​ break;
254​
255​ case "up":
256​ if (topEdge + changeY > 0.0) {
257​ return changeY;
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​ } else {
259​ return -topEdge;
260​ }
261​
262​ case "down":
263​ newY = newY + selection.height;
264​ if (newY < screenMaxY) {
265​ return changeY;
266​ }
267​ if (newY > screenMaxY) {
268​ overlap = newY - screenMaxY;
269​ return changeY - overlap;
270​ }
271​ break;
272​
273​ }
274​ }
275​ } // end Keys.onPressed
276​
86 277​
87 Keys.onReleased: { 278​ Keys.onReleased: {
88 if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) { 279​ if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) {
89 toggleMagnifier = false; 280​ toggleMagnifier = false;
90 cropDisplayCanvas.requestPaint(); 281​ cropDisplayCanvas.requestPaint();
91 } 282​ }
92 } 283​ }
93 284​
▲ Show 20 Lines • Show All 155 Lines • ▼ Show 20 Line(s)302 Canvas {
249 TextMetrics { 440​ TextMetrics {
250 id: selectionTextMetrics 441​ id: selectionTextMetrics
251 } 442​ }
252 443​
253 Rectangle { 444​ Rectangle {
254 id: midHelpText; 445​ id: midHelpText;
255 objectName: "midHelpText"; 446​ objectName: "midHelpText";
256 447​
257 height: midHelpTextElement.height + 40; 448​ height: midHelpTextElement.height + 20;
258 width: midHelpTextElement.width + 40; 449​ width: midHelpTextElement.width + 20;
259 radius: 4; 450​ radius: 4;
260 border.color: systemPalette.windowText; 451​ border.color: systemPalette.windowText;
261 color: labelBackgroundColour; 452​ color: labelBackgroundColour;
262 453​
263 visible: false; 454​ visible: false;
264 anchors.centerIn: parent; 455​ anchors.centerIn: parent;
265 456​
266 Label { 457​ Label {
267 id: midHelpTextElement; 458​ id: midHelpTextElement;
268 text: i18n("Click anywhere to start drawing a selection rectangle,\n" + 459​ text: i18n("Click anywhere to start drawing a selection rectangle,\n" +
269 "or press Esc to cancel."); 460​ "or press Esc to cancel.");
270 font.pixelSize: Qt.application.font.pixelSize * 1.2; 461​ font.pixelSize: Qt.application.font.pixelSize * 1.2;
271 462​
272 anchors.centerIn: parent; 463​ anchors.centerIn: parent;
273 } 464​ }
274 } 465​ }
275 466​
276 Rectangle { 467​ Rectangle {
277 id: bottomHelpText; 468​ id: bottomHelpText;
278 objectName: "bottomHelpText"; 469​ objectName: "bottomHelpText";
279 470​
280 height: bottomHelpTextElement.height + 20; 471​ height: bottomHelpTextElement.height
281 width: bottomHelpTextElement.width + 20; 472​ width: bottomHelpTextElement.width
282 radius: 4; 473​ radius: 4;
283 border.color: systemPalette.windowText; 474​ border.color: systemPalette.windowText;
284 color: labelBackgroundColour; 475​ color: labelBackgroundColour;
285 476​
286 visible: false; 477​ visible: false;
287 anchors.bottom: parent.bottom; 478​ anchors.bottom: parent.bottom;
288 anchors.horizontalCenter: parent.horizontalCenter; 479​ anchors.horizontalCenter: parent.horizontalCenter;
289 480​
481​ RowLayout {
482​ id: bottomHelpTextElement
483​
290 GridLayout { 484​ GridLayout {
291 id: bottomHelpTextElement; ​
292 columns: 2 485​ columns: 2
293 anchors.centerIn: parent; 486​ Layout.alignment: Qt.AlignLeft && Qt.AlignHCenter
487​ Layout.leftMargin: 10
488​ Layout.topMargin: 5
489​ Layout.bottomMargin: 5
490​ Layout.rightMargin: 10
294 491​
295 Label { 492​ Label {
296 text: i18n("Enter, double-click:"); 493​ text: i18n("Enter, double-click:");
297 Layout.alignment: Qt.AlignRight; 494​ Layout.alignment: Qt.AlignRight;
298 } 495​ }
299 Label { text: i18n("Take screenshot"); } 496​ Label { text: i18n("Take screenshot"); }
300 497​
301 Label { 498​ Label {
302 text: i18n("Shift:"); 499​ text: i18n("Shift:");
303 Layout.alignment: Qt.AlignRight; 500​ Layout.alignment: Qt.AlignRight;
304 } 501​ }
305 Label { text: i18n("Hold to toggle magnifier"); } 502​ Label { text: i18n("Hold to toggle magnifier"); }
306 503​
307 Label { 504​ Label {
308 text: i18n("Right-click:"); 505​ text: i18n("Right-click:");
309 Layout.alignment: Qt.AlignRight; 506​ Layout.alignment: Qt.AlignRight;
310 } 507​ }
311 Label { text: i18n("Reset selection"); } 508​ Label { text: i18n("Reset selection"); }
ngrahamUnsubmitted
Done

"Hove"? :-)

How about "Move selection rectangle"

ngraham: "Hove"? :-) How about "Move selection rectangle"
312 509​
510​
511​ Label {
512​ text: i18n("Arrow keys:");
513​ Layout.alignment: Qt.AlignRight | Qt.AlignTop;
514​ }
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?
515​ Label { text: i18n("Move selection rectangle \n" +
516​ "Hold Alt to resize, Shift to fine-tune"); }
517​
313 Label { 518​ Label {
314 text: i18n("Esc:"); 519​ text: i18n("Esc:");
315 Layout.alignment: Qt.AlignRight; 520​ Layout.alignment: Qt.AlignRight;
ngrahamUnsubmitted
Done

etc.

ngraham: etc.
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 } 521​ }
317 Label { text: i18n("Cancel"); } 522​ Label { text: i18n("Cancel"); }
318 } 523​ }
319 } 524​ }
525​ }
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"?
526​
ngrahamUnsubmitted
Done

etc.

ngraham: etc.
rkflxUnsubmitted
Done

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

rkflx: Do you mean `|` instead of `&&` by any chance?
320 527​
321 // Use Rectangle so that the background is white when cursor nearby edge 528​ // Use Rectangle so that the background is white when cursor nearby edge
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…
322 Rectangle { 529​ Rectangle {
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…
323 id: crossMagnifier; 530​ id: crossMagnifier;
324 531​
325 height: (magPixels * 2 + 1) * magZoom; 532​ height: (magPixels * 2 + 1) * magZoom;
326 width: height; 533​ width: height;
327 border.width: 0; 534​ border.width: 0;
328 visible: false; 535​ visible: false;
329 clip: true 536​ clip: true
330 537​
Show All 32 Lines
363 Rectangle { 570​ Rectangle {
364 x: (magPixels + 1) * magZoom; 571​ x: (magPixels + 1) * magZoom;
365 y: magPixels * magZoom; 572​ y: magPixels * magZoom;
366 width: magPixels * magZoom; 573​ width: magPixels * magZoom;
367 height: magZoom; 574​ height: magZoom;
368 color: crossColour; 575​ color: crossColour;
369 } 576​ }
370 } 577​ }
371 578​
rkflxUnsubmitted
Not Done

Unrelated whitespace change.

rkflx: Unrelated whitespace change.
372 } 579​ }
373 580​
374 MouseArea { 581​ MouseArea {
375 anchors.fill: imageBackground; 582​ anchors.fill: imageBackground;
376 583​
377 property int startx: 0; 584​ property int startx: 0;
378 property int starty: 0; 585​ property int starty: 0;
379 586​
▲ Show 20 Lines • Show All 54 Lines • Show Last 20 Lines