refactor:drop_audio_gui_player #132
Conversation
one more step towards deprecating this in favor of ovos-media and untangling the GUI audio will now be handled fully by audio service instead of having special treatment and checking if GUI is available first a QML player can be made as an audio plugin later to restore this functionality
one more step towards deprecating this in favor of ovos-media and untangling the GUI audio will now be handled fully by audio service instead of having special treatment and checking if GUI is available first a QML player can be made as an audio plugin later to restore this functionality
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 18 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant modifications to the audio playback handling within the OVOS project. Key changes include the removal of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
ovos_plugin_common_play/ocp/player.py (1)
Line range hint
427-465: LGTM! Consider extracting playback handling logic into separate methods.The
playfunction correctly handles playback for different backend types. The logic is sound and covers all the necessary steps such as stream validation, GUI updates, event emissions, and mpris updates.To further improve the code, consider extracting the playback handling logic for each backend type (AUDIO_SERVICE, AUDIO, SKILL, VIDEO, WEBVIEW) into separate methods. This will make the
playfunction more readable and maintainable by reducing its size and complexity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- ovos_plugin_common_play/ocp/gui.py (2 hunks)
- ovos_plugin_common_play/ocp/player.py (1 hunks)
- ovos_plugin_common_play/ocp/res/ui/+mediacenter/OVOSAudioPlayer.qml (0 hunks)
- ovos_plugin_common_play/ocp/res/ui/+mediacenter/PlayerLoader.qml (1 hunks)
- ovos_plugin_common_play/ocp/res/ui/+mediacenter/SpectrumWaveDelegate.qml (0 hunks)
- ovos_plugin_common_play/ocp/res/ui/OVOSAudioPlayer.qml (0 hunks)
- ovos_plugin_common_play/ocp/res/ui/PlayerLoader.qml (1 hunks)
- ovos_plugin_common_play/ocp/res/ui/SpectrumWaveDelegate.qml (0 hunks)
- ovos_plugin_common_play/ocp/res/ui/qmldir (0 hunks)
Files not reviewed due to no reviewable changes (5)
- ovos_plugin_common_play/ocp/res/ui/+mediacenter/OVOSAudioPlayer.qml
- ovos_plugin_common_play/ocp/res/ui/+mediacenter/SpectrumWaveDelegate.qml
- ovos_plugin_common_play/ocp/res/ui/OVOSAudioPlayer.qml
- ovos_plugin_common_play/ocp/res/ui/SpectrumWaveDelegate.qml
- ovos_plugin_common_play/ocp/res/ui/qmldir
Additional context used
Ruff
ovos_plugin_common_play/ocp/gui.py
11-11:
ovos_workshop.backwards_compat.MediaEntryimported but unusedRemove unused import:
ovos_workshop.backwards_compat.MediaEntry(F401)
Additional comments not posted (3)
ovos_plugin_common_play/ocp/res/ui/PlayerLoader.qml (1)
16-16: Approve the change as it aligns with the PR objectives.The change to the
backgroundAllowedPlayersproperty is in line with the goal of deprecating the current audio GUI player and moving towards the ovos-media framework. This change simplifies audio handling by ensuring that audio is managed entirely by the audio service.Please ensure that this change does not break existing functionality and that the codebase is updated to be compatible with the new audio handling approach. Run the following script to verify the usage of
"OVOSAudioPlayer.qml"in the codebase:Verification successful
Approve the change as it aligns with the PR objectives and does not negatively impact the codebase.
The removal of "OVOSAudioPlayer.qml" from the
backgroundAllowedPlayersproperty inPlayerLoader.qmlis consistent with the rest of the codebase. There are no other direct references to "OVOSAudioPlayer.qml" that would be affected by this change. This modification supports the goal of deprecating the current audio GUI player and moving towards the ovos-media framework for simplified audio handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of "OVOSAudioPlayer.qml" in the codebase. # Test: Search for the usage of "OVOSAudioPlayer.qml". Expect: No occurrences. rg --type qml -A 5 $'"OVOSAudioPlayer.qml"'Length of output: 44
Script:
#!/bin/bash # Description: Verify rg functionality and search for audio player references # Test 1: Verify rg is working by searching for a known term echo "Test 1: Verifying rg functionality" rg --type qml "import" # Test 2: Search for partial matches of OVOSAudioPlayer and AudioPlayer echo "Test 2: Searching for partial audio player matches" rg --type qml -i "ovosaudioplayer|audioplayer" # Test 3: Look for other audio player references echo "Test 3: Searching for general audio player references" rg --type qml -i "player|audio"Length of output: 53997
ovos_plugin_common_play/ocp/res/ui/+mediacenter/PlayerLoader.qml (1)
16-16: LGTM! The change aligns with the PR objective.The modification to the
backgroundAllowedPlayersproperty, removing"OVOSAudioPlayer.qml"and leaving only"OVOSSyncPlayer.qml", aligns with the PR objective to move towards deprecating the current audio GUI player in favor of the ovos-media framework.This change simplifies the audio handling process by ensuring that audio is managed entirely by the audio service, eliminating the need for special treatment or checks to determine the availability of the GUI. It reflects a strategic shift to untangle the existing GUI components from the audio handling logic, streamlining the overall architecture.
ovos_plugin_common_play/ocp/gui.py (1)
314-318: LGTM!The simplification of the
_get_player_pagemethod looks good. It aligns with the PR objective of deprecating the current audio GUI player by defaulting toself.audio_service_pagefor any backend that is not explicitlyPlaybackType.VIDEOorPlaybackType.WEBVIEW.
remove check for deprecated hard coded string
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
ovos_plugin_common_play/ocp/player.py (1)
Line range hint
505-532: Looks good, but please clarify the commented out MPRIS code.The
stopfunction correctly handles stopping playback based on the active backend. Clearing the playlist to prevent autoplay is a good practice, and the logic for each backend is implemented as expected.However, I noticed that the code for stopping the MPRIS player when the backend is
MPRISis commented out. Is there a specific reason for disabling it? If stopping the MPRIS player is safe and necessary, consider enabling that code block.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- ovos_plugin_common_play/ocp/init.py (2 hunks)
- ovos_plugin_common_play/ocp/player.py (2 hunks)
- ovos_plugin_common_play/ocp/search.py (5 hunks)
- requirements/requirements_extra.txt (0 hunks)
- test/unittests/test_ocp_player.py (4 hunks)
Files not reviewed due to no reviewable changes (1)
- requirements/requirements_extra.txt
Files skipped from review due to trivial changes (1)
- ovos_plugin_common_play/ocp/search.py
Additional comments not posted (6)
ovos_plugin_common_play/ocp/__init__.py (2)
329-330: Simplified notification logic for better user experience.The changes remove the unnecessary conditional check for the "smartspeaker" active extension, allowing the "Sorry, no matches found" notification to be displayed consistently. Setting the footer text to the same message ensures a coherent user experience.
347-347: Simplified notification logic for better user experience.The changes remove the unnecessary conditional check for the "smartspeaker" active extension, allowing the "Found a match" notification to be displayed consistently, thereby improving the user experience.
ovos_plugin_common_play/ocp/player.py (2)
Line range hint
425-457: LGTM!The
playfunction correctly handles playback based on the active backend. It validates the stream, updates the GUI, track history, and emits appropriate events. The logic for each backend is implemented as expected.
Line range hint
458-504: LGTM!The
play_nextfunction correctly handles playing the next track based on the active backend, loop state, and shuffle settings. It properly delegates to the MPRIS player or skill when applicable, and handles playlist navigation, repeat, and shuffle scenarios as expected. The logic for merging search results and stopping playback when there are no more tracks is implemented correctly.test/unittests/test_ocp_player.py (2)
Line range hint
277-298: LGTM! The test has been simplified and updated to reflect the new playback handling.The removal of the
@patchdecorator for mocking the GUI state simplifies the test by directly checking the behavior ofvalidate_streamwithout considering the GUI context. This change aligns with the shift towards an audio service-based playback handling, as evident from the updated expected playback state (PlaybackType.AUDIO_SERVICE).
Line range hint
416-483: LGTM! The test has been updated to expect an improved playback event communication.The changes in the expected message structure for the emitted messages during playback indicate a shift towards a more structured and informative approach to notifying about playback state and metadata changes. The new message types ("ovos.common_play.track.state" and "gui.player.media.service.set.meta") and the inclusion of track state and metadata in the message data suggest an enhancement in how the playback events and metadata are communicated.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- ovos_plugin_common_play/ocp/init.py (3 hunks)
- ovos_plugin_common_play/ocp/gui.py (9 hunks)
Additional context used
Ruff
ovos_plugin_common_play/ocp/__init__.py
1-1:
timeimported but unused(F401)
ovos_plugin_common_play/ocp/gui.py
12-12:
ovos_workshop.backwards_compat.MediaEntryimported but unusedRemove unused import:
ovos_workshop.backwards_compat.MediaEntry(F401)
Additional comments not posted (10)
ovos_plugin_common_play/ocp/__init__.py (1)
Line range hint
32-54: LGTM!The changes made in this code segment simplify the
OCPclass and improve the user experience. The removal of intent-related code suggests a move towards a more flexible architecture, which aligns with the PR objectives.ovos_plugin_common_play/ocp/gui.py (9)
168-172: LGTM!The method correctly shows a warning notification for playback errors and removes it after a short delay. The changes look good.
278-278: LGTM!Removing the search spinner before showing the home screen is a good practice to ensure a clean UI transition. The change looks good.
297-297: LGTM!Removing the search spinner before showing the player is a good practice to ensure a clean UI transition. The change is consistent with the approach taken in
show_homeand looks good.
310-315: LGTM!The method correctly determines the appropriate player page based on the active backend. The logic is straightforward and covers the necessary cases. The changes look good.
443-443: LGTM!Showing search results and releasing the screen after a timeout at the end of playback is a good user experience. The logic is straightforward and the changes look good.
444-445: LGTM!The method correctly updates the footer text and shows the busy page to notify the user about the search status. The logic is straightforward and the changes look good.
448-448: LGTM!Removing the busy page in the
remove_search_spinnermethod is consistent with its purpose. The change is straightforward and looks good.
496-496: LGTM!Removing the search spinner before showing the player screen in the
OCPExternalGuiInterfaceclass is consistent with the approach taken in theOCPMediaPlayerGUIclass. The change is straightforward and looks good.
12-12: ** Remove unused import.**The static analysis tool has correctly identified that
MediaEntryis imported but unused in the provided code. Please remove this import to keep the code clean.-from ovos_workshop.backwards_compat import (MediaType, Playlist, MediaEntry, PlayerState, LoopState, +from ovos_workshop.backwards_compat import (MediaType, Playlist, PlayerState, LoopState, PlaybackType, PluginStream, dict2entry)Tools
Ruff
12-12:
ovos_workshop.backwards_compat.MediaEntryimported but unusedRemove unused import:
ovos_workshop.backwards_compat.MediaEntry(F401)
let OCP define default stream extractors allow 1.X.X with mycroft compat removed OpenVoiceOS/ovos-ocp-audio-plugin#132
let OCP define default stream extractors allow 1.X.X with mycroft compat removed OpenVoiceOS/ovos-ocp-audio-plugin#132
let OCP define default stream extractors allow 1.X.X with mycroft compat removed OpenVoiceOS/ovos-ocp-audio-plugin#132
one more step towards deprecating this in favor of ovos-media and untangling the GUI
audio will now be handled fully by audio service instead of having special treatment and checking if GUI is available first
a QML player can be made as an audio plugin later to restore this functionality
since this is a breaking change also deleted all old code that become the ocp_pipeline in core, compat with mycroft/core<=0.0.7 is fully removed now
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores