Beam -> Remote with less restrictions #1221
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR replaces the legacy Beam output pathway with a generic Remote mechanism by removing Beam-specific branches, introducing a new Remote plugin and settings UI, implementing a Sequence diagram for remote metadata ingestion via /v1/remoteinputsequenceDiagram
participant Client as Remote Client
participant Server as Webserver
participant REMOTEDB as Remote MetadataDB
Client->>Server: POST /v1/remoteinput (metadata, optional secret)
Server->>Server: Validate JSON
alt Secret required
Server->>Server: Validate secret
alt Secret invalid
Server-->>Client: 403 Forbidden
end
end
Server->>REMOTEDB: write_to_metadb(metadata)
REMOTEDB-->>Server: (dbid)
Server-->>Client: 200 OK (dbid)
Sequence diagram for TrackPoll sending metadata to remote serversequenceDiagram
participant TrackPoll
participant RemoteServer as Remote Server
participant RemoteDB as Remote MetadataDB
TrackPoll->>RemoteServer: POST /v1/remoteinput (metadata, optional secret)
alt Success
RemoteServer->>RemoteDB: write_to_metadb(metadata)
RemoteDB-->>RemoteServer: (dbid)
RemoteServer-->>TrackPoll: 200 OK (dbid)
else Auth failure
RemoteServer-->>TrackPoll: 403 Forbidden
else Server error
RemoteServer-->>TrackPoll: 500 Error
end
Class diagram for new Remote input plugin and settingsclassDiagram
class Plugin {
+str displayname
+str remotedbfile
+MetadataDB remotedb
+str mixmode
+event_handler
+TrackMetadata metadata
+observer
+__init__(config, qsettings)
+install()
+_reset_meta()
+defaults(qsettings)
+setup_watcher()
+_read_track(event)
+start()
+getplayingtrack()
+getrandomtrack(playlist)
+stop()
+on_m3u_dir_button()
+connect_settingsui(qwidget, uihelp)
+load_settingsui(qwidget)
+verify_settingsui(qwidget)
+save_settingsui(qwidget)
+desc_settingsui(qwidget)
}
Plugin --|> InputPlugin
class BeamSettings {
+config
+widget
+connect(uihelp, widget)
+load(config, widget)
+_on_enable_toggled(enabled)
+save_settings()
+verify(widget)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
+ Coverage 66.73% 67.07% +0.34%
==========================================
Files 68 70 +2
Lines 11527 11713 +186
==========================================
+ Hits 7692 7856 +164
- Misses 3835 3857 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes - here's some feedback:
- Consider removing or gating the
/tmp/remote_debug.jsondump in_write_to_remotebehind a verbose or debug flag to avoid writing unexpected files in production. - The
BeamSettingsclass innowplaying/remote_settings.pynow configures remote output—renaming it toRemoteSettings(and updating references) would improve clarity and avoid confusion. - There’s a lot of repeated code in
SettingsUIaround looping over plugin keys; extracting the widget setup/connect loops into a helper could reduce boilerplate and make future maintenance easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing or gating the `/tmp/remote_debug.json` dump in `_write_to_remote` behind a verbose or debug flag to avoid writing unexpected files in production.
- The `BeamSettings` class in `nowplaying/remote_settings.py` now configures remote output—renaming it to `RemoteSettings` (and updating references) would improve clarity and avoid confusion.
- There’s a lot of repeated code in `SettingsUI` around looping over plugin keys; extracting the widget setup/connect loops into a helper could reduce boilerplate and make future maintenance easier.
## Individual Comments
### Comment 1
<location> `nowplaying/processes/trackpoll.py:426` </location>
<code_context>
+ async with aiohttp.ClientSession() as session:
</code_context>
<issue_to_address>
No error handling for network failures or unreachable remote server.
Wrap the session.post call in a try/except block to handle potential aiohttp exceptions and ensure graceful error handling.
</issue_to_address>
### Comment 2
<location> `nowplaying/remote_settings.py:72` </location>
<code_context>
+
+ def save_settings(self):
+ """Save settings to configuration."""
+ if not self.config or not self.widget:
+ return
+
</code_context>
<issue_to_address>
The save_settings method silently returns if config or widget is missing.
Consider logging a warning or raising an exception when config or widget is missing to improve error visibility.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def save_settings(self):
"""Save settings to configuration."""
if not self.config or not self.widget:
return
=======
def save_settings(self):
"""Save settings to configuration."""
if not self.config or not self.widget:
logging.warning(
"Cannot save settings: %s is missing.",
"config" if not self.config else "widget"
)
return
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `nowplaying/remote_settings.py:104` </location>
<code_context>
+ def verify(widget: 'QWidget') -> bool:
+ """Verify beam settings."""
+ # Basic validation - could be extended later
+ if widget.enable_checkbox.isChecked():
+ if not widget.server_lineedit.text().strip():
+ raise PluginVerifyError("Server field is required when beam output is enabled")
+ return True
</code_context>
<issue_to_address>
The verify method only checks for a non-empty server field when enabled, but does not validate port or secret.
Also validate that the port is a valid integer within the allowed range and ensure the secret is not empty if required, to avoid misconfiguration.
</issue_to_address>
### Comment 4
<location> `nowplaying/inputs/remote.py:27` </location>
<code_context>
+ qsettings: QWidget | None = None):
+ super().__init__(config=config, qsettings=qsettings)
+ self.displayname = "Remote"
+ self.remotedbfile: str = config.cparser.value('remote/remotedb', type=str)
+ self.remotedb: nowplaying.db.MetadataDB | None = None
+ self.mixmode = "newest"
</code_context>
<issue_to_address>
No default value is provided for remotedbfile if the config is missing the key.
If 'remote/remotedb' is missing, remotedbfile will be None, which may cause errors. Please add a default value or handle the missing key appropriately.
</issue_to_address>
### Comment 5
<location> `nowplaying/inputs/remote.py:57` </location>
<code_context>
+ return
+
+ logging.info("Opening %s for input", self.remotedbfile)
+ self.observer = nowplaying.db.DBWatcher(databasefile=self.remotedbfile)
+ self.observer.start(customhandler=self._read_track)
+
</code_context>
<issue_to_address>
No error handling if the database file does not exist or is inaccessible.
Add error handling for cases where remotedbfile is invalid or missing to ensure DBWatcher fails gracefully with a clear message or fallback.
</issue_to_address>
### Comment 6
<location> `tests/test_trackpoll.py:211` </location>
<code_context>
+ config.cparser.setValue('remote/enabled', False)
+ config.cparser.sync()
+
+ trackpoll = nowplaying.processes.trackpoll.TrackPoll(stopevent=threading.Event(),
+ config=config,
+ testmode=True)
</code_context>
<issue_to_address>
Tests use aioresponses to mock HTTP requests, but do not assert on the request payload.
Consider adding assertions to verify that the payload sent matches the expected metadata, both when the secret is present and absent, to ensure correct data transmission.
Suggested implementation:
```python
from aioresponses import aioresponses
@pytest.mark.asyncio
async def test_trackpoll_write_to_remote_payload(trackpollbootstrap):
''' test _write_to_remote payload with and without secret '''
config = trackpollbootstrap
config.cparser.setValue('remote/enabled', True)
config.cparser.setValue('remote/url', 'http://example.com/metadata')
config.cparser.sync()
# Test metadata
metadata = {
"artist": "Test Artist",
"title": "Test Title",
"album": "Test Album"
}
# Case 1: Secret present
config.cparser.setValue('remote/secret', 'supersecret')
config.cparser.sync()
with aioresponses() as m:
m.post('http://example.com/metadata', status=200)
trackpoll = nowplaying.processes.trackpoll.TrackPoll(
stopevent=threading.Event(),
config=config,
testmode=True
)
await trackpoll._write_to_remote(metadata)
assert m.called
request = m.requests[('POST', 'http://example.com/metadata')][0]
sent_json = request.kwargs['json']
assert sent_json['artist'] == "Test Artist"
assert sent_json['title'] == "Test Title"
assert sent_json['album'] == "Test Album"
assert sent_json['secret'] == "supersecret"
# Case 2: Secret absent
config.cparser.removeOption('remote', 'secret')
config.cparser.sync()
with aioresponses() as m:
m.post('http://example.com/metadata', status=200)
trackpoll = nowplaying.processes.trackpoll.TrackPoll(
stopevent=threading.Event(),
config=config,
testmode=True
)
await trackpoll._write_to_remote(metadata)
assert m.called
request = m.requests[('POST', 'http://example.com/metadata')][0]
sent_json = request.kwargs['json']
assert sent_json['artist'] == "Test Artist"
assert sent_json['title'] == "Test Title"
assert sent_json['album'] == "Test Album"
assert 'secret' not in sent_json
# Properly shut down trackpoll to avoid Windows timing issues
await trackpoll.stop()
await asyncio.sleep(0.1) # Brief pause to let cleanup finish
```
- Ensure that `nowplaying.processes.trackpoll.TrackPoll._write_to_remote` is an async method and can be called directly in the test.
- If the remote URL or payload structure is different in your codebase, adjust the assertions accordingly.
- If you use a different config or mocking setup, adapt the test to fit your conventions.
</issue_to_address>
### Comment 7
<location> `docs/input/remote.md:10` </location>
<code_context>
+forth!
+
+**What's Now Playing** supports a configuration where each setup has
+their own app configuration running. One or more installation on
+DJ computers sending the track information to a central one. That
+central one will then perform any additional lookups and send the
</code_context>
<issue_to_address>
Typo: 'installation' should be 'installations'.
The sentence should read: 'One or more installations on DJ computers send the track information to a central one.'
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
their own app configuration running. One or more installation on
DJ computers sending the track information to a central one. That
central one will then perform any additional lookups and send the
=======
their own app configuration running. One or more installations on
DJ computers send the track information to a central one. That
central one will then perform any additional lookups and send the
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| config.cparser.setValue('remote/enabled', False) | ||
| config.cparser.sync() | ||
|
|
||
| trackpoll = nowplaying.processes.trackpoll.TrackPoll(stopevent=threading.Event(), |
There was a problem hiding this comment.
suggestion (testing): Tests use aioresponses to mock HTTP requests, but do not assert on the request payload.
Consider adding assertions to verify that the payload sent matches the expected metadata, both when the secret is present and absent, to ensure correct data transmission.
Suggested implementation:
from aioresponses import aioresponses
@pytest.mark.asyncio
async def test_trackpoll_write_to_remote_payload(trackpollbootstrap):
''' test _write_to_remote payload with and without secret '''
config = trackpollbootstrap
config.cparser.setValue('remote/enabled', True)
config.cparser.setValue('remote/url', 'http://example.com/metadata')
config.cparser.sync()
# Test metadata
metadata = {
"artist": "Test Artist",
"title": "Test Title",
"album": "Test Album"
}
# Case 1: Secret present
config.cparser.setValue('remote/secret', 'supersecret')
config.cparser.sync()
with aioresponses() as m:
m.post('http://example.com/metadata', status=200)
trackpoll = nowplaying.processes.trackpoll.TrackPoll(
stopevent=threading.Event(),
config=config,
testmode=True
)
await trackpoll._write_to_remote(metadata)
assert m.called
request = m.requests[('POST', 'http://example.com/metadata')][0]
sent_json = request.kwargs['json']
assert sent_json['artist'] == "Test Artist"
assert sent_json['title'] == "Test Title"
assert sent_json['album'] == "Test Album"
assert sent_json['secret'] == "supersecret"
# Case 2: Secret absent
config.cparser.removeOption('remote', 'secret')
config.cparser.sync()
with aioresponses() as m:
m.post('http://example.com/metadata', status=200)
trackpoll = nowplaying.processes.trackpoll.TrackPoll(
stopevent=threading.Event(),
config=config,
testmode=True
)
await trackpoll._write_to_remote(metadata)
assert m.called
request = m.requests[('POST', 'http://example.com/metadata')][0]
sent_json = request.kwargs['json']
assert sent_json['artist'] == "Test Artist"
assert sent_json['title'] == "Test Title"
assert sent_json['album'] == "Test Album"
assert 'secret' not in sent_json
# Properly shut down trackpoll to avoid Windows timing issues
await trackpoll.stop()
await asyncio.sleep(0.1) # Brief pause to let cleanup finish- Ensure that
nowplaying.processes.trackpoll.TrackPoll._write_to_remoteis an async method and can be called directly in the test. - If the remote URL or payload structure is different in your codebase, adjust the assertions accordingly.
- If you use a different config or mocking setup, adapt the test to fit your conventions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a new Remote input/output feature to replace legacy Beam functionality, including a Remote plugin, settings UI, webserver endpoint, and TrackPoll support; remove Beam-specific code, update docs, add tests, and bump Python support
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: