Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device preferences are not saved when set from Talk settings #12185

Closed
danxuliu opened this issue Apr 22, 2024 · 3 comments · Fixed by #12189
Closed

Device preferences are not saved when set from Talk settings #12185

danxuliu opened this issue Apr 22, 2024 · 3 comments · Fixed by #12189
Assignees

Comments

@danxuliu
Copy link
Member

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

This is a regression introduced in #11936 and #12108

Before the introduction of the preferred media devices list, when a device was selected its id was stored in the BrowserStorage. Then, whenever that device was found again (for example, when the list of available devices was loaded for the first time after a reload) that device was set as the selected device.

In #11936 the added OR conditions seem to override the id from the BrowserStorage and cause the last device in the list to become the selected one. In #12108 the conditions were removed altogether, so the ids saved to the BrowserStorage are no longer used when loading the available devices, and the first available device ends being the selected one (unless the preferred device list was also saved).

The MediaSettings component saves the preferred device list when applying changes, so after joining a call the selected devices are persisted. Thus I guess it would be enough to call mediaDevicesManager.updatePreferences() when closing the Talk settings (but I have not tested it).

However, note that just closing the MediaSettings does not persist the selected device; this is done only when joining the call. Maybe for consistency the MediaSettings should also persist the selected device when simply closed, even if the call is not joined? Specially given that, although the selected device is not persisted between reloads, it is "persisted" between different openings of the media settings within the same session (that is, open the media settings, select another device, close the media settings without joining the call, open the media settings again -> Same selected device; open the media settings, select another device, close the media settings without joining the call, reload, open the media settings again -> Previously selected device)

How to test

  • Connect three or more microphones/audio input devices
    • If testing with Firefox and PulseAudio you will need just an extra microphone/audio input device due to the Monitor source loaded for the actual microphone
    • If testing with Chromium the Monitor sources are not listed, but you will need just an extra microphone/audio input device anyway due to the Default source being provided in addition to the actual audio input devices
    • If using PulseAudio the additional microphones/audio input devices can be added by adding a null source with pactl load-module module-null-source or the echo cancelling module with pactl load-module module-echo-cancel
  • Create a public conversation
  • In a private window, open the conversation
  • Open Talk settings
  • Select the second microphone/audio input device in the list
  • Close Talk settings
  • Reload
  • Open Talk settings

Expected result

The second microphone/audio input device is selected

Actual result

The first microphone/audio input device is selected (or the last one if checking 2d20812^, which in practice is #11936)

@Antreesy
Copy link
Contributor

Antreesy commented Apr 22, 2024

Thus I guess it would be enough to call mediaDevicesManager.updatePreferences() when closing the Talk settings (but I have not tested it).

Yes, we could move it to:

  • MediaDevicesPreview @update:deviceId event (or on dialog close, to make it once);
  • MediaSettings closeModal() method:
    • before the joining, that should act the same as in Talk settings;
    • but if I'm in the call, and close modal without applying, that should act as dismissal, without saving preferences (because I don't proceed with new devices in the call either);

@danxuliu
Copy link
Member Author

danxuliu commented Apr 22, 2024

  • MediaSettings closeModal() method:
    • but if I'm in the call, and close modal without applying, that should act as dismissal, without saving preferences (because I don't proceed with new devices in the call either);

Actually... you do :-( In fact I was going to open an issue about that also. But it is unrelated to the latest changes, though.

If you open the media settings during a call and select a new device that device will immediately become the selected/active one. It does not matter if you then close the media settings without clicking on Apply settings, the device is already selected. Note that it is different in the case of the virtual background; in that case the changes are in fact not applied in the call until Apply settings is clicked.

This is a legacy behaviour from a limitation in the browsers. Back in the day (I am not sure if that still applies, though) it was not possible to have more than one active device of the same kind at the same time. To overcome that and taking advantage of the fact that, in most cases, settings in Nextcloud are immediately applied without having to explicitly use an Accept or Apply button, as soon as a device was selected it became the active device application wide, stopping any track from any previously selected device of that kind.

I think there are two options, one safer and one riskier. The safer one would be not to show Apply settings when the device is changed, but keep showing it for the virtual background. It might be a bit inconsistent, but it should work.

The riskier one would be to first verify if it is possible to have more than one device of the same kind active at the same time* in all the currently supported browsers and, if it is, adjust the code of MediaDevicesManager and all related classes to reflect that. Besides being more consistent with the virtual background behaviour this would make possible to switch to another device in the media settings while still have the previous one active and only switch to the new one in the call after applying the settings. Yet I am not sure if, at least at this point, it is worth the trouble...

*It would be good to dig the old pull requests too just in case there are more problematic browser behaviours that I am not recalling right now.

@DorraJaouad
Copy link
Contributor

At first, persisting devices (update preferences) was intended to be only after joining call because user can be experimenting devices and can close media settings after that, BUT the info that I totally forgot is that devices are actually applied on each change which now makes the first assumption contradicting devices selection 🙈.

@Antreesy , first option is good. Update preferences needs be called @update:deviceId event because user may refresh the window without closing modal. This will cover both media settings and Talk settings devices changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants