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

Feat(MediaSettings): Add audio test #11624

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Feat(MediaSettings): Add audio test #11624

merged 2 commits into from
Mar 5, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Feb 21, 2024

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

Screencast :

audio-test.webm

🚧 Tasks

  • Design review
  • Code review

🏁 Checklist

@DorraJaouad DorraJaouad added this to the 💞 Next Major (29) milestone Feb 21, 2024
@DorraJaouad DorraJaouad requested a review from Antreesy February 21, 2024 12:41
@DorraJaouad DorraJaouad self-assigned this Feb 21, 2024
@DorraJaouad
Copy link
Contributor Author

Audio Output: Add the "voice message" option on the media settings screen to allow recording and then playing back? Or a simple "bing" sound for testing?

I used something like bing sound for audio output testing. However, I can also add voice recording as a test for the mic.
cc @nextcloud/designers @nickvergessen

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind adding a new sound, just feeling that default call waiting fits better here

src/components/MediaSettings/MediaSettings.vue Outdated Show resolved Hide resolved
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @DorraJaouad
I would left-align this new button so that the speaker icon is vertically aligned with the mic and camera icons, and change the test to "Test speakers".

Also, given that we're adding more elements, could we move the "always show dialog" setting in the 3-dot menu next to the button as an action checkbox component?

@nickvergessen
Copy link
Member

However, I can also add voice recording as a test for the mic.

Exactly that was the idea, it would basically allow you to test both, mic and speaker

img/dingSound.flac Outdated Show resolved Hide resolved
@DorraJaouad
Copy link
Contributor Author

Exactly that was the idea, it would basically allow you to test both, mic and speaker

Right, but some users want just to test speaker and they don't want to randomly say some words XD . That's why I chose the first option. So add both or only voice recorder ?

@nickvergessen
Copy link
Member

So add both or only voice recorder ?

I leave that to design cc @marcoambrosini

As for what to play, any issue with playing an already existing file (less files, more caching, less license issues, etc)

@marcoambrosini marcoambrosini self-assigned this Feb 23, 2024
@marcoambrosini
Copy link
Member

marcoambrosini commented Feb 26, 2024

@DorraJaouad @nickvergessen would it be possible to have a third entry with speaker selection similar to Jitsi? Or at least showing which is the current speaker?

@DorraJaouad
Copy link
Contributor Author

sinkId can show the audio output being used but read only and it is not supported on Safari.

Otherwise, selectAudioOutput allows user to select audio ouput device but it is only supported on Firefox.

@Antreesy
Copy link
Contributor

Antreesy commented Mar 4, 2024

Or at least showing which is the current speaker?

navigator.mediaDevices.enumerateDevices() can list available output devices. Though we couldn't identify, which is being used by the browser (at best it shows Default). https://developer.mozilla.org/en-US/docs/Web/API/AudioContext is currently not implemented widely, so there's nothing to do atm.

navigator.mediaDevices.enumerateDevices().then((devices) => {
      devices.forEach((device) => {
        console.log(`${device.kind}: ${device.label} id = ${device.deviceId}`);
      });
    })

image

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this will suffice for now. I'll open an issue for improvements once this is in

@Antreesy Antreesy force-pushed the feat/1181/add-audio-test branch from cdfc1b2 to cb89e14 Compare March 5, 2024 14:17
Signed-off-by: DorraJaouad <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the feat/1181/add-audio-test branch from cb89e14 to c699ada Compare March 5, 2024 19:32
@Antreesy Antreesy merged commit aba2df7 into main Mar 5, 2024
45 checks passed
@Antreesy Antreesy deleted the feat/1181/add-audio-test branch March 5, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

System check for call participants
4 participants