Audio playback window #465
Conversation
- Add dedicated audio playback window with close button - Prevent buffer overflow by checking audio data length during playback - Update playback controls to use proper component names - Stop audio playback gracefully when track ends - Refactor UI elements to use shared close button component This commit introduces a floating audio playback window to display ongoing audio streams. It fixes potential buffer overflow issues by adding bounds checks during audio data processing. The audio controller now correctly stops playback and updates UI states when tracks end. Component names were updated for clarity, and the new window leverages the shared RobrixCloseButton component for consistency.
There was a problem hiding this comment.
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/audio/audio_controller.rs:252
- The magic number '44' is used as an offset for audio data; consider defining a named constant (e.g., WAV_HEADER_SIZE) for better clarity and maintainability.
let pos = Arc::new(Mutex::new(44));
Remove arrogant 'log!' Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new audio playback functionality by adding dedicated components for audio messages and playback, and updates existing UI templates to support audio messages.
- Added AudioPlaybackWindow, AudioMessageUI, and AudioController components.
- Updated room_screen to use new audio message templates in both regular and condensed views.
- Integrated audio modules into the app's main layout.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/sliding_sync.rs | Removed an extraneous blank line. |
| src/shared/styles.rs | Added a new RobrixCloseButton component with custom styling. |
| src/shared/icon_button.rs | Added a new RobrixButton component with updated layout definitions. |
| src/lib.rs | Cleaned up module imports and removed an unused comment. |
| src/home/room_screen.rs | Updated imports and added audio message templates for both regular and condensed views. |
| src/audio/mod.rs | Introduced the new audio module with its subcomponents. |
| src/audio/audio_playback_window.rs | Added the AudioPlaybackWindow component for managing audio playback UI. |
| src/audio/audio_message_ui.rs | Added the AudioMessageUI component to handle audio message-specific UI events. |
| src/audio/audio_controller.rs | Added the AudioController for managing audio playback and processing. |
| src/app.rs | Integrated audio playback components into the app layout. |
Fix typo Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/home/room_screen.rs:3704
- [nitpick] Consider renaming 'is_playing_mg' to a more descriptive name such as 'playing_status_guard' for clarity.
let is_playing_mg = is_playing.lock().unwrap();
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR integrates new audio playback functionalities into the application by adding an audio playback window, refining the audio message UI, and updating the audio controller logic. Key changes include:
- Introducing new UI components (RobrixCloseButton, RobrixButton) and integrating the audio playback window.
- Modifying the timeline view to support new audio message templates.
- Adding new modules for handling audio (audio_message_ui, audio_controller, audio_playback_window) and updating app initialization to include these.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sliding_sync.rs | Removed an extra blank line to improve code formatting. |
| src/shared/styles.rs | Added a new close button component for Robrix with clear styling. |
| src/shared/icon_button.rs | Introduced a new RobrixButton component with updated properties. |
| src/lib.rs | Integrated the new audio module and cleaned up an obsolete comment. |
| src/home/room_screen.rs | Updated message templates to use the new audio message UI components. |
| src/audio/*.rs | Added and integrated modules for audio playback, controller, and UI. |
| src/app.rs | Updated app layout to include the new audio controller and playback window. |
| // Use `pos + audio.bit_depth.ilog2()` rather than `pos` to ensure no panic when computing isize mentioned above. | ||
| if pos + audio.bit_depth.ilog2() as usize > audio_data_len - 1 { |
There was a problem hiding this comment.
[nitpick] Consider extracting the calculation 'pos + audio.bit_depth.ilog2()' into a well-named variable and add a clarifying comment to explain its relation to the sample boundary check.
| // Use `pos + audio.bit_depth.ilog2()` rather than `pos` to ensure no panic when computing isize mentioned above. | |
| if pos + audio.bit_depth.ilog2() as usize > audio_data_len - 1 { | |
| // Calculate the position of the next sample boundary to ensure no out-of-bounds access. | |
| let sample_boundary_pos = pos + audio.bit_depth.ilog2() as usize; | |
| // Check if the sample boundary position exceeds the audio data length. | |
| if sample_boundary_pos > audio_data_len - 1 { |
Copilot uses AI. Check for mistakes.
| impl Drop for AudioMessageUI { | ||
| fn drop(&mut self) { | ||
| if self.timeline_audio_event_item_id.as_ref().is_some() && self.is_playing { | ||
| let info = self.view.label(id!(fetching_info)).text(); | ||
| Cx::post_action(AudioMessageUIAction::ToPlaybackWindowAction(info)); |
There was a problem hiding this comment.
[nitpick] Avoid posting UI actions from within the Drop implementation as it might trigger unexpected behavior after the widget is disposed; consider handling this cleanup outside of Drop.
| impl Drop for AudioMessageUI { | |
| fn drop(&mut self) { | |
| if self.timeline_audio_event_item_id.as_ref().is_some() && self.is_playing { | |
| let info = self.view.label(id!(fetching_info)).text(); | |
| Cx::post_action(AudioMessageUIAction::ToPlaybackWindowAction(info)); | |
| impl AudioMessageUI { | |
| pub fn cleanup(&mut self, cx: &mut Cx) { | |
| if self.timeline_audio_event_item_id.as_ref().is_some() && self.is_playing { | |
| let info = self.view.label(id!(fetching_info)).text(); | |
| cx.action(AudioMessageUIAction::ToPlaybackWindowAction(info)); |
Copilot uses AI. Check for mistakes.
| self.visible = false; | ||
| self.redraw(cx); | ||
| } | ||
| for action in actions { | ||
| if let Some(AudioMessageUIAction::ToPlaybackWindowAction(info)) = action.downcast_ref() { | ||
| self.view.label(id!(info)).set_text(cx, info); | ||
| self.view.visible = true; | ||
| self.visible = true; |
There was a problem hiding this comment.
[nitpick] The visibility is being toggled on both 'self.visible' and 'self.view.visible'. Consider consolidating these visibility controls into a single property or clearly documenting the reason for toggling both.
| self.visible = false; | |
| self.redraw(cx); | |
| } | |
| for action in actions { | |
| if let Some(AudioMessageUIAction::ToPlaybackWindowAction(info)) = action.downcast_ref() { | |
| self.view.label(id!(info)).set_text(cx, info); | |
| self.view.visible = true; | |
| self.visible = true; | |
| self.view.visible = false; | |
| self.redraw(cx); | |
| } | |
| for action in actions { | |
| if let Some(AudioMessageUIAction::ToPlaybackWindowAction(info)) = action.downcast_ref() { | |
| self.view.label(id!(info)).set_text(cx, info); | |
| self.view.visible = true; |
Copilot uses AI. Check for mistakes.
No description provided.