Merge pull request #12189 from nextcloud/fix/12185/devices-saving

fix(MediaSettings): sync preferences with devices direct selection

6 files changed, 136 insertions, 96 deletions

diff --git a/src/components/MediaSettings/MediaSettings.vue b/src/components/MediaSettings/MediaSettings.vue
index 6ee20a477..ea8bb05bb 100644
--- a/src/components/MediaSettings/MediaSettings.vue
+++ b/src/components/MediaSettings/MediaSettings.vue
@@ -92,12 +92,12 @@
 					:devices="devices"
 					:device-id="audioInputId"
 					@refresh="updateDevices"
-					@update:deviceId="audioInputId = $event" />
+					@update:deviceId="handleAudioInputIdChange" />
 				<MediaDevicesSelector kind="videoinput"
 					:devices="devices"
 					:device-id="videoInputId"
 					@refresh="updateDevices"
-					@update:deviceId="videoInputId = $event" />
+					@update:deviceId="handleVideoInputIdChange" />
 				<MediaDevicesSpeakerTest />
 			</div>
 
@@ -312,7 +312,6 @@ export default {
 			videoOn: undefined,
 			silentCall: false,
 			updatedBackground: undefined,
-			deviceIdChanged: false,
 			audioDeviceStateChanged: false,
 			videoDeviceStateChanged: false,
 			isRecordingFromStart: false,
@@ -422,7 +421,7 @@ export default {
 		},
 
 		showUpdateChangesButton() {
-			return this.updatedBackground || this.deviceIdChanged || this.audioDeviceStateChanged
+			return this.updatedBackground || this.audioDeviceStateChanged
 				|| this.videoDeviceStateChanged
 		},
 	},
@@ -452,14 +451,12 @@ export default {
 		},
 
 		audioInputId(audioInputId) {
-			this.deviceIdChanged = true
 			if (this.showDeviceSelection && audioInputId && !this.audioOn) {
 				this.toggleAudio()
 			}
 		},
 
 		videoInputId(videoInputId) {
-			this.deviceIdChanged = true
 			if (this.showDeviceSelection && videoInputId && !this.videoOn) {
 				this.toggleVideo()
 			}
@@ -474,9 +471,10 @@ export default {
 		subscribe('talk:media-settings:show', this.showModal)
 		subscribe('talk:media-settings:hide', this.closeModalAndApplySettings)
 
-		const devicesPreferred = BrowserStorage.getItem('devicesPreferred')
-		if (!devicesPreferred) {
-			this.tabContent = 'devices'
+		// FIXME: this is a workaround to remove the old key from the browser storage
+		// To be removed in the future
+		if (BrowserStorage.getItem('devicesPreferred')) {
+			BrowserStorage.removeItem('devicesPreferred')
 		}
 	},
 
@@ -491,16 +489,22 @@ export default {
 			if (page === 'video-verification') {
 				this.isPublicShareAuthSidebar = true
 			}
+
+			if (!BrowserStorage.getItem('audioInputDevicePreferred') || !BrowserStorage.getItem('videoInputDevicePreferred')) {
+				this.tabContent = 'devices'
+			}
 		},
 
 		closeModal() {
 			this.modal = false
 			this.updatedBackground = undefined
-			this.deviceIdChanged = false
 			this.audioDeviceStateChanged = false
 			this.videoDeviceStateChanged = false
 			this.isPublicShareAuthSidebar = false
 			this.isRecordingFromStart = false
+			// Update devices preferences
+			this.updatePreferences('audioinput')
+			this.updatePreferences('videoinput')
 		},
 
 		toggleAudio() {
@@ -545,7 +549,6 @@ export default {
 				emit('local-video-control-button:toggle-video')
 			}
 
-			this.updatePreferences()
 			this.closeModal()
 		},
 
@@ -656,7 +659,17 @@ export default {
 
 		setRecordingConsentGiven(value) {
 			this.$emit('update:recording-consent-given', value)
-		}
+		},
+
+		handleAudioInputIdChange(audioInputId) {
+			this.audioInputId = audioInputId
+			this.updatePreferences('audioinput')
+		},
+
+		handleVideoInputIdChange(videoInputId) {
+			this.videoInputId = videoInputId
+			this.updatePreferences('videoinput')
+		},
 	},
 }
 </script>
@@ -664,6 +677,7 @@ export default {
 <style lang="scss" scoped>
 .media-settings {
 	padding: calc(var(--default-grid-baseline) * 5);
+	padding-bottom: 0;
 
 	&__title {
 		text-align: center;
@@ -713,9 +727,14 @@ export default {
 
 	&__call-buttons {
 		display: flex;
+		z-index: 1;
 		align-items: center;
 		justify-content: center;
 		gap: var(--default-grid-baseline);
+		position: sticky;
+		bottom: 0;
+		background-color: var(--color-main-background);
+		padding: 10px 0 20px;
 	}
 }
 
diff --git a/src/components/SettingsDialog/MediaDevicesPreview.vue b/src/components/SettingsDialog/MediaDevicesPreview.vue
index 1ca524ab9..1d6433adc 100644
--- a/src/components/SettingsDialog/MediaDevicesPreview.vue
+++ b/src/components/SettingsDialog/MediaDevicesPreview.vue
@@ -9,7 +9,7 @@
 			:devices="devices"
 			:device-id="audioInputId"
 			@refresh="updateDevices"
-			@update:deviceId="audioInputId = $event" />
+			@update:deviceId="handleAudioInputIdChange" />
 		<div class="preview preview-audio">
 			<div v-if="!audioPreviewAvailable"
 				class="preview-not-available">
@@ -38,7 +38,7 @@
 			:devices="devices"
 			:device-id="videoInputId"
 			@refresh="updateDevices"
-			@update:deviceId="videoInputId = $event" />
+			@update:deviceId="handleVideoInputIdChange" />
 		<div class="preview preview-video">
 			<div v-if="!videoPreviewAvailable"
 				class="preview-not-available">
@@ -95,6 +95,7 @@ export default {
 		const {
 			devices,
 			updateDevices,
+			updatePreferences,
 			currentVolume,
 			currentThreshold,
 			audioPreviewAvailable,
@@ -111,6 +112,7 @@ export default {
 			video,
 			devices,
 			updateDevices,
+			updatePreferences,
 			currentVolume,
 			currentThreshold,
 			audioPreviewAvailable,
@@ -176,7 +178,20 @@ export default {
 
 			return t('spreed', 'Error while accessing camera')
 		},
-	}
+	},
+
+	methods: {
+		handleAudioInputIdChange(audioInputId) {
+			this.audioInputId = audioInputId
+			this.updatePreferences('audioinput')
+		},
+
+		handleVideoInputIdChange(videoInputId) {
+			this.videoInputId = videoInputId
+			this.updatePreferences('videoinput')
+		},
+
+	},
 }
 </script>
 
diff --git a/src/composables/useDevices.js b/src/composables/useDevices.js
index 4ad5a90ed..aa0b2714a 100644
--- a/src/composables/useDevices.js
+++ b/src/composables/useDevices.js
@@ -155,10 +155,11 @@ export function useDevices(video, initializeOnMounted) {
 
 	/**
 	 * Update preference counters for devices (audio and video)
+	 * @param {string} kind the kind of the input stream to update ('audioinput' or 'videoinput')
 	 * @public
 	 */
-	function updatePreferences() {
-		mediaDevicesManager.updatePreferences()
+	function updatePreferences(kind) {
+		mediaDevicesManager.updatePreferences(kind)
 	}
 
 	/**
diff --git a/src/services/__tests__/mediaDevicePreferences.spec.js b/src/services/__tests__/mediaDevicePreferences.spec.js
index deaf296b6..8311420d0 100644
--- a/src/services/__tests__/mediaDevicePreferences.spec.js
+++ b/src/services/__tests__/mediaDevicePreferences.spec.js
@@ -6,7 +6,7 @@ import {
 	getFirstAvailableMediaDevice,
 	listMediaDevices,
 	populateMediaDevicesPreferences,
-	updateMediaDevicesPreferences,
+	promoteMediaDevice,
 } from '../mediaDevicePreferences.ts'
 
 describe('mediaDevicePreferences', () => {
@@ -122,73 +122,71 @@ describe('mediaDevicePreferences', () => {
 		})
 	})
 
-	describe('updateMediaDevicesPreferences', () => {
+	describe('promoteMediaDevice', () => {
 		it('returns null if preference lists were not updated (no id or default id provided)', () => {
 			const ids = [null, undefined, 'default']
 
 			const getOutput = (id) => {
-				const attributes = { devices: allDevices, audioInputId: id, videoInputId: id }
-				return updateMediaDevicesPreferences(attributes, audioInputPreferenceList, videoInputPreferenceList)
+				return promoteMediaDevice({
+					kind: 'audioinput',
+					devices: allDevices,
+					inputList: audioInputPreferenceList,
+					inputId: id
+				})
 			}
 
 			// Assert
 			ids.forEach(id => {
-				expect(getOutput(id)).toMatchObject({ newAudioInputList: null, newVideoInputList: null })
+				expect(getOutput(id)).toEqual(null)
 			})
 		})
 
 		it('returns updated preference lists (device A id provided)', () => {
-			const attributes = { devices: allDevices, audioInputId: audioInputDeviceA.deviceId, videoInputId: videoInputDeviceA.deviceId }
-			const output = updateMediaDevicesPreferences(attributes, audioInputPreferenceList, videoInputPreferenceList)
+			const output = promoteMediaDevice({
+				kind: 'audioinput',
+				devices: allDevices,
+				inputList: audioInputPreferenceList,
+				inputId: audioInputDeviceA.deviceId
+			})
 
 			// Assert: should put device A on top of default device
-			expect(output).toMatchObject({
-				newAudioInputList: [audioInputDeviceA, audioInputDeviceDefault, audioInputDeviceB],
-				newVideoInputList: [videoInputDeviceA, videoInputDeviceDefault, videoInputDeviceB],
-			})
+			expect(output).toEqual([audioInputDeviceA, audioInputDeviceDefault, audioInputDeviceB])
 		})
 
 		it('returns null if preference lists were not updated (device A id provided but not available)', () => {
-			const attributes = {
+			const output = promoteMediaDevice({
+				kind: 'audioinput',
 				devices: allDevices.filter(device => !['da1234567890', 'da4567890123'].includes(device.deviceId)),
-				audioInputId: audioInputDeviceA.deviceId,
-				videoInputId: videoInputDeviceA.deviceId,
-			}
-			const output = updateMediaDevicesPreferences(attributes, audioInputPreferenceList, videoInputPreferenceList)
+				inputList: audioInputPreferenceList,
+				inputId: audioInputDeviceA.deviceId
+			})
 
 			// Assert
-			expect(output).toMatchObject({ newAudioInputList: null, newVideoInputList: null })
+			expect(output).toEqual(null)
 		})
 
 		it('returns null if preference lists were not updated (all devices are not available)', () => {
-			const attributes = {
+			const output = promoteMediaDevice({
+				kind: 'audioinput',
 				devices: allDevices.filter(device => !['audioinput', 'videoinput'].includes(device.kind)),
-				audioInputId: audioInputDeviceA.deviceId,
-				videoInputId: videoInputDeviceA.deviceId,
-			}
-			const output = updateMediaDevicesPreferences(attributes, audioInputPreferenceList, videoInputPreferenceList)
+				inputList: audioInputPreferenceList,
+				inputId: audioInputDeviceA.deviceId
+			})
 
 			// Assert
-			expect(output).toMatchObject({ newAudioInputList: null, newVideoInputList: null })
+			expect(output).toEqual(null)
 		})
 
 		it('returns updated preference lists (device B id provided, but not registered, default device and device A not available)', () => {
-			const attributes = {
+			const output = promoteMediaDevice({
+				kind: 'audioinput',
 				devices: allDevices.filter(device => !['default', 'da1234567890', 'da4567890123'].includes(device.deviceId)),
-				audioInputId: audioInputDeviceB.deviceId,
-				videoInputId: videoInputDeviceB.deviceId,
-			}
-			const output = updateMediaDevicesPreferences(
-				attributes,
-				[audioInputDeviceDefault, audioInputDeviceA],
-				[videoInputDeviceDefault, videoInputDeviceA],
-			)
+				inputList: [audioInputDeviceDefault, audioInputDeviceA],
+				inputId: audioInputDeviceB.deviceId
+			})
 
 			// Assert: should put device C on top of device B, but not the device A
-			expect(output).toMatchObject({
-				newAudioInputList: [audioInputDeviceDefault, audioInputDeviceA, audioInputDeviceB],
-				newVideoInputList: [videoInputDeviceDefault, videoInputDeviceA, videoInputDeviceB],
-			})
+			expect(output).toEqual([audioInputDeviceDefault, audioInputDeviceA, audioInputDeviceB])
 		})
 	})
 })
diff --git a/src/services/mediaDevicePreferences.ts b/src/services/mediaDevicePreferences.ts
index 56ef57813..a1d8eccde 100644
--- a/src/services/mediaDevicePreferences.ts
+++ b/src/services/mediaDevicePreferences.ts
@@ -21,6 +21,13 @@ enum DeviceKind {
 	AudioOutput = 'audiooutput',
 }
 
+type PromotePayload = {
+	kind: DeviceKind,
+	devices: MediaDeviceInfo[],
+	inputList: MediaDeviceInfo[],
+	inputId: InputId
+}
+
 /**
  * List all registered devices in order of their preferences
  * Show whether device is currently unplugged or selected, if information is available
@@ -90,13 +97,14 @@ function registerNewMediaDevice(device: MediaDeviceInfo, devicesList: MediaDevic
  *
  * Returns changed preference lists for audio / video devices (null, if it hasn't been changed)
  *
- * @param kind kind of device
- * @param devices list of available devices
- * @param inputList list of registered audio/video devices in order of preference
- * @param inputId id of currently selected input
+ * @param data the wrapping object
+ * @param data.kind kind of device
+ * @param data.devices list of available devices
+ * @param data.inputList list of registered audio/video devices in order of preference
+ * @param data.inputId id of currently selected input
  * @return {InputListUpdated} updated devices list (null, if it has not been changed)
  */
-function promoteMediaDevice(kind: DeviceKind, devices: MediaDeviceInfo[], inputList: MediaDeviceInfo[], inputId: InputId): InputListUpdated {
+function promoteMediaDevice({ kind, devices, inputList, inputId } : PromotePayload) : InputListUpdated {
 	if (!inputId) {
 		return null
 	}
@@ -164,27 +172,9 @@ function populateMediaDevicesPreferences(devices: MediaDeviceInfo[], audioInputL
 	}
 }
 
-/**
- * Update devices preferences. Assuming that preferred devices were selected, should be called after applying the selection:
- * so either with joining the call or changing device during the call
- *
- * Returns changed preference lists for audio / video devices (null, if it hasn't been changed)
- *
- * @param attributes MediaDeviceManager attributes
- * @param audioInputList list of registered audio devices in order of preference
- * @param videoInputList list of registered video devices in order of preference
- * @return {InputLists} object with updated devices lists (null, if they have not been changed)
- */
-function updateMediaDevicesPreferences(attributes: Attributes, audioInputList: MediaDeviceInfo[], videoInputList: MediaDeviceInfo[]): InputLists {
-	return {
-		newAudioInputList: promoteMediaDevice(DeviceKind.AudioInput, attributes.devices, audioInputList, attributes.audioInputId),
-		newVideoInputList: promoteMediaDevice(DeviceKind.VideoInput, attributes.devices, videoInputList, attributes.videoInputId),
-	}
-}
-
 export {
 	getFirstAvailableMediaDevice,
 	listMediaDevices,
 	populateMediaDevicesPreferences,
-	updateMediaDevicesPreferences,
+	promoteMediaDevice,
 }
diff --git a/src/utils/webrtc/MediaDevicesManager.js b/src/utils/webrtc/MediaDevicesManager.js
index b1819952e..83bbd36dc 100644
--- a/src/utils/webrtc/MediaDevicesManager.js
+++ b/src/utils/webrtc/MediaDevicesManager.js
@@ -8,7 +8,7 @@ import {
 	getFirstAvailableMediaDevice,
 	listMediaDevices,
 	populateMediaDevicesPreferences,
-	updateMediaDevicesPreferences,
+	promoteMediaDevice,
 } from '../../services/mediaDevicePreferences.ts'
 import EmitterMixin from '../EmitterMixin.js'
 
@@ -199,12 +199,17 @@ MediaDevicesManager.prototype = {
 			// according to the spec, a single device of each kind (if at least
 			// one device of that kind is available) with an empty deviceId is
 			// returned, which will not be registered in the preference list.
+			let deviceIdChanged = false
 			if (this.attributes.audioInputId === undefined || this.attributes.audioInputId === previousFirstAvailableAudioInputId) {
 				this.attributes.audioInputId = getFirstAvailableMediaDevice(devices, this._preferenceAudioInputList) || devices.find(device => device.kind === 'audioinput')?.deviceId
-				console.debug(listMediaDevices(this.attributes, this._preferenceAudioInputList, this._preferenceVideoInputList))
+				deviceIdChanged = true
 			}
 			if (this.attributes.videoInputId === undefined || this.attributes.videoInputId === previousFirstAvailableVideoInputId) {
 				this.attributes.videoInputId = getFirstAvailableMediaDevice(devices, this._preferenceVideoInputList) || devices.find(device => device.kind === 'videoinput')?.deviceId
+				deviceIdChanged = true
+			}
+
+			if (deviceIdChanged) {
 				console.debug(listMediaDevices(this.attributes, this._preferenceAudioInputList, this._preferenceVideoInputList))
 			}
 
@@ -242,25 +247,37 @@ MediaDevicesManager.prototype = {
 		}
 	},
 
-	updatePreferences() {
-		const { newAudioInputList, newVideoInputList } = updateMediaDevicesPreferences(
-			this.attributes,
-			this._preferenceAudioInputList,
-			this._preferenceVideoInputList,
-		)
+	updatePreferences(kind) {
+		if (kind === 'audioinput') {
+			const newAudioInputList = promoteMediaDevice({
+				kind,
+				devices: this.attributes.devices,
+				inputList: this._preferenceAudioInputList,
+				inputId: this.attributes.audioInputId
+			})
 
-		if (newAudioInputList) {
-			this._preferenceAudioInputList = newAudioInputList
-			BrowserStorage.setItem('audioInputPreferences', JSON.stringify(newAudioInputList))
-		}
-		if (newVideoInputList) {
-			this._preferenceVideoInputList = newVideoInputList
-			BrowserStorage.setItem('videoInputPreferences', JSON.stringify(newVideoInputList))
-		}
+			if (newAudioInputList) {
+				this._preferenceAudioInputList = newAudioInputList
+				BrowserStorage.setItem('audioInputPreferences', JSON.stringify(newAudioInputList))
+			}
+			if (!BrowserStorage.getItem('audioInputDevicePreferred')) {
+				BrowserStorage.setItem('audioInputDevicePreferred', true)
+			}
+		} else if (kind === 'videoinput') {
+			const newVideoInputList = promoteMediaDevice({
+				kind,
+				devices: this.attributes.devices,
+				inputList: this._preferenceVideoInputList,
+				inputId: this.attributes.videoInputId
+			})
 
-		const devicesPreferred = BrowserStorage.getItem('devicesPreferred')
-		if (!devicesPreferred) {
-			BrowserStorage.setItem('devicesPreferred', true)
+			if (newVideoInputList) {
+				this._preferenceVideoInputList = newVideoInputList
+				BrowserStorage.setItem('videoInputPreferences', JSON.stringify(newVideoInputList))
+			}
+			if (!BrowserStorage.getItem('videoInputDevicePreferred')) {
+				BrowserStorage.setItem('videoInputDevicePreferred', true)
+			}
 		}
 
 	},