Added spotify playback and other stuff - #83
sushant2812 wants to merge 4 commits into
Conversation
sushant2812
changed the title
ldelvoye
left a comment
ldelvoye
left a comment
There was a problem hiding this comment.
Overall the feature is really cool. I really appreciate the PR, it seems like your implementation agent took the liberty to overwrite important architectural and implementation quality rules (architecture.md, CONTRIBUTING.md). If things were unclear for your agent please let me know and I'll try to patch things up.
Lastly, this PR bundles four changes: a shell write seam, transport controls, search + picker, and album art with a new dependency. Those could definitely be shipped as separate PRs where we can validate and let features soak.
There was a problem hiding this comment.
architecture: this is the scratch CLI the ASCII module was ported from, this should be deleted?
| return | ||
| enabled = not state.shuffle | ||
|
|
||
| def work(credentials, http): |
There was a problem hiding this comment.
architecture: the panel is only supposed to draw, talking to spotify is the source's job, and deciding when to call the source is the shell's job. Here the panel picks the function and the arguments itself
There was a problem hiding this comment.
The rule that catches this (test_seams.py) was ignored
| self.panel = panel | ||
| self.item = item | ||
|
|
||
| class CredentialWorkRequested(Message): |
There was a problem hiding this comment.
The shell runs code it can't identify. The shell is meant to know what an integration is doing, so it can gate it later. Here it's handed a closure and told "run this", with no way to tell a shuffle toggle from anything else.
| from smorg.shell.refresh_indicator import RefreshIndicator | ||
| from smorg.shell.terminal_palette import StatusColors, status_colors, widget_background | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
The panel base class now knows about credentials. Credentials were only ever the shell's and auth's business. Now every panel's base class has them in its signature.
There was a problem hiding this comment.
Please don't change the architecture requirements
| def _run_mode_change(self, work, verb: str) -> None: | ||
| self._work_pending = True | ||
| self.post_message( | ||
| Panel.CredentialWorkRequested( |
There was a problem hiding this comment.
cost: refresh_on_success is True here, so one shuffle toggle costs the PUT, then player, queue, recently-played and the cover download again. Cache the cover by url, or skip the refresh for mode changes the panel already knows the outcome of
| dependencies = [ | ||
| "httpx>=0.28", | ||
| "keyring>=25.0", | ||
| "pillow>=12.3.0", |
There was a problem hiding this comment.
dependency: 12.3.0 is the version that happened to be installed, not the oldest that works. Also a hard dependency for one integration's cosmetic feature, worth a thought
| else: | ||
| repeat_glyph = "🔁 off" | ||
|
|
||
| columns = ( |
There was a problem hiding this comment.
code quality: these hints hardcode keys the manifest already declares. Change the manifest and the bar lies, read them from the actions instead
| work = queue_selected | ||
| elif isinstance(chosen, (Album, Playlist)): | ||
| if action is _SearchAction.QUEUE: | ||
| self.notify("only songs can be queued", severity="warning") |
There was a problem hiding this comment.
code quality: unreachable, the queue picker already filters to tracks
| | | MCP transport | REST transport | | ||
| | ---------------- | ------------- | -------------- | | ||
| | **OAuth** | Linear | — | | ||
| | **OAuth** | Linear | Spotify | |
There was a problem hiding this comment.
keep this row: Spotify was already OAuth + REST on main while the doc still called it a roadmap candidate. It's the prose above that has to go back, not this
This branch has not been deployed
2 participants
Added support for Spotify playback and: