fix/drop_OCP_mycroft_hacks #80
Conversation
OCP was loaded as a regular audio plugin, this was the only way to inject it into mycroft-core in OVOS this is no longer needed and OCP is loaded directly as part of ovos-audio (until ovos-media is released) the compat layer is dropped to avoid issues in edge cases, such as it getting selected as the default audio plugin if no others are installed, which can cause an infinite loop at playback time
WalkthroughThe recent changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AudioService
participant OCPAudioBackend
Client->>AudioService: load_services()
AudioService->>AudioService: find_default()
AudioService->>OCPAudioBackend: Instantiate with configs
OCPAudioBackend-->>AudioService: Instance created
AudioService->>AudioService: Setup event handlers
AudioService-->>Client: Default audio backend set
Poem
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 as PR comments)
Additionally, you can add 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_audio/audio.py (3 hunks)
Additional context used
Ruff
ovos_audio/audio.py
102-102: Do not use bare
except(E722)
Additional comments not posted (2)
ovos_audio/audio.py (2)
Line range hint
118-125:
LGTM!The changes to the
find_defaultmethod are straightforward and correct.
151-151: LGTM! But verify the impact of removing thedisable_ocpflag.The changes to the
load_servicesmethod are correct. However, ensure that removing thedisable_ocpflag doesn't affect other parts of the codebase.
actually call find_ocp()... forgot that in previous commit
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_audio/audio.py (4 hunks)
Additional context used
Ruff
ovos_audio/audio.py
104-104: Do not use bare
except(E722)
Additional comments not posted (2)
ovos_audio/audio.py (2)
Line range hint
108-117:
LGTM!The changes ensure that the default backend is always set up, improving reliability.
Tools
Ruff
104-104: Do not use bare
except(E722)
Line range hint
130-158:
LGTM!The changes simplify the logic and ensure that the default backend is always set up, improving reliability. Additionally, the method now sets up event handlers for audio playback, which is essential for functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- ovos_audio/audio.py (4 hunks)
Additional context used
Ruff
ovos_audio/audio.py
109-109: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
Additional comments not posted (2)
ovos_audio/audio.py (2)
113-121: Ensure proper handling of missing default backend.The function should handle the case where no default backend is found more explicitly.
Ensure that the removal of the
disable_ocpflag check does not cause unintended side effects.- if not self.service: + if not self.service or not self.default: LOG.error("No audio player plugins found!") return False
159-163: Ensure compatibility with future releases.The comment indicates that the current implementation will be replaced by
ovos-mediain a future release. Ensure that the transition is smooth and backward compatibility is maintained.Ensure that the removal of the
disable_ocpflag does not cause unintended side effects.
OCP was loaded as a regular audio plugin, this was the only way to inject it into mycroft-core
in OVOS this is no longer needed and OCP is loaded directly as part of ovos-audio (until ovos-media is released)
the compat layer is dropped to avoid issues in edge cases, such as it getting selected as the default audio plugin if no others are installed, which can cause an infinite loop at playback time
Summary by CodeRabbit
disable_ocpflag.