refactor: dedupe artist image menu, extract CollectionActionItem - #2826
Closed
PonceGL wants to merge 2 commits into
Closed
refactor: dedupe artist image menu, extract CollectionActionItem#2826PonceGL wants to merge 2 commits into
PonceGL wants to merge 2 commits into
Conversation
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.
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. |
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
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.
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.kthas two mutually exclusive top bar implementations(
SharedArtistTopBarProbe/CustomCollapsingTopBar, picked byUseSharedCollapsibleTopBarProbe), each with its own trigger button and itsown
showImageMenustate — but theDropdownMenucontent itself (Changephoto / Reset to default: same strings, same icons, same
hasCustomImagebranch) 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, calledfrom both places. Each call site keeps its own trigger button and its own
remember { mutableStateOf(false) }— those genuinely differ(
FilledIconButtonvsSmallFloatingActionButton, different position) andaren't part of the duplication. Zero behavior change.
Commit 2 — extract CollectionActionItem
PlaylistActionItemwasprivateinsidePlaylistDetailScreen.kt,self-contained (icon, label, onClick — nothing playlist-specific), used only
in that file's options sheet. Moved to
presentation/components/CollectionActionItem.ktand renamed — it's a rowfor 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 inPlaylistDetailScreen.ktonce 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).
assembleDebugsucceeds, full JVMbaseline unaffected (5 pre-existing failures, none new).