Changeset View
Standalone View
src/QuickEditor/EditorRoot.qml
Show All 34 Lines | 25 | Item { | |||
---|---|---|---|---|---|
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 | ||||
rkflx: Good code design avoids as much global state as possible. This variable is only really relevant… | |||||
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) { | ||
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… | |||||
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; | ||
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; | ||||
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… | |||||
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… | |||||
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; | ||||
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); | ||
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 349 Lines • Show Last 20 Lines |
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.