Skip to content

Improve media ipc#3155

Open
Ape wants to merge 6 commits into
noctalia-dev:mainfrom
Ape:pr/improve-media-ipc
Open

Improve media ipc#3155
Ape wants to merge 6 commits into
noctalia-dev:mainfrom
Ape:pr/improve-media-ipc

Conversation

@Ape

@Ape Ape commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

This adds

media <..|play|next-player|previous-player>

and changes some logic in the mpris service so that it's a lot more convenient and behaves the way I would expect.

Type of Change

  • Bug fix
  • New feature

Manual Coverage

  • Tested on Hyprland

Checklist

  • This PR is ready for review, or it is marked as Draft.
  • I read and followed the relevant guidance in CONTRIBUTING.md.
  • I ran just format with clang-format v22+ installed.
  • I ran the relevant build or test commands.
  • I self-reviewed the changes.
  • I checked for new warnings or errors.
  • I will update end-user documentation after merge.
  • This PR adds no new user-facing strings.
  • I did not edit non-English translation files unless this PR is explicitly for translation tooling, an import/export sync, or a maintainer-requested locale change.
  • I used the existing canonical names for config keys, IPC names, paths, and identifiers.

Ape added 6 commits June 26, 2026 16:12
In MPRIS Pause is the generally useful and expected command instead of
Stop. Stop is very inconsistently implemented in players and
semantically it includes clearing the track position. Also, `media
toggle` already uses pause semantics instead of stop.

We might want to consider renaming our ipc command to `media pause`, but
I don't think it matters much. I don't think anyone actually wants the
Stop command for anything.
I was consistently having issues where my paused player was marked as
playing.
Automatically clear the pinned player preference when another player is
playing and the pinned player is not. This lets an explicit player
choice stay active while relevant, but avoids a paused pinned player
blocking active playback from another source.
@Ape Ape force-pushed the pr/improve-media-ipc branch from 0fed9c8 to 16eb24c Compare June 26, 2026 13:12
@ItsLemmy

Copy link
Copy Markdown
Collaborator

Review: PR #3155 — "Improve media ipc"

What it does: Adds media play, media next-player, and media previous-player IPC actions; reworks stop() to prefer Pause over Stop (and no longer dismiss the player); and reworks applyPlayerSnapshot so the active/pinned player auto-follows whichever player starts/stops playing. It also drops two pause-recovery playbackStatus = "Playing" overrides. Overall a reasonable UX improvement, but it leaves orphaned subsystems and reverts a deliberate heuristic without explanation.

Findings (most severe first)

  1. mpris_service.cpp:785 (removed) — Removing the sole dismissPlayer() call orphans the entire "dismissed players" subsystem.
    stop() was the only caller of dismissPlayer(), and dismissPlayer() is the only place that inserts into m_stoppedPlayers (line 2185). After this PR m_stoppedPlayers can never become non-empty, so every isDismissed() guard in chooseActivePlayer() (lines 2062, 2071, 2091, 2100, 2108, 2115), the erases at 770/1746/2034, and dismissPlayer() itself (2179) all become unreachable dead code. Either the dismiss feature is being intentionally retired — in which case this dead machinery should be deleted in this PR — or it's an unintended loss of "stop removes the player from active rotation." Worth confirming which, and cleaning up accordingly.

  2. mpris_service.cpp:559 and :1606 (removed) — Dropping playbackStatus = "Playing" reverts the no-signal-pause recovery heuristic.
    Both removed lines corrected a player that falsely reports Paused while its position keeps advancing past kPauseRecoveryMinJumpUs (a known quirk on some players/compositors). With them gone, the model leaves such a player at Paused (only the m_recentNoSignalPauseAt erase remains). Since chooseActivePlayer()'s "most recent playing" path (line 2071) only considers playbackStatus == "Playing", an actually-playing-but-stuck-Paused player drops out of active selection — the exact bug that heuristic was added to fix. This may be intentional given the new active-follow logic, but it's a deliberate heuristic being reverted with no mention in the PR; the author should confirm intent.

  3. mpris_service.cpp (new } else if (clearedPinnedPlayerPreference) tail of applyPlayerSnapshot) — Unreachable branch.
    clearedPinnedPlayerPreference is only ever set inside the becamePlaying or transitionedFromPlaying paths, both of which require playbackStatus to differ between existing->second and info — which guarantees existing->second != info, so the outer if always wins. The else if (the existing->second == info case) can never run with clearedPinnedPlayerPreference == true. Harmless defensive code, but dead; can be dropped.

Design note (not a bug)

The new becamePlaying clear logic resets an explicit user pin (media next-player) as soon as any other player starts playing while the pinned one isn't playing. That matches the "active follows playback" intent, but it does mean an explicit pin to a paused player is silently overridden by background playback — worth a sentence in the PR/commit so it's a documented choice rather than a surprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants