Differential D13450 Diff 37903 src/QuickEditor/EditorRoot.qml

Changeset View

Standalone View

src/QuickEditor/EditorRoot.qml

Show All 32 Lines		25	Item {
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/resize 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