fix: guard media template seek/re-entry, add capability flags - #430
fix: guard media template seek/re-entry, add capability flags#430JarbasAl wants to merge 1 commit into
Conversation
seek_forward/seek_backward crashed with TypeError when a backend's get_track_position() returned None; they now log and no-op instead. ocp_start is now idempotent while playback is already active, so a re-entrant call no longer double-emits LOADED_MEDIA/PLAYING or calls play() twice. MediaBackend also gains supports_seek and supports_pause class attributes (both default True) so consumers can check backend capability before calling seek/pause.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Checking back in with the latest test results. 📡I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewThe draft for the big day is ready for review. 📝 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
⚖️ License CheckEnsuring our licenses allow for commercial use. 🏢 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📋 Repo HealthChecking the repo's mental health (aka developer happiness). 😊 ✅ All required files present. Latest Version: ✅ 🔍 LintThe latest check report is now ready. 📝 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)Ensuring our certificates are valid and trustworthy. 📜 ✅ No known vulnerabilities found (65 packages scanned). 📊 CoverageHow much of the logic is under the microscope? 🔬 ✅ 82.8% total coverage Files below 80% coverage (23 files)
Full report: download the 🔨 Build TestsTesting the integrity of the build artifacts. 🏺
❌ 3.10: Install OK, tests failed Your digital assistant in the world of OVOS 🤖 |
1 participant
This fixes three small defects in
ovos_plugin_manager/templates/media.py, the base class OCP media backends subclass.seek_forwardandseek_backwardcalledself.get_track_position() + mswith no guard on the return value. A backend that legitimately has no track position yet (nothing loaded, a live stream, a backend still starting up) returnsNone, and the arithmetic raisesTypeError. Both methods now check forNone, log at debug level, and return without seeking.ocp_startre-emittedPLAYER.PLAYINGandMEDIA.LOADED_MEDIAand calledplay()again every time it was invoked, even if playback was already active. A caller that invokesocp_starttwice in a row — which does happen given the eventual consistency of bus messaging — would double-start playback.ocp_startis now idempotent: a_ocp_playingflag tracks whether playback is already active, and a re-entrant call is a no-op untilocp_stop,ocp_error, or a newload_trackresets it. TheAudioPlayerBackend/VideoPlayerBackend/WebPlayerBackendsubclasses, which each emit an extra track-state message on top of the base call, were updated the same way so they don't double-emit either.MediaBackendgains two class attributes,supports_seekandsupports_pause, both defaulting toTrue. They're purely additive — existing plugins are unaffected — and let backends that genuinely can't seek or pause (a live stream, for instance) declare that so callers can check before calling.ovos-media currently guards against all three of these at every call site that touches a backend. None of that guarding is removed here; this just makes the template itself correct, so those call-site guards become redundant rather than load-bearing, and can be simplified later without changing behavior.
Tests: added adversarial cases to
test/unittests/test_media_templates.pycovering aNoneposition on both seek directions, re-entrantocp_start(base class andAudioPlayerBackend),ocp_startresuming correctly afterocp_stopand after a newload_track, and the new capability-flag defaults. Every new test was confirmed to fail against the unfixed template (reverted via patch) and pass after the fix. Full existing suite: 969 passed, 3 pre-existing failures intest_deprecation_noise.py(unrelated — deprecation-warning counts forsolvers/OpenVoiceOSPlugin, untouched by this change) unchanged before and after;test_hardware.py/test_led_animations.pyskipped, missing the optionalovos-hardware-helpersdependency, also unrelated.