Skip to content

feat(mpris): pin preferred media source - #3150

Open
scottsteely wants to merge 2 commits into
AvengeMedia:masterfrom
scottsteely:master
Open

feat(mpris): pin preferred media source#3150
scottsteely wants to merge 2 commits into
AvengeMedia:masterfrom
scottsteely:master

Conversation

@scottsteely

Copy link
Copy Markdown
Screenshot example

Description

Add a minimal "pin media source" option so the bar media widget and OSD keep showing a preferred player (e.g. a music player) even when another source (e.g. YouTube in the browser) starts playing.

  • MprisController: new pinnedIdentity property + setPinned(identity); _bestPlayingPlayer() prefers a playing pinned player (exact identity match, then case-insensitive substring fallback). Existing behavior is unchanged when no pin is set.
  • Persistence via SessionData.pinnedPlayerIdentity (spec entry added so SessionData.set() accepts the key); empty string clears the pin.
  • MediaDropdownOverlay (Media Players panel): each player row gets a theme-colored circle indicator on the right. Empty circle = no pin (initial state). Right-click a row to make it the default source (circle fills); right-click another row to move the pin; right-click the pinned row again to clear (back to empty). Left-click behavior unchanged (selects the player).

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • [ X] New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Refactor / internal cleanup
  • Documentation
  • Other

Related issues

Screenshots / video

Screenshot example

Checklist

  • [X ] My code follows the conventions in CONTRIBUTING.md
  • [X ] I have tested my changes locally
  • [X ] New user-facing strings are wrapped in I18n.tr() with translator context, reusing existing terms where possible
  • [ X] Go changes: ran make fmt, added/updated tests, make test passes, and go mod tidy is clean
  • [X ] QML changes: ran make lint-qml with no new warnings
  • I have opened a corresponding pull request in dlx-docs to document any new behaviors: https://github.com/AvengeMedia/DankLinux-Docs
    i'll do that last one next

Add a minimal "pin media source" option so the bar media widget and OSD
keep showing a preferred player (e.g. a music player) even when another
source (e.g. YouTube in the browser) starts playing.
 - MprisController: new pinnedIdentity property + setPinned(identity);
  _bestPlayingPlayer() prefers a playing pinned player (exact identity
  match, then case-insensitive substring fallback). Existing behavior
  is unchanged when no pin is set.
- Persistence via SessionData.pinnedPlayerIdentity (spec entry added so
  SessionData.set() accepts the key); empty string clears the pin.
- MediaDropdownOverlay (Media Players panel): each player row gets a
  theme-colored circle indicator on the right. Empty circle = no pin
  (initial state). Right-click a row to make it the default source
  (circle fills); right-click another row to move the pin; right-click
  the pinned row again to clear (back to empty). Left-click behavior
  unchanged (selects the player).
@hthienloc

Copy link
Copy Markdown
Member

/claude review

Comment thread quickshell/Services/MprisController.qml
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Claude review

One minor issue; the feature itself looks sound and existing selection behavior is preserved when no pin is set.

  • setPinned() imperatively assigns pinnedIdentity, destroying its binding to SessionData.pinnedPlayerIdentity (redundant, since SessionData.set() already propagates) — quickshell/Services/MprisController.qml:313

Checked: pin resolution order in _bestPlayingPlayer(), SessionData spec/persistence round-trip, delegate layout and right-click handling in MediaDropdownOverlay (no new user-facing strings, en.json/template.json untouched, no Go changes). Model: claude-opus-5.

Concur with Claude's suggestion.

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@bbedward

Copy link
Copy Markdown
Collaborator

/claude review

Comment on lines +251 to +256
if (pinnedIdentity !== "") {
const pinLower = pinnedIdentity.toLowerCase();
const pinned = playing.find(player => player.identity === pinnedIdentity) ?? playing.find(player => (player.identity || "").toLowerCase().includes(pinLower));
if (pinned)
return pinned;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pin branch returns before every canControl check the rest of this function is built around, so a pinned player that is playing but not controllable becomes activePlayer and the transport controls go dead — the existing code deliberately swaps to a same-track controllable mirror in that case (line 263).

This is reachable because identity is not unique: browsers register one MPRIS service per tab under the same identity ("Chromium", "Mozilla Firefox"), and find takes whichever comes first, which may be the uncontrollable instance.

Prefer a controllable match before falling back:

Suggested change
if (pinnedIdentity !== "") {
const pinLower = pinnedIdentity.toLowerCase();
const pinned = playing.find(player => player.identity === pinnedIdentity) ?? playing.find(player => (player.identity || "").toLowerCase().includes(pinLower));
if (pinned)
return pinned;
}
if (pinnedIdentity !== "") {
const pinLower = pinnedIdentity.toLowerCase();
const exact = playing.filter(player => player.identity === pinnedIdentity);
const matches = exact.length > 0 ? exact : playing.filter(player => (player.identity || "").toLowerCase().includes(pinLower));
const pinned = matches.find(player => player.canControl) ?? matches[0];
if (pinned)
return pinned;
}

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude review

One finding; the previously-flagged binding-destroying assignment in setPinned() is fixed, and the rest looks sound.

  • Pin resolution bypasses the canControl handling the rest of _bestPlayingPlayer() relies on, so a pinned-but-uncontrollable player (identities are not unique — one MPRIS service per browser tab) can take over and kill the transport controls — quickshell/Services/MprisController.qml:251

Checked: pin resolution order and interaction with the existing mirror/paused paths, SessionData spec + persistence round-trip (empty string clears, toJson drops defaults), delegate layout and right-click handling in MediaDropdownOverlay (imports, id scoping, no binding loop). No new user-facing strings; en.json/template.json untouched; no Go changes. Model: claude-opus-5.

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.

3 participants