Differential D14252 Diff 38364 src/QuickEditor/EditorRoot.qml

Changeset View

Standalone View

View Options

src/QuickEditor/EditorRoot.qml

Show All 34 Lines25Item {
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; 41​ property double largeChange: 15;
42 property double smallChange: 1 / Screen.devicePixelRatio; 42​ property double smallChange: 1 / Screen.devicePixelRatio;
43​ property bool resize: false; // toggle for resize versus move actions
rkflxUnsubmitted
Not Done

Good code design avoids as much global state as possible. This variable is only really relevant for the bounds checking, though. See also the other inline comment.

rkflx: Good code design avoids as much global state as possible. This variable is only really relevant…
sharveyAuthorUnsubmitted
Not Done

I'm going to beg off this error as "knowing the logic, but not the style."

I'll rework the patch without it.

Thanks for another entry in my C++ style notebook.

sharvey: I'm going to beg off this error as "knowing the logic, but not the style." I'll rework the…
43 44​
44 SystemPalette { 45​ SystemPalette {
45 id: systemPalette; 46​ id: systemPalette;
46 } 47​ }
47 48​
48 function setInitialSelection(xx, yy, ww, hh) { 49​ function setInitialSelection(xx, yy, ww, hh) {
49 if (selection) { 50​ if (selection) {
50 selection.destroy(); 51​ selection.destroy();
Show All 25 Lines
76 Keys.onEnterPressed: accept() 77​ Keys.onEnterPressed: accept()
77 78​
78 Keys.onEscapePressed: { 79​ Keys.onEscapePressed: {
79 cancelImage(); 80​ cancelImage();
80 } 81​ }
81 82​
82 Keys.onPressed: { 83​ Keys.onPressed: {
83 84​
84 var change; 85​ const screenMaxX = cropDisplayCanvas.width;
86​ const screenMaxY = cropDisplayCanvas.height;
87​ const minRectHeight = 20;
88​ const minRectWidth = 20;
85 89​
86 // shift key alone = magnifier toggle 90​ // shift key alone = magnifier toggle
87 if (event.modifiers & Qt.ShiftModifier) { 91​ if (event.modifiers & Qt.ShiftModifier) {
88 toggleMagnifier = true; 92​ toggleMagnifier = true;
89 } 93​ }
90 94​
91 // nested switches for arrow keys based on modifier keys 95​ // nested switches for arrow keys based on modifier keys
92 switch(event.modifiers) { 96​ switch(event.modifiers) {
93 97​
94 case Qt.NoModifier: 98​ case Qt.NoModifier:
99​ resize = false;
95 switch (event.key) { 100​ switch (event.key) {
96 101​
97 case Qt.Key_Left: 102​ case Qt.Key_Left:
98 change = checkBounds(-largeChange, 0.0, "left"); 103​ moveSizeRect(-largeChange, 0.0, "left");
99 selection.x += change; ​
100 break; 104​ break;
101 case Qt.Key_Right: 105​ case Qt.Key_Right:
102 change = checkBounds(largeChange, 0.0, "right"); 106​ moveSizeRect(largeChange, 0.0, "right");
103 selection.x += change; ​
104 break; 107​ break;
105 case Qt.Key_Up: 108​ case Qt.Key_Up:
106 change = checkBounds(0.0, -largeChange, "up"); 109​ moveSizeRect(0.0, -largeChange, "up");
107 selection.y += change; ​
108 break; 110​ break;
109 case Qt.Key_Down: 111​ case Qt.Key_Down:
110 change = checkBounds(0.0, largeChange, "down"); 112​ moveSizeRect(0.0, largeChange, "down");
111 selection.y += change; ​
112 break; 113​ break;
113 } 114​ }
114 115​
115 break; // end no modifier (just arrows) 116​ break; // end no modifier (just arrows - large move)
116 117​
117 case Qt.ShiftModifier: 118​ case Qt.ShiftModifier:
119​ resize = false;
118 switch (event.key) { 120​ switch (event.key) {
119 121​
120 case Qt.Key_Left: 122​ case Qt.Key_Left:
121 change = checkBounds(-smallChange, 0.0, "left"); 123​ moveSizeRect(-smallChange, 0.0, "left");
122 selection.x += change; ​
123 break; 124​ break;
124 125​
125 case Qt.Key_Right: 126​ case Qt.Key_Right:
126 change = checkBounds(smallChange, 0.0, "right"); 127​ moveSizeRect(smallChange, 0.0, "right");
127 selection.x += change; ​
128 break; 128​ break;
129 129​
130 case Qt.Key_Up: 130​ case Qt.Key_Up:
131 change = checkBounds(0.0, -smallChange, "up"); 131​ moveSizeRect(0.0, -smallChange, "up");
132 selection.y += change; ​
133 break; 132​ break;
134 133​
135 case Qt.Key_Down: 134​ case Qt.Key_Down:
136 change = checkBounds(0.0, smallChange, "down"); 135​ moveSizeRect(0.0, smallChange, "down");
137 selection.y += change; ​
138 break; 136​ break;
139 } 137​ }
140 138​
141 break; // end Shift + arrows (large move) 139​ break; // end Shift + arrows (small move)
142 140​
143 case Qt.AltModifier: 141​ case Qt.AltModifier:
142​ resize = true;
144 switch (event.key) { 143​ switch (event.key) {
145 144​
146 case Qt.Key_Left: 145​ case Qt.Key_Left:
147 change = checkBounds(-largeChange, 0.0, "left"); 146​ moveSizeRect(-largeChange, 0.0, "left");
148 if (selection.width + change <= 0.0) { ​
149 selection.width = 0.0; ​
150 } else { ​
151 selection.width += change; ​
152 } ​
153 break; 147​ break;
154 148​
155 case Qt.Key_Right: 149​ case Qt.Key_Right:
156 change = checkBounds(largeChange, 0.0, "right"); 150​ moveSizeRect(largeChange, 0.0, "right");
157 selection.width += change; ​
158 break; 151​ break;
159 152​
160 case Qt.Key_Up: 153​ case Qt.Key_Up:
161 change = checkBounds(0.0, -largeChange, "up"); 154​ moveSizeRect(0.0, -largeChange, "up");
162 if (selection.height + change <= 0.0) { ​
163 selection.height = 0.0; ​
164 } else { ​
165 selection.height += change; ​
166 } ​
167 break; 155​ break;
168 156​
169 case Qt.Key_Down: 157​ case Qt.Key_Down:
170 change = checkBounds(0.0, largeChange, "down"); 158​ moveSizeRect(0.0, largeChange, "down");
171 selection.height = selection.height + change; ​
172 break; 159​ break;
173 } 160​ }
174 161​
175 break; // end ALT + arrows (resize rectangle) 162​ break; // end ALT + arrows (resize rectangle - large change)
176 163​
177 case (Qt.ShiftModifier + Qt.AltModifier): 164​ case (Qt.ShiftModifier + Qt.AltModifier):
165​ resize = true;
178 switch (event.key) { 166​ switch (event.key) {
179 167​
180 case Qt.Key_Left: 168​ case Qt.Key_Left:
181 change = checkBounds(-smallChange, 0.0, "left"); 169​ moveSizeRect(-smallChange, 0.0, "left");
182 if (selection.width + change <= 0.0) { ​
183 selection.width = 0.0; ​
184 } else { ​
185 selection.width += change; ​
186 } ​
187 break; 170​ break;
188 171​
189 case Qt.Key_Right: 172​ case Qt.Key_Right:
190 change = checkBounds(smallChange, 0.0, "right"); 173​ moveSizeRect(smallChange, 0.0, "right");
191 selection.width += change; ​
192 break; 174​ break;
193 175​
194 case Qt.Key_Up: 176​ case Qt.Key_Up:
195 change = checkBounds(0.0, -smallChange, "up"); 177​ moveSizeRect(0.0, -smallChange, "up");
196 if (selection.height + change <= 0.0) { ​
197 selection.height = 0.0; ​
198 } else { ​
199 selection.height += change ​
200 } ​
201 break; 178​ break;
202 179​
203 case Qt.Key_Down: 180​ case Qt.Key_Down:
204 change = checkBounds(0.0, smallChange, "down"); 181​ moveSizeRect(0.0, smallChange, "down");
205 selection.height += change; ​
206 break; 182​ break;
207 } 183​ }
208 184​
209 break; // end Shift + ALT + arrows (large resize rectangle) 185​ break; // end Shift + ALT + arrows (small resize rectangle)
210 186​
211 } 187​ }
212 188​
213 // all switches done; repaint on any keypress ​
214 cropDisplayCanvas.requestPaint(); ​
215 ​
216 function checkBounds(changeX, changeY, direction) { ​
217 ​
218 var leftEdge = selection.x; ​
219 var rightEdge = selection.x + selection.width; ​
220 var topEdge = selection.y; ​
221 var bottomEdge = selection.y + selection.height; ​
222 189​
223 const screenMaxX = cropDisplayCanvas.width; 190​ function moveSizeRect(changeX, changeY, direction) {
224 const screenMaxY = cropDisplayCanvas.height; ​
225 191​
192​ var action;
226 var newX; 193​ var newX;
227 var newY; 194​ var newY;
195​ var newRight;
196​ var newBottom;
228 197​
229 var overlap; 198​ switch (resize) {
rkflxUnsubmitted
Not Done

A switch for a bool looks really weird. Normally for bools an if is used, but what's better here is probably to make the action a (string) parameter of the function and keep the switch.

Even better would be to write two functions (one for each action, i.e. move or resize).

rkflx: A `switch` for a `bool` looks really weird. Normally for `bools` an `if` is used, but what's…
sharveyAuthorUnsubmitted
Not Done

My goal in writing one function was to avoid duplication. A lot of the code will be the same (checking edges, etc). I will rework the function to use resize as a string.

sharvey: My goal in writing one function was to avoid duplication. A lot of the code will be the same…
230 199​ case false: // move
231 newX = selection.x + changeX; ​
232 newY = selection.y + changeY; ​
233 200​
234 switch (direction) { 201​ switch (direction) {
235 ​
236 case "left": 202​ case "left":
237 if (leftEdge + changeX > 0.0) { 203​ newX = selection.x += changeX;
238 return changeX; 204​ action = (newX >= 0.0) ? selection.x = newX : selection.x = 0.0;
rkflxUnsubmitted
Not Done

That's a bit difficult to read. Would the following simpler code also do what you want?

selection.x += changeX;
if (selection.x < 0.0) {
  selection.x = 0.0;
}
rkflx: That's a bit difficult to read. Would the following simpler code also do what you want? ```…
205​ break;
206​ case "right":
207​ newRight = (selection.x + selection.width) + changeX;
208​ action = (newRight <= screenMaxX) ? selection.x += changeX : selection.x = screenMaxX - selection.width;
209​ break;
210​ case "up":
211​ newY = selection.y += changeY;
212​ action = (newY >= 0.0) ? selection.y = newY : selection.y = 0.0;
213​ break;
214​ case "down":
215​ newBottom = selection.y + selection.height + changeY;
216​ action = (newBottom <= screenMaxY) ? selection.y = selection.y + changeY : selection.y = screenMaxY - selection.height;
sharveyAuthorUnsubmitted
Not Done

The use of action = with the ternary operator is to quiet QtCreator. According to the Javascript/ECMAScript standards, a ternary operator can be used without a variable assignment, but QtCreator clutters up the editor with warnings. action never gets used, except as a placeholder.

sharvey: The use of `action =` with the ternary operator is to quiet QtCreator. According to the…
rkflxUnsubmitted
Not Done

Hmh, in general adding workarounds to the code to avoid warnings in an editor is an anti-pattern. Qt Creator might not be perfect, but often enough the warnings have a point.

There would be no warning if you wrote that line like this:

selection.y = newBottom <= screenMaxY ? selection.y + changeY
                                      : screenMaxY - selection.height;
rkflx: Hmh, in general adding workarounds to the code to avoid warnings in an editor is an anti…
sharveyAuthorUnsubmitted
Not Done

I confess I never felt right about the Qt Creator kludge. Thanks for the guidance. I will try to avoid "anti-patterns" going forward.

sharvey: I confess I never felt right about the Qt Creator kludge. Thanks for the guidance. I will try…
217​ break;
239 } 218​ }
240 219​
241 if (leftEdge + changeX < 0.0) { 220​ break;
242 return -leftEdge; 221​ // end of movement switch cases
222​
223​
224​ case true: // resizing rectangle
225​
226​ switch (direction) {
227​ case "left":
228​ newX = selection.width += changeX;
229​ if (newX <= minRectWidth) {
230​ selection.width = minRectWidth;
231​ break;
243 } 232​ }
233​ action = (newX >= 0.0) ? selection.width + changeX : selection.x = 0.0;
244 break; 234​ break;
245 235​
246 case "right": 236​ case "right":
247 newX = newX + selection.width; 237​ newX = selection.width += changeX
248 if (newX < screenMaxX) { 238​ action = (newX <= screenMaxX) ? selection.width + changeX : selection.width = (selection.x + screenMaxX);
249 return changeX; ​
250 } ​
251 if (newX > screenMaxX) { ​
252 overlap = newX - screenMaxX; ​
253 return changeX - overlap; ​
254 } ​
255 break; 239​ break;
256 240​
257 case "up": 241​ case "up":
258 if (topEdge + changeY > 0.0) { 242​ newY = selection.height += changeY;
259 return changeY; 243​ if (newY <= minRectHeight) {
260 } else { 244​ selection.height = minRectHeight;
261 return -topEdge; 245​ break;
262 } 246​ }
247​ action = (newY >= 0.0) ? selection.height = newY : selection.y = 0.0;
rkflxUnsubmitted
Not Done

Aren't you adding changeY two times to selection.height here?

rkflx: Aren't you adding `changeY` two times to `selection.height` here?
248​ break;
263 249​
264 case "down": 250​ case "down":
265 newY = newY + selection.height; 251​ newY = selection.height += changeY;
266 if (newY < screenMaxY) { 252​ action = (newY <= screenMaxY) ? selection.height + changeY : selection.height = (selection.y + screenMaxY);
rkflxUnsubmitted
Not Done

Note that selection.height + changeY will be assigned to action, which is not used later on, making calculating the sum a bit pointless in the first place.

rkflx: Note that `selection.height + changeY` will be assigned to `action`, which is not used later on…
267 return changeY; ​
268 } ​
269 if (newY > screenMaxY) { ​
270 overlap = newY - screenMaxY; ​
271 return changeY - overlap; ​
272 } ​
273 break; 253​ break;
254​ }
274 255​
275 } 256​ }
276 } 257​ }
258​
259​ // all switches done; repaint on any keypress
260​ cropDisplayCanvas.requestPaint();
261​
277 } // end Keys.onPressed 262​ } // end Keys.onPressed
278 263​
279 264​
280 Keys.onReleased: { 265​ Keys.onReleased: {
281 if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) { 266​ if (toggleMagnifier && !(event.modifiers & Qt.ShiftModifier)) {
282 toggleMagnifier = false; 267​ toggleMagnifier = false;
283 cropDisplayCanvas.requestPaint(); 268​ cropDisplayCanvas.requestPaint();
284 } 269​ }
▲ Show 20 Lines • Show All 348 Lines • Show Last 20 Lines