Skip to content

refactor: dedupe artist image menu, extract CollectionActionItem - #2826

Closed
PonceGL wants to merge 2 commits into
PixelPlayerHQ:masterfrom
PonceGL:chore/p2-p3-artist-menu-and-collection-action-item
Closed

refactor: dedupe artist image menu, extract CollectionActionItem#2826
PonceGL wants to merge 2 commits into
PixelPlayerHQ:masterfrom
PonceGL:chore/p2-p3-artist-menu-and-collection-action-item

Conversation

@PonceGL

@PonceGL PonceGL commented Sep 10, 2026

Copy link
Copy Markdown

Two small independent cleanups from #2813's checklist, combined here since
both are cosmetic and unrelated in code area — two atomic commits, one PR.

Commit 1 — dedupe the artist image-edit dropdown menu

ArtistDetailScreen.kt has two mutually exclusive top bar implementations
(SharedArtistTopBarProbe / CustomCollapsingTopBar, picked by
UseSharedCollapsibleTopBarProbe), each with its own trigger button and its
own showImageMenu state — but the DropdownMenu content itself (Change
photo / Reset to default: same strings, same icons, same hasCustomImage
branch) was copy-pasted between them.

Not an overflow menu, as an earlier draft of this fix assumed — it's the
image-edit menu; neither screen has an overflow menu at all.

Extracted the shared content into a private ArtistImageEditMenu, called
from both places. Each call site keeps its own trigger button and its own
remember { mutableStateOf(false) } — those genuinely differ
(FilledIconButton vs SmallFloatingActionButton, different position) and
aren't part of the duplication. Zero behavior change.

Commit 2 — extract CollectionActionItem

PlaylistActionItem was private inside PlaylistDetailScreen.kt,
self-contained (icon, label, onClick — nothing playlist-specific), used only
in that file's options sheet. Moved to
presentation/components/CollectionActionItem.kt and renamed — it's a row
for any collection's action list, not just playlists.

This one is a real prerequisite, not just cleanup: the planned album/artist
collection-detail screens are expected to reuse this same row. Dropped two
imports (Painter, clickable) left orphaned in PlaylistDetailScreen.kt
once the only code using them moved out.

Testing

No new tests — both are pure extract-method refactors, verified by hand
against the originals for behavioral equivalence (same items, same order,
same click handling, same layout). assembleDebug succeeds, full JVM
baseline unaffected (5 pre-existing failures, none new).

SharedArtistTopBarProbe and CustomCollapsingTopBar are two mutually
exclusive top bar implementations (picked by
UseSharedCollapsibleTopBarProbe), each with its own trigger button and
its own showImageMenu state — but the DropdownMenu content itself
(Change photo / Reset to default, same strings, same icons, same
hasCustomImage branch) was copy-pasted between them. Not an overflow
menu, as an earlier plan draft called it — it's the image-edit menu.

Extracted the shared content into a private ArtistImageEditMenu, called
from both places. Each call site keeps its own trigger button and its
own remember { mutableStateOf(false) } — those differ (FilledIconButton
vs SmallFloatingActionButton, different position) and aren't part of
the duplication.

Zero behavior change: same items, same order, same onClick, same
dismiss-then-callback sequencing.
…tDetailScreen

PlaylistActionItem was private inside PlaylistDetailScreen.kt, self-
contained (icon, label, onClick — nothing playlist-specific), used
only in that file's options sheet. Moved to
presentation/components/CollectionActionItem.kt and renamed — it's a
row for any library collection's action list, not just playlists.

F4's collection-detail work (album and artist screens) is expected to
reuse this same row (F4.md line 326), so this preparatory extraction
is a real prerequisite there, unlike P.2 and P.1.

Also dropped two imports (Painter, clickable) left orphaned in
PlaylistDetailScreen.kt now that the only code using them moved out.

Zero behavior change: same layout, same colors, same 4 call sites,
same arguments.
@PonceGL

PonceGL commented Sep 10, 2026

Copy link
Copy Markdown
Author

Closing for now — reorganizing how this work is staged. It'll go through our fork first and we'll propose it upstream again, possibly bundled differently, once the larger feature it's part of is further along. Not a rejection, just a process change on our side.

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.

1 participant