Output change #597
Output change #597
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🎉 Welcome @Mayank4352!
We appreciate your contribution! 🚀 |
Mayank4352
force-pushed
the
output_change
branch
from
5c11f31 to
e5ff288
Compare
|
Hey @M4dhav, Kindly review this PR |
M4dhav
left a comment
M4dhav
left a comment
There was a problem hiding this comment.
Please fix merge conflicts
|
Can you please explain the implementation a bit? What enumeration does, how audio devices are discovered, etc |
|
->The enumerateDevices() uses webrtc.navigator.mediaDevices.enumerateDevices() which queries for all available media devices (microphones, cameras, speakers). This is the main crux of the controller file, rest are the UI changes. I've made a simple data model that wraps WebRTC's MediaDeviceInfo which will Decouple the app from WebRTC's API, handles nullable fields (kind, groupId) with defaults and makes testing easier (we were able to create AudioDevice directly without WebRTC) |
| return ElevatedButton.icon( | ||
| return ElevatedButton( | ||
| onPressed: () async { | ||
| await _deleteRoomDialog( | ||
| controller.isAdmin | ||
| ? AppLocalizations.of(context)!.delete | ||
| : AppLocalizations.of(context)!.leave, | ||
| () async { | ||
| if (controller.isAdmin) { | ||
| if (liveKitController.isRecording.value == true) { | ||
| await controller.endLiveChapter(); | ||
| } else { | ||
| customSnackbar( | ||
| AppLocalizations.of(context)!.error, | ||
| AppLocalizations.of(context)!.noRecordingError, | ||
| LogType.error, | ||
| ); | ||
| } | ||
| } else { | ||
| await controller.leaveRoom(); | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| style: ElevatedButton.styleFrom( | ||
| backgroundColor: const Color.fromARGB(255, 241, 108, 98), | ||
| backgroundColor: Colors.redAccent, | ||
| foregroundColor: Colors.white, | ||
| shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(20)), | ||
| ), | ||
| icon: const Icon(Icons.exit_to_app), | ||
| label: Text(AppLocalizations.of(context)!.leaveButton), | ||
| child: const Icon(Icons.call_end, size: 24), |
There was a problem hiding this comment.
Why are there changes to the end call button?
There was a problem hiding this comment.
To make space the output change button as with the current size of end call button the ribbon was looking to cramped so i adjust it's size bit, also i saw that the color for the buttons was just a static color so i changed that to use the color from the theme
| Widget _buildAudioSettingsButton() { | ||
| return FloatingActionButton( | ||
| onPressed: () => showAudioDeviceSelector(context), | ||
| backgroundColor: Theme.of(context).colorScheme.secondary, | ||
| child: const Icon(Icons.settings_voice), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why FloatingActionButton and not ElevatedButton.icon like the others?
There was a problem hiding this comment.
I used a FloatingActionButton initially while iterating on the layout, I'll switch it to an ElevatedButton.icon
There was a problem hiding this comment.
Hey, @M4dhav after reviewing it, i believe the other widgets also use FAB, do you still want me to make it ElevatedButton.icon or should i keep it the way it is currently???
| return ElevatedButton.icon( | ||
| return ElevatedButton( | ||
| onPressed: () async { | ||
| await _deleteRoomDialog( | ||
| controller.appwriteRoom.isUserAdmin | ||
| ? AppLocalizations.of(context)!.delete | ||
| : AppLocalizations.of(context)!.leave, | ||
| () async { | ||
| if (controller.appwriteRoom.isUserAdmin) { | ||
| await controller.deleteRoom(); | ||
| } else { | ||
| await controller.leaveRoom(); | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| style: ElevatedButton.styleFrom( | ||
| backgroundColor: const Color.fromARGB(255, 241, 108, 98), | ||
| backgroundColor: Colors.redAccent, | ||
| foregroundColor: Colors.white, | ||
| shape: RoundedRectangleBorder( | ||
| borderRadius: BorderRadius.circular(20), | ||
| ), | ||
| ), | ||
| icon: const Icon(Icons.exit_to_app), | ||
| label: Text(AppLocalizations.of(context)!.leaveButton), | ||
| child: const Icon(Icons.call_end, size: 24), |
There was a problem hiding this comment.
Unrelated changes to End Call button
There was a problem hiding this comment.
Those are needed for the space to not look cramped, i just made some cosmetic changes in the icon for it to look more like a end call button, initially it used static color, i replaced that with the theme one
| Widget _buildAudioSettingsButton() { | ||
| return FloatingActionButton( | ||
| onPressed: () => showAudioDeviceSelector(context), | ||
| backgroundColor: Theme.of(context).colorScheme.onSecondary, | ||
| child: const Icon(Icons.volume_up), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Will fix it, used it while playing around with UI
| if (!Get.isRegistered<AudioDeviceController>()) { | ||
| Get.put(AudioDeviceController()); | ||
| } |
There was a problem hiding this comment.
Instead of using GetX here, just instantiate an instance within the dialog, as you do not need dependency injection here
| Get.put(AudioDeviceController()); | ||
| } | ||
| final controller = AudioDeviceController(); | ||
| controller.refreshDevices(); |
There was a problem hiding this comment.
refreshDevices is async so await this call
| showModalBottomSheet( | ||
| context: context, | ||
| isScrollControlled: true, | ||
| backgroundColor: Colors.transparent, | ||
| builder: (context) => const AudioDeviceSelectorDialog(), | ||
| ); | ||
| builder: (context) => AudioDeviceSelectorDialog(controller: controller), | ||
| ).whenComplete(() => controller.dispose()); |
There was a problem hiding this comment.
use GetX APIs to show the modal
| IconButton( | ||
| icon: const Icon(Icons.close), | ||
| onPressed: () => Get.back(), | ||
| onPressed: () => Navigator.of(context).pop(), |
| SizedBox(width: UiSizes.width_8), | ||
| ElevatedButton( | ||
| onPressed: () => Get.back(), | ||
| onPressed: () => Navigator.of(context).pop(), |
2 participants
Description
Allow Users to Select Preferred Speaker
Fixes #567
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested Locally using two devices for both rooms and calls
Checklist:
Maintainer Checklist