Differential D4751 Diff 11922 applet/contents/ui/ListItemBase.qml

Changeset View

Standalone View

applet/contents/ui/ListItemBase.qml

Show First 20 Lines • Show All 89 Lines • ▼ Show 20 Line(s)		88	MouseArea {
90	cursorShape: dragArea.enabled ? (pressed ? Qt.ClosedHandCursor : Qt.OpenHandCursor) : undefined	90		cursorShape: dragArea.enabled ? (pressed ? Qt.ClosedHandCursor : Qt.OpenHandCursor) : undefined
91	}	91		}
92	}	92		}
93	}	93		}
94		94
95	ColumnLayout {	95		ColumnLayout {
96	id: column	96		id: column
97		97
		98		RowLayout {
		99		Layout.fillWidth: true
		100
98	PlasmaExtras.Heading {	101		PlasmaExtras.Heading {
99	id :textLabel	102		id: textLabel
100	Layout.fillWidth: true	103		Layout.fillWidth: true
101	level: 5	104		level: 5
102	opacity: 0.6	105		opacity: 0.6
103	wrapMode: Text.NoWrap	106		wrapMode: Text.NoWrap
104	elide: Text.ElideRight	107		elide: Text.ElideRight
105	}	108		}
		109		PlasmaComponents.ToolButton {
		110		id: contextMenuButton
		111		Layout.preferredHeight: slider.height
		112		Layout.preferredWidth: Layout.preferredHeight
		113		checkable: true
		114		iconName: "application-menu"
		115		onClicked: contextMenu.show(x, y + height)
		116		}
			droscaUnsubmitted Done Add Layout.fillWidth: true drosca: Add Layout.fillWidth: true
		117		}
106		118
107	RowLayout {	119		RowLayout {
108	PlasmaCore.IconItem {	120		PlasmaCore.IconItem {
109	readonly property bool isPlayback: type.substring(0, 4) == "sink"	121		readonly property bool isPlayback: type.substring(0, 4) == "sink"
110	Layout.maximumHeight: slider.height * 0.85	122		Layout.maximumHeight: slider.height * 0.85
111	Layout.maximumWidth: slider.height * 0.85	123		Layout.maximumWidth: slider.height * 0.85
112	source: Icon.name(Volume, Muted, isPlayback ? "audio-volume" : "microphone-sensitivity")	124		source: Icon.name(Volume, Muted, isPlayback ? "audio-volume" : "microphone-sensitivity")
113		125
114	MouseArea {	126		MouseArea {
115	anchors.fill: parent	127		anchors.fill: parent
116	onPressed: Muted = !Muted	128		onPressed: Muted = !Muted
117	}	129		}
118	}	130		}
119		131
120	PlasmaComponents.Slider {	132		PlasmaComponents.Slider {
121	id: slider	133		id: slider
122		134
123	// Helper properties to allow async slider updates.	135		// Helper properties to allow async slider updates.
			droscaUnsubmitted Done As @Zren pointed out, please change it to ToolButton. This is actually a button, so there is no reason to make it IconItem + MouseArea. Also as it is now, it breaks accessibility. drosca: As @Zren pointed out, please change it to ToolButton. This is actually a button, so there is no…
			romanggAuthorUnsubmitted Done How does it break accessibility? romangg: How does it break accessibility?
			droscaUnsubmitted Done You would need to manually set Accessible.role to Accessible.Button and other properties, but that's not really the main issue I have with that. This is clickable button, so it should be (and look like) button Item, for the consistency sake. drosca: You would need to manually set Accessible.role to Accessible.Button and other properties, but…
			broulikUnsubmitted Done Missing `Accessible` Cannot be tabbed to Doesn't have a proper hover effect Has no tooltip etc etc broulik: * Missing `Accessible` * Cannot be tabbed to * Doesn't have a proper hover effect * Has no…
124	// While we are sliding we must not react to value updates	136		// While we are sliding we must not react to value updates
125	// as otherwise we can easily end up in a loop where value	137		// as otherwise we can easily end up in a loop where value
			droscaUnsubmitted Done Use Layout.preferredHeight instead of maximumHeight. drosca: Use Layout.preferredHeight instead of maximumHeight.
126	// changes trigger volume changes trigger value changes.	138		// changes trigger volume changes trigger value changes.
127	property int volume: Volume	139		property int volume: Volume
128	property bool ignoreValueChange: true	140		property bool ignoreValueChange: true
129		141
130	Layout.fillWidth: true	142		Layout.fillWidth: true
131	minimumValue: PulseAudio.MinimalVolume	143		minimumValue: PulseAudio.MinimalVolume
132	maximumValue: maxVolumeValue	144		maximumValue: maxVolumeValue
133	stepSize: maximumValue / maxVolumePercent	145		stepSize: maximumValue / maxVolumePercent
			broulikUnsubmitted Done Don't mess with the opacity, use the `active` property on `IconItem` which will have `KIconLoader` use the correct hover state (you could have configured it to tint the icon instead etc) broulik: Don't mess with the opacity, use the `active` property on `IconItem` which will have…
134	visible: HasVolume	146		visible: HasVolume
135	enabled: VolumeWritable	147		enabled: VolumeWritable
136	opacity: Muted ? 0.5 : 1	148		opacity: Muted ? 0.5 : 1
			droscaUnsubmitted Done parent.opacity: mouseArea.containsMouse ? 0.8 : 1.0 drosca: parent.opacity: mouseArea.containsMouse ? 0.8 : 1.0
137		149
138	Component.onCompleted: {	150		Component.onCompleted: {
139	ignoreValueChange = false;	151		ignoreValueChange = false;
140	}	152		}
141		153
142	onVolumeChanged: {	154		onVolumeChanged: {
143	var oldIgnoreValueChange = ignoreValueChange;	155		var oldIgnoreValueChange = ignoreValueChange;
144	ignoreValueChange = true;	156		ignoreValueChange = true;
▲ Show 20 Lines • Show All 68 Lines • ▼ Show 20 Line(s)
213	}	225		}
214		226
215	MouseArea {	227		MouseArea {
216	anchors.fill: parent	228		anchors.fill: parent
217	acceptedButtons: Qt.MiddleButton	229		acceptedButtons: Qt.MiddleButton
218	onClicked: Muted = !Muted	230		onClicked: Muted = !Muted
219	}	231		}
220	}	232		}
		233
		234		PlasmaComponents.ContextMenu {
		235		id: contextMenu
		236
		237		onStatusChanged: {
			droscaUnsubmitted Done I would prefer if you rewrite it to normal `if`, it's not clear what this is doing at the first look. Or maybe in ToolButton use `checked: contextMenu.status == PlasmaComponents.DialogStatus.Open`, not sure if it will work though. drosca: I would prefer if you rewrite it to normal `if`, it's not clear what this is doing at the first…
		238		if (status == PlasmaComponents.DialogStatus.Closed) {
		239		contextMenuButton.checked = false;
		240		}
		241		}
		242
		243		function newSeperator() {
		244		return Qt.createQmlObject("import org.kde.plasma.components 2.0 as PlasmaComponents; PlasmaComponents.MenuItem { separator: true }", contextMenu);
		245		}
		246		function newMenuItem() {
		247		return Qt.createQmlObject("import org.kde.plasma.components 2.0 as PlasmaComponents; PlasmaComponents.MenuItem {}", contextMenu);
		248		}
		249
		250		function loadDynamicActions() {
		251		contextMenu.clearMenuItems();
		252
		253		// Mute
		254		var menuItem = newMenuItem();
		255		menuItem.text = i18nc("Checkable switch for (un-)muting sound output.", "Mute");
		256		menuItem.checkable = true;
		257		menuItem.checked = Muted;
		258		menuItem.clicked.connect(function() {
		259		Muted = !Muted
		260		});
		261		contextMenu.addMenuItem(menuItem);
		262
		263		// Default
		264		if (typeof PulseObject.default === "boolean") {
			droscaUnsubmitted Done I know it was like that in the original code, but please rewrite it to use directly the properties (`Muted` in this case) instead of accessing it through `PulseObject`. drosca: I know it was like that in the original code, but please rewrite it to use directly the…
		265		var menuItem = newMenuItem();
		266		menuItem.text = i18nc("Checkable switch to change the current default output.", "Default");
		267		menuItem.checkable = true;
		268		menuItem.checked = PulseObject.default
		269		menuItem.clicked.connect(function() {
		270		PulseObject.default = true
		271		});
		272		contextMenu.addMenuItem(menuItem);
		273		}
			broulikUnsubmitted Done Please add some context for translators broulik: Please add some context for translators
		274
		275		// Ports
		276		if (PulseObject.ports && PulseObject.ports.length > 0) {
		277		contextMenu.addMenuItem(newSeperator());
		278
		279		var isMultiplePorts = (1 < PulseObject.ports.length);
		280		var menuItem = newMenuItem();
		281		menuItem.text = i18nc("Heading for a list of ports of a device (for example built-in laptop speakers or a plug for headphones)", "Ports");
		282		menuItem.enabled = false;
		283		contextMenu.addMenuItem(menuItem);
			droscaUnsubmitted Done Please move literal to right side of comparison: `PulseObject.ports.length > 0` or even in this case you can just write `PulseObject.ports.length`. But actually make it `Ports.length` as said in previous comment. drosca: Please move literal to right side of comparison: `PulseObject.ports.length > 0` or even in this…
			ZrenUnsubmitted Done Definitely go for the explicit `var > 0` as the latter could be read as "if this variable exists". That's usually used as a JS minimisation trick. Zren: Definitely go for the explicit `var > 0` as the latter could be read as "if this variable…
			romanggAuthorUnsubmitted Done Please move literal to right side I like to read my inequalities from lower value to higher one. Easier to read the code this way. Besides style is there something else speaking against that? What @Zren said? ...as the latter could be read as "if this variable exists". Please explain what you mean by this. romangg: > Please move literal to right side I like to read my inequalities from lower value to higher…
			ZrenUnsubmitted Done I prefer `if (Ports.length > 0)` since that's how you'd read it in English. You wouldn't ask "is 0 less than the number of ports". Zren: I prefer `if (Ports.length > 0)` since that's how you'd read it in English. You wouldn't ask…
			droscaUnsubmitted Done Also, at least in plasma code, `(var > 2)` is used everywhere, so please change it. drosca: Also, at least in plasma code, `(var > 2)` is used everywhere, so please change it.
		284
		285		for (var i = 0; i < PulseObject.ports.length; i++) {
		286		var port = PulseObject.ports[i];
		287		var menuItem = newMenuItem();
		288		menuItem.text = port.description;
		289		menuItem.enabled = isMultiplePorts;
		290		menuItem.checkable = true;
		291		menuItem.checked = i === PulseObject.activePortIndex;
		292		var setActivePort = function(portIndex) {
		293		return function() {
		294		PulseObject.activePortIndex = portIndex;
		295		};
		296		};
		297		menuItem.clicked.connect(setActivePort(i));
		298		contextMenu.addMenuItem(menuItem);
		299		}
		300		}
		301		}
		302
		303		function show(x, y) {
		304		loadDynamicActions();
		305		open(x, y);
		306		}
		307		}
221	}	308		}
			droscaUnsubmitted Done If we now have a button for showing this menu, I think we can completely remove the right click context menu. drosca: If we now have a button for showing this menu, I think we can completely remove the right click…