[Help Wanted] feat: add cover flow to player#653
Draft
tvillega wants to merge 10 commits into
Draft
Conversation
Contributor
Author
|
@eddyizm please add a help wanted flag to this PR. I hope somebody can help attaching the queue to this new UI. |
This is exactly the same as before, but with an MVVM architecture patter with the following flow: Fragment -> RecyclerView -> Adapter -> ViewModel -> Repository -> Glide (data fetch). Here comes the fun part. setupCoverFlow (ran in the fragment when the UI is built), must be fed with the coverArt id recollected in the repository (all the way down on the MVVM chain). I stopped here because I have a confusion on the Repository -> Glide MVVM chain. The image fetch is done on the fragment, so the repository must read the queue itself to get the coverArt id?
Contributor
Author
|
With the new scaffolding, this is how // Fragment is built (player UI), then this is run
public void onViewCreated(@NonNull View root, @Nullable Bundle savedInstanceState) {
super.onViewCreated(root, savedInstanceState);
playerCoverFlow = root.findViewById(R.id.player_cover_flow);
CoverRepository repository = new MediaBrowserCoverRepository(mediaBrowserListenableFuture);
CoverFlowViewModelFactory factory = new CoverFlowViewModelFactory(repository);
viewModel = new ViewModelProvider(this, factory)
.get(CoverFlowViewModel.class);
viewModel.getCovers().observe(getViewLifecycleOwner(), this::setupCoverFlow);
}
// Last line of the previous code hooks to this function to populate covers when data arrives
private void setupCoverFlow(@NonNull List<Cover> covers) {
if (covers.isEmpty()) return;
CoverFlowAdapter adapter = new CoverFlowAdapter(
requireContext(),
covers,
(cover, imageView) -> {
String coverArtId = cover.getCoverArtId();
if (coverArtId != null) {
CustomGlideRequest.Builder
.from(requireContext(),
coverArtId,
CustomGlideRequest.ResourceType.Song)
.build()
.into(imageView);
}
});
playerCoverFlow.setAdapter(adapter);
playerCoverFlow.setLayoutManager(
new LinearLayoutManager(requireContext(),
LinearLayoutManager.HORIZONTAL,
false));
playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32));
playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll());
UIUtil.centerAndSnapRecyclerView(playerCoverFlow);
}I followed Android's MVVM architecture pattern (which other parts of this app already follow), summarized as: The snippet above corresponds to public List<Cover> getCovers() throws Exception {
MediaBrowser mediaBrowser = mediaBrowserFuture.get();
MediaMetadata metadata = mediaBrowser.getMediaMetadata(); // <- Unused var
List<String> urls = Arrays.asList(
"https://images.dog.ceo/breeds/affenpinscher/n02110627_11858.jpg",
"https://images.dog.ceo/breeds/hound-english/n02089973_811.jpg",
"https://images.dog.ceo/breeds/shiba/shiba-14.jpg"
);
List<Cover> covers = new ArrayList<>();
for (String url : urls) {
covers.add(new Cover(url, null));
}
return covers;
}Those hardcoded URL's are the mockup that feed the recycler view. The If an URL is not provided and a coverArtId is provided instead, I don't know which one of them will be useful, but the one that stays will be sent to the Repository file. What's next?Find out how to extract either the An alternative path I just discovered is to not create the MVVM arch model for CoverFlow and instead hook to the existent MVVM from I actually like more this approach, but I took the long path to understand how that even works in the first place. |
Contributor
📝 WalkthroughWalkthroughIntroduces a horizontal cover-flow RecyclerView in the player controller screen. A new ChangesCover-flow RecyclerView feature
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over PlayerControllerFragment,MediaBrowserCoverRepository: Fragment setup on onViewCreated
end
participant PlayerControllerFragment
participant CoverFlowViewModel
participant MediaBrowserCoverRepository
participant MediaBrowser
participant CoverFlowAdapter
participant UIUtil
PlayerControllerFragment->>MediaBrowserCoverRepository: new(mediaBrowserFuture)
PlayerControllerFragment->>CoverFlowViewModel: create via factory(repository)
CoverFlowViewModel->>MediaBrowserCoverRepository: getCovers() on executor
MediaBrowserCoverRepository->>MediaBrowser: mediaBrowserFuture.get()
MediaBrowser-->>MediaBrowserCoverRepository: MediaBrowser + MediaMetadata
MediaBrowserCoverRepository-->>CoverFlowViewModel: List<Cover> (hardcoded URLs)
CoverFlowViewModel-->>PlayerControllerFragment: LiveData posts List<Cover>
PlayerControllerFragment->>CoverFlowAdapter: new(context, covers, OnCoverBoundListener)
PlayerControllerFragment->>UIUtil: horizontalSpacing() + scaleOnScroll()
PlayerControllerFragment->>UIUtil: centerAndSnapRecyclerView(playerCoverFlow)
UIUtil-->>PlayerControllerFragment: PagerSnapHelper + scroll listener attached
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java (1)
198-208: ⚡ Quick winAvoid issuing two image loads into the same
ImageViewduring bind.
CoverFlowAdapter.bind(...)already loadscover.getUrl(), and this callback may immediately issueCustomGlideRequestinto the same view. Prefer one source-selection path (coverArtIdvs URL) to avoid redundant work/flicker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java` around lines 198 - 208, The callback in PlayerControllerFragment is issuing a Glide image load into the same ImageView that CoverFlowAdapter.bind() already populates with cover.getUrl(). This creates redundant image loads and potential flickering. Refactor the logic to use a single source-selection path: check if coverArtId exists and use CustomGlideRequest if it does, otherwise rely on the adapter's URL-based load. Consider consolidating this decision into the bind method or the adapter itself rather than having competing load requests target the same ImageView.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/cappielloantonio/tempo/repository/MediaBrowserCoverRepository.java`:
- Around line 27-51: The method retrieves metadata at the beginning but then
ignores it, instead returning a hardcoded list of dog image URLs in the covers
loop. Replace the static URL list creation (the Arrays.asList with three dog
image URLs) with logic that extracts the actual coverArtId from the metadata
object using metadata.extras.getString("coverArtId") and creates Cover objects
based on real playback data instead of placeholder URLs. This will ensure the
repository returns actual cover art data synchronized with the queue state.
- Around line 26-27: The mediaBrowserFuture.get() call in the media browser
cover loading code has no timeout specified, which can cause the application to
hang indefinitely if the future never completes. Replace the unbounded
mediaBrowserFuture.get() call with the overloaded version that accepts a timeout
duration and time unit parameters (e.g., get with a reasonable timeout like 5 or
10 seconds) to ensure that if the future doesn't resolve within that period, a
TimeoutException is thrown and the cover loading can fail gracefully instead of
hanging.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/adapter/CoverFlowAdapter.java`:
- Around line 47-52: The onBindViewHolder method in CoverFlowAdapter is
triggering two separate image load requests for the same ImageView: one inside
holder.bind(cover) which loads cover.getUrl(), and another through
boundListener.onCoverBound(cover, holder.coverImage) for items with coverArtId.
To fix this, refactor the code to use a single image loading path. Either pass
the necessary coverArtId information to the bind method so it can make a single
optimized request, or conditionally call boundListener only when a custom image
request is specifically needed (avoiding the automatic load in bind). Apply this
same consolidation pattern to all occurrences of this binding logic including
the location around lines 70-75.
- Line 15: The import statement in CoverFlowAdapter.java is using
org.jspecify.annotations.NonNull, but JSpecify is not a declared dependency and
this creates inconsistency with the rest of the codebase which uses
androidx.annotation.NonNull. Replace the import statement at line 15 that
currently imports from org.jspecify.annotations with an import from
androidx.annotation.NonNull to align with the project's nullness checking
toolchain and match other files like UIUtil.java.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java`:
- Around line 175-179: The MediaBrowserCoverRepository is being instantiated
before mediaBrowserListenableFuture is initialized, which causes it to receive a
null value that will crash when used. Move the initialization of
mediaBrowserListenableFuture to occur before the MediaBrowserCoverRepository is
created in the constructor, or defer the CoverFlowViewModel creation (the
ViewModelProvider.get(CoverFlowViewModel.class) call) until after the
initializeBrowser() method has been called at line 320, ensuring the
mediaBrowserListenableFuture is properly initialized before being passed to the
MediaBrowserCoverRepository constructor.
- Around line 188-223: The setupCoverFlow method is being called repeatedly
(likely from a LiveData observer) and accumulating item decorations, scroll
listeners, and recreating the snap helper on each call, causing state conflicts
and wasted resources. Introduce a boolean flag to guard the one-time
initialization code on lines 220-222 (the addItemDecoration,
addOnScrollListener, and centerAndSnapRecyclerView calls) so they only execute
during the first call to setupCoverFlow. On subsequent calls, only update the
adapter with the new cover data. Additionally, instead of returning early when
covers.isEmpty() on line 189, clear the adapter or remove all items from the
RecyclerView to properly update the UI state, then return.
In `@app/src/main/java/com/cappielloantonio/tempo/util/UIUtil.java`:
- Around line 154-167: The scale calculation in the onScrolled method can
produce values below MIN for edge items, causing distorted card rendering. Add a
guard to check if rv.getWidth() is zero to prevent division by zero issues, then
clamp the computed scale variable to the valid range between MIN and MAX using
Math.max and Math.min functions before applying it to child views with
setScaleX, setScaleY, and setElevation.
In
`@app/src/main/java/com/cappielloantonio/tempo/viewmodel/CoverFlowViewModel.java`:
- Line 34: The ioExecutor field in CoverFlowViewModel is never shut down when
the ViewModel is destroyed, causing the background thread to leak. Override the
onCleared() lifecycle method in the CoverFlowViewModel class and call shutdown()
on the ioExecutor to properly terminate the thread when the ViewModel is
destroyed. Apply this same fix to any other Executor instances created in the
class (such as the one at line 61) to ensure all background threads are properly
cleaned up.
- Around line 39-55: The CoverFlowViewModel is hardcoding mock cover data
instead of using the injected repository dependency, which breaks the MVVM
pattern. Replace the entire mock data creation block (the Arrays.asList and for
loop that populates coversList with hardcoded image URLs) with a call to
repository.getCovers(). The repository call should be uncommented and its result
should be posted to coversLiveData instead of the mock list. This will ensure
the ViewModel properly delegates data fetching to the repository layer.
---
Nitpick comments:
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java`:
- Around line 198-208: The callback in PlayerControllerFragment is issuing a
Glide image load into the same ImageView that CoverFlowAdapter.bind() already
populates with cover.getUrl(). This creates redundant image loads and potential
flickering. Refactor the logic to use a single source-selection path: check if
coverArtId exists and use CustomGlideRequest if it does, otherwise rely on the
adapter's URL-based load. Consider consolidating this decision into the bind
method or the adapter itself rather than having competing load requests target
the same ImageView.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be8fb485-fb3d-4856-be02-cf3f729f129a
📒 Files selected for processing (10)
app/src/main/java/com/cappielloantonio/tempo/factory/CoverFlowViewModelFactory.javaapp/src/main/java/com/cappielloantonio/tempo/model/Cover.javaapp/src/main/java/com/cappielloantonio/tempo/repository/CoverRepository.javaapp/src/main/java/com/cappielloantonio/tempo/repository/MediaBrowserCoverRepository.javaapp/src/main/java/com/cappielloantonio/tempo/ui/adapter/CoverFlowAdapter.javaapp/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.javaapp/src/main/java/com/cappielloantonio/tempo/util/UIUtil.javaapp/src/main/java/com/cappielloantonio/tempo/viewmodel/CoverFlowViewModel.javaapp/src/main/res/layout/inner_fragment_player_controller_layout.xmlapp/src/main/res/layout/item_cover_flow.xml
Comment on lines
+26
to
+27
| MediaBrowser mediaBrowser = mediaBrowserFuture.get(); // blocks only inside a background thread | ||
| MediaMetadata metadata = mediaBrowser.getMediaMetadata(); |
Contributor
There was a problem hiding this comment.
Add a bounded wait when resolving mediaBrowserFuture.
Line 26 uses mediaBrowserFuture.get() with no timeout; if the future never resolves, cover loading can hang indefinitely and never post data.
Suggested fix
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
...
- MediaBrowser mediaBrowser = mediaBrowserFuture.get(); // blocks only inside a background thread
+ MediaBrowser mediaBrowser;
+ try {
+ mediaBrowser = mediaBrowserFuture.get(5, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ throw new IllegalStateException("Timed out waiting for MediaBrowser", e);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| MediaBrowser mediaBrowser = mediaBrowserFuture.get(); // blocks only inside a background thread | |
| MediaMetadata metadata = mediaBrowser.getMediaMetadata(); | |
| MediaBrowser mediaBrowser; | |
| try { | |
| mediaBrowser = mediaBrowserFuture.get(5, TimeUnit.SECONDS); | |
| } catch (TimeoutException e) { | |
| throw new IllegalStateException("Timed out waiting for MediaBrowser", e); | |
| } | |
| MediaMetadata metadata = mediaBrowser.getMediaMetadata(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/repository/MediaBrowserCoverRepository.java`
around lines 26 - 27, The mediaBrowserFuture.get() call in the media browser
cover loading code has no timeout specified, which can cause the application to
hang indefinitely if the future never completes. Replace the unbounded
mediaBrowserFuture.get() call with the overloaded version that accepts a timeout
duration and time unit parameters (e.g., get with a reasonable timeout like 5 or
10 seconds) to ensure that if the future doesn't resolve within that period, a
TimeoutException is thrown and the cover loading can fail gracefully instead of
hanging.
Comment on lines
+27
to
+51
| MediaMetadata metadata = mediaBrowser.getMediaMetadata(); | ||
|
|
||
| // Inject this here, somehow, since it grabs the covertArtId | ||
| /*` | ||
| CustomGlideRequest.Builder | ||
| .from(requireContext(), metadata.extras.getString("coverArtId"), CustomGlideRequest.ResourceType.Song) | ||
| .build() | ||
| .into(bind.playerHeaderLayout.playerHeaderMediaCoverImage); | ||
|
|
||
| */ | ||
| // ----------------------------------------------------------------- | ||
| // Replace the below with the real extraction logic from metadata. | ||
| // For demonstration we just return the three dog‑image URLs. | ||
| // ----------------------------------------------------------------- | ||
| List<String> urls = Arrays.asList( | ||
| "https://images.dog.ceo/breeds/affenpinscher/n02110627_11858.jpg", | ||
| "https://images.dog.ceo/breeds/hound-english/n02089973_811.jpg", | ||
| "https://images.dog.ceo/breeds/shiba/shiba-14.jpg" | ||
| ); | ||
|
|
||
| List<Cover> covers = new ArrayList<>(); | ||
| for (String url : urls) { | ||
| covers.add(new Cover(url, null)); // coverArtId can be filled later if needed | ||
| } | ||
| return covers; |
Contributor
There was a problem hiding this comment.
Repository currently bypasses the playback source, so Cover Flow cannot sync with queue state.
At Line 27 metadata is retrieved, but Lines 41-50 return a static dog URL list instead of mapping the actual playback/queue data. This breaks the intended data contract for end-to-end queue synchronization.
Direction for fix
- MediaMetadata metadata = mediaBrowser.getMediaMetadata();
- ...
- List<String> urls = Arrays.asList(...);
- List<Cover> covers = new ArrayList<>();
- for (String url : urls) {
- covers.add(new Cover(url, null));
- }
- return covers;
+ // Build covers from the active queue/media items so RecyclerView state
+ // and playback queue represent the same ordering/source of truth.
+ // (Use media item art URI and/or coverArtId extracted from metadata extras.)
+ return extractCoversFromQueue(mediaBrowser);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/repository/MediaBrowserCoverRepository.java`
around lines 27 - 51, The method retrieves metadata at the beginning but then
ignores it, instead returning a hardcoded list of dog image URLs in the covers
loop. Replace the static URL list creation (the Arrays.asList with three dog
image URLs) with logic that extracts the actual coverArtId from the metadata
object using metadata.extras.getString("coverArtId") and creates Cover objects
based on real playback data instead of placeholder URLs. This will ensure the
repository returns actual cover art data synchronized with the queue state.
| import com.cappielloantonio.tempo.R; | ||
| import com.cappielloantonio.tempo.model.Cover; | ||
|
|
||
| import org.jspecify.annotations.NonNull; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether JSpecify is explicitly declared and how broadly it's used.
set -euo pipefail
echo "== Build files mentioning jspecify =="
rg -n --iglob '*gradle*' 'jspecify|org\.jspecify'
echo
echo "== Java/Kotlin files importing org.jspecify.annotations.NonNull =="
rg -n 'import\s+org\.jspecify\.annotations\.NonNull'
echo
echo "== Java/Kotlin files importing androidx.annotation.NonNull =="
rg -n 'import\s+androidx\.annotation\.NonNull'Repository: eddyizm/tempus
Length of output: 96
🏁 Script executed:
# Check if the file exists and read its imports
fd -n 'CoverFlowAdapter.java' --exec head -80 {}
echo "=== Checking for build files ==="
fd -t f -e gradle -e xml | grep -E '(build|pom)' | head -20Repository: eddyizm/tempus
Length of output: 370
🏁 Script executed:
# More comprehensive search for jspecify in the entire codebase
echo "== All files with jspecify anywhere in content =="
rg 'jspecify' -i
echo
echo "== All imports of NonNull annotations =="
rg 'import.*NonNull' -A 1Repository: eddyizm/tempus
Length of output: 45111
🏁 Script executed:
# Check main build.gradle files for dependencies
echo "=== app/build.gradle ==="
cat app/build.gradle | grep -A 100 "dependencies"
echo
echo "=== Root build.gradle ==="
head -100 build.gradleRepository: eddyizm/tempus
Length of output: 3326
🏁 Script executed:
# Check if jspecify appears anywhere in gradle files (even if commented)
echo "=== Any mention of jspecify in all gradle files ==="
rg 'jspecify' -i app/build.gradle build.gradle gradle.properties settings.gradle 2>/dev/null || echo "No jspecify found"
# Also check if CoverFlowAdapter.java and UIUtil.java are the ONLY files with jspecify imports
echo
echo "=== All lines mentioning jspecify in source files ==="
rg 'org\.jspecify' --type javaRepository: eddyizm/tempus
Length of output: 405
Replace org.jspecify.annotations.NonNull with androidx.annotation.NonNull for consistency.
JSpecify is not declared as a dependency in the build configuration. This adapter and UIUtil.java are the only two files in the codebase using JSpecify annotations; all other files use AndroidX annotations. The inconsistency should be resolved by standardizing on androidx.annotation.NonNull to match the rest of the project and ensure the nullness checking toolchain is properly aligned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/adapter/CoverFlowAdapter.java`
at line 15, The import statement in CoverFlowAdapter.java is using
org.jspecify.annotations.NonNull, but JSpecify is not a declared dependency and
this creates inconsistency with the rest of the codebase which uses
androidx.annotation.NonNull. Replace the import statement at line 15 that
currently imports from org.jspecify.annotations with an import from
androidx.annotation.NonNull to align with the project's nullness checking
toolchain and match other files like UIUtil.java.
Comment on lines
+47
to
+52
| public void onBindViewHolder(@NonNull CoverViewHolder holder, int position) { | ||
| Cover cover = covers.get(position); | ||
| holder.bind(cover); | ||
| // Let the fragment (or caller) run the extra CustomGlideRequest if it needs the id. | ||
| boundListener.onCoverBound(cover, holder.coverImage); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid issuing two image loads for the same bind target.
onBindViewHolder() loads cover.getUrl() (via bind) and then triggers a second request through boundListener for items with coverArtId. This doubles work and can cause image flicker/race effects on recycled rows.
Suggested fix
- Cover cover = covers.get(position);
- holder.bind(cover);
- // Let the fragment (or caller) run the extra CustomGlideRequest if it needs the id.
- boundListener.onCoverBound(cover, holder.coverImage);
+ Cover cover = covers.get(position);
+ // Use one source-of-truth load path per item.
+ if (cover.getCoverArtId() != null) {
+ boundListener.onCoverBound(cover, holder.coverImage);
+ } else {
+ holder.bind(cover);
+ }Also applies to: 70-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/adapter/CoverFlowAdapter.java`
around lines 47 - 52, The onBindViewHolder method in CoverFlowAdapter is
triggering two separate image load requests for the same ImageView: one inside
holder.bind(cover) which loads cover.getUrl(), and another through
boundListener.onCoverBound(cover, holder.coverImage) for items with coverArtId.
To fix this, refactor the code to use a single image loading path. Either pass
the necessary coverArtId information to the bind method so it can make a single
optimized request, or conditionally call boundListener only when a custom image
request is specifically needed (avoiding the automatic load in bind). Apply this
same consolidation pattern to all occurrences of this binding logic including
the location around lines 70-75.
Comment on lines
+175
to
+179
| CoverRepository repository = new MediaBrowserCoverRepository(mediaBrowserListenableFuture); | ||
| CoverFlowViewModelFactory factory = new CoverFlowViewModelFactory(repository); | ||
|
|
||
| viewModel = new ViewModelProvider(this, factory) | ||
| .get(CoverFlowViewModel.class); |
Contributor
There was a problem hiding this comment.
Initialize mediaBrowserListenableFuture before building the repository.
At Line 175, MediaBrowserCoverRepository is created before initializeBrowser() runs (Line 320), so the repository can capture a null future. When repository-backed loading is enabled, this is a crash path.
Also applies to: 319-321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java`
around lines 175 - 179, The MediaBrowserCoverRepository is being instantiated
before mediaBrowserListenableFuture is initialized, which causes it to receive a
null value that will crash when used. Move the initialization of
mediaBrowserListenableFuture to occur before the MediaBrowserCoverRepository is
created in the constructor, or defer the CoverFlowViewModel creation (the
ViewModelProvider.get(CoverFlowViewModel.class) call) until after the
initializeBrowser() method has been called at line 320, ensuring the
mediaBrowserListenableFuture is properly initialized before being passed to the
MediaBrowserCoverRepository constructor.
Comment on lines
+188
to
+223
| private void setupCoverFlow(@NonNull List<Cover> covers) { | ||
| if (covers.isEmpty()) return; // optionally show an empty‑state view | ||
|
|
||
| // ----------------------------------------------------------------- | ||
| // 4️⃣ Create the adapter – the lambda receives the Cover object | ||
| // and the ImageView that just got bound. | ||
| // ----------------------------------------------------------------- | ||
| CoverFlowAdapter adapter = new CoverFlowAdapter( | ||
| requireContext(), | ||
| covers, | ||
| (cover, imageView) -> { | ||
| // Run the *extra* Glide request only when we have a valid id. | ||
| String coverArtId = cover.getCoverArtId(); | ||
| if (coverArtId != null) { | ||
| CustomGlideRequest.Builder | ||
| .from(requireContext(), | ||
| coverArtId, | ||
| CustomGlideRequest.ResourceType.Song) | ||
| .build() | ||
| .into(imageView); // load into the same thumbnail ImageView | ||
| } | ||
| }); | ||
|
|
||
| // ----------------------------------------------------------------- | ||
| // 5️⃣ RecyclerView basics (same as your original code) | ||
| // ----------------------------------------------------------------- | ||
| playerCoverFlow.setAdapter(adapter); | ||
| playerCoverFlow.setLayoutManager( | ||
| new LinearLayoutManager(requireContext(), | ||
| LinearLayoutManager.HORIZONTAL, | ||
| false)); | ||
|
|
||
| playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32)); | ||
| playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll()); | ||
| UIUtil.centerAndSnapRecyclerView(playerCoverFlow); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PlayerControllerFragment.java" -type fRepository: eddyizm/tempus
Length of output: 147
🏁 Script executed:
rg "setupCoverFlow" -B 5 -A 15Repository: eddyizm/tempus
Length of output: 3535
🏁 Script executed:
rg "playerCoverFlow" --max-count 20 -B 2 -A 2Repository: eddyizm/tempus
Length of output: 5242
🏁 Script executed:
cat -n app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java | head -250Repository: eddyizm/tempus
Length of output: 12624
🏁 Script executed:
rg "UIUtil\.(centerAndSnapRecyclerView|horizontalSpacing|scaleOnScroll)" -A 10Repository: eddyizm/tempus
Length of output: 1545
🏁 Script executed:
find . -name "UIUtil.java" -type fRepository: eddyizm/tempus
Length of output: 122
🏁 Script executed:
rg "class UIUtil|fun centerAndSnapRecyclerView|fun horizontalSpacing|fun scaleOnScroll" -A 20Repository: eddyizm/tempus
Length of output: 2162
🏁 Script executed:
rg "centerAndSnapRecyclerView|horizontalSpacing|scaleOnScroll" -B 2 -A 15 app/src/main/java/com/cappielloantonio/tempo/util/UIUtil.javaRepository: eddyizm/tempus
Length of output: 1603
🏁 Script executed:
rg "centerAndSnapRecyclerView" -A 40 app/src/main/java/com/cappielloantonio/tempo/util/UIUtil.javaRepository: eddyizm/tempus
Length of output: 1132
Make setupCoverFlow idempotent—gating decoration, listener, and snap helper setup to run only once.
Lines 220–222 execute on every LiveData emission, accumulating item decorations, scroll listeners, and reattaching the snap helper. Each call to addItemDecoration() and addOnScrollListener() stacks additional instances, and UIUtil.centerAndSnapRecyclerView() creates and reattaches a new PagerSnapHelper each time, which is wasteful and can lead to state conflicts on repeated data updates. Additionally, line 189 returns early on empty data without clearing the UI, leaving stale covers visible.
Guard the decoration, listener, and snap setup with a flag to run only on first initialization, while updating the adapter on each call:
+ private boolean coverFlowInitialized = false;
private void setupCoverFlow(`@NonNull` List<Cover> covers) {
- if (covers.isEmpty()) return; // optionally show an empty‑state view
+ if (covers.isEmpty()) {
+ playerCoverFlow.setAdapter(null);
+ playerCoverFlow.setVisibility(View.GONE);
+ return;
+ }
+ playerCoverFlow.setVisibility(View.VISIBLE);
CoverFlowAdapter adapter = new CoverFlowAdapter(
requireContext(),
covers,
(cover, imageView) -> {
String coverArtId = cover.getCoverArtId();
if (coverArtId != null) {
CustomGlideRequest.Builder
.from(requireContext(), coverArtId, CustomGlideRequest.ResourceType.Song)
.build()
.into(imageView);
}
});
playerCoverFlow.setAdapter(adapter);
- playerCoverFlow.setLayoutManager(new LinearLayoutManager(requireContext(), LinearLayoutManager.HORIZONTAL, false));
- playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32));
- playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll());
- UIUtil.centerAndSnapRecyclerView(playerCoverFlow);
+ if (!coverFlowInitialized) {
+ playerCoverFlow.setLayoutManager(new LinearLayoutManager(requireContext(), LinearLayoutManager.HORIZONTAL, false));
+ playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32));
+ playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll());
+ UIUtil.centerAndSnapRecyclerView(playerCoverFlow);
+ coverFlowInitialized = true;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void setupCoverFlow(@NonNull List<Cover> covers) { | |
| if (covers.isEmpty()) return; // optionally show an empty‑state view | |
| // ----------------------------------------------------------------- | |
| // 4️⃣ Create the adapter – the lambda receives the Cover object | |
| // and the ImageView that just got bound. | |
| // ----------------------------------------------------------------- | |
| CoverFlowAdapter adapter = new CoverFlowAdapter( | |
| requireContext(), | |
| covers, | |
| (cover, imageView) -> { | |
| // Run the *extra* Glide request only when we have a valid id. | |
| String coverArtId = cover.getCoverArtId(); | |
| if (coverArtId != null) { | |
| CustomGlideRequest.Builder | |
| .from(requireContext(), | |
| coverArtId, | |
| CustomGlideRequest.ResourceType.Song) | |
| .build() | |
| .into(imageView); // load into the same thumbnail ImageView | |
| } | |
| }); | |
| // ----------------------------------------------------------------- | |
| // 5️⃣ RecyclerView basics (same as your original code) | |
| // ----------------------------------------------------------------- | |
| playerCoverFlow.setAdapter(adapter); | |
| playerCoverFlow.setLayoutManager( | |
| new LinearLayoutManager(requireContext(), | |
| LinearLayoutManager.HORIZONTAL, | |
| false)); | |
| playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32)); | |
| playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll()); | |
| UIUtil.centerAndSnapRecyclerView(playerCoverFlow); | |
| } | |
| private boolean coverFlowInitialized = false; | |
| private void setupCoverFlow(`@NonNull` List<Cover> covers) { | |
| if (covers.isEmpty()) { | |
| playerCoverFlow.setAdapter(null); | |
| playerCoverFlow.setVisibility(View.GONE); | |
| return; | |
| } | |
| playerCoverFlow.setVisibility(View.VISIBLE); | |
| // ----------------------------------------------------------------- | |
| // 4️⃣ Create the adapter – the lambda receives the Cover object | |
| // and the ImageView that just got bound. | |
| // ----------------------------------------------------------------- | |
| CoverFlowAdapter adapter = new CoverFlowAdapter( | |
| requireContext(), | |
| covers, | |
| (cover, imageView) -> { | |
| // Run the *extra* Glide request only when we have a valid id. | |
| String coverArtId = cover.getCoverArtId(); | |
| if (coverArtId != null) { | |
| CustomGlideRequest.Builder | |
| .from(requireContext(), | |
| coverArtId, | |
| CustomGlideRequest.ResourceType.Song) | |
| .build() | |
| .into(imageView); // load into the same thumbnail ImageView | |
| } | |
| }); | |
| // ----------------------------------------------------------------- | |
| // 5️⃣ RecyclerView basics (same as your original code) | |
| // ----------------------------------------------------------------- | |
| playerCoverFlow.setAdapter(adapter); | |
| if (!coverFlowInitialized) { | |
| playerCoverFlow.setLayoutManager( | |
| new LinearLayoutManager(requireContext(), | |
| LinearLayoutManager.HORIZONTAL, | |
| false)); | |
| playerCoverFlow.addItemDecoration(UIUtil.horizontalSpacing(32)); | |
| playerCoverFlow.addOnScrollListener(UIUtil.scaleOnScroll()); | |
| UIUtil.centerAndSnapRecyclerView(playerCoverFlow); | |
| coverFlowInitialized = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/ui/fragment/PlayerControllerFragment.java`
around lines 188 - 223, The setupCoverFlow method is being called repeatedly
(likely from a LiveData observer) and accumulating item decorations, scroll
listeners, and recreating the snap helper on each call, causing state conflicts
and wasted resources. Introduce a boolean flag to guard the one-time
initialization code on lines 220-222 (the addItemDecoration,
addOnScrollListener, and centerAndSnapRecyclerView calls) so they only execute
during the first call to setupCoverFlow. On subsequent calls, only update the
adapter with the new cover data. Additionally, instead of returning early when
covers.isEmpty() on line 189, clear the adapter or remove all items from the
RecyclerView to properly update the UI state, then return.
Comment on lines
+154
to
+167
| public void onScrolled(@NonNull RecyclerView rv, int dx, int dy) { | ||
| final float centerX = rv.getWidth() / 2f; | ||
|
|
||
| for (int i = 0; i < rv.getChildCount(); i++) { | ||
| View child = rv.getChildAt(i); | ||
|
|
||
| float childCenter = (child.getLeft() + child.getRight()) / 2f; | ||
| float distance = Math.abs(centerX - childCenter); | ||
| float scale = MAX - (distance / centerX) * (MAX - MIN); | ||
|
|
||
| child.setScaleX(scale); | ||
| child.setScaleY(scale); | ||
| child.setElevation(scale * ELEVATION_FACTOR); | ||
| } |
Contributor
There was a problem hiding this comment.
Clamp computed scale to a safe range.
At Line 162, scale can go below MIN for edge items; this can produce distorted card rendering. Clamp to [MIN, MAX] and guard zero-width cases.
Suggested patch
`@Override`
public void onScrolled(`@NonNull` RecyclerView rv, int dx, int dy) {
final float centerX = rv.getWidth() / 2f;
+ if (centerX <= 0f) return;
for (int i = 0; i < rv.getChildCount(); i++) {
View child = rv.getChildAt(i);
float childCenter = (child.getLeft() + child.getRight()) / 2f;
float distance = Math.abs(centerX - childCenter);
- float scale = MAX - (distance / centerX) * (MAX - MIN);
+ float rawScale = MAX - (distance / centerX) * (MAX - MIN);
+ float scale = Math.max(MIN, Math.min(MAX, rawScale));
child.setScaleX(scale);
child.setScaleY(scale);
child.setElevation(scale * ELEVATION_FACTOR);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/cappielloantonio/tempo/util/UIUtil.java` around lines
154 - 167, The scale calculation in the onScrolled method can produce values
below MIN for edge items, causing distorted card rendering. Add a guard to check
if rv.getWidth() is zero to prevent division by zero issues, then clamp the
computed scale variable to the valid range between MIN and MAX using Math.max
and Math.min functions before applying it to child views with setScaleX,
setScaleY, and setElevation.
| return coversLiveData; | ||
| } | ||
|
|
||
| private final Executor ioExecutor = Executors.newSingleThreadExecutor(); |
Contributor
There was a problem hiding this comment.
Shut down the executor in onCleared() to avoid leaking a background thread.
The single-thread executor created at Line 34 is never terminated. When the ViewModel is destroyed, that thread can outlive the lifecycle unnecessarily.
Suggested fix
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
...
- private final Executor ioExecutor = Executors.newSingleThreadExecutor();
+ private final ExecutorService ioExecutor = Executors.newSingleThreadExecutor();
...
private void loadCovers() {
ioExecutor.execute(() -> {
try {
...
} catch (Exception e) {
coversLiveData.postValue(Collections.emptyList());
}
});
}
+
+ `@Override`
+ protected void onCleared() {
+ ioExecutor.shutdownNow();
+ super.onCleared();
+ }
}Also applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/viewmodel/CoverFlowViewModel.java`
at line 34, The ioExecutor field in CoverFlowViewModel is never shut down when
the ViewModel is destroyed, causing the background thread to leak. Override the
onCleared() lifecycle method in the CoverFlowViewModel class and call shutdown()
on the ioExecutor to properly terminate the thread when the ViewModel is
destroyed. Apply this same fix to any other Executor instances created in the
class (such as the one at line 61) to ensure all background threads are properly
cleaned up.
Comment on lines
+39
to
+55
| // Disabled, correct implementation not done just yet | ||
| // List<Cover> list = repository.getCovers(); // Disabled as it ain't implemented | ||
|
|
||
| /* Mock of data fecthing */ | ||
| List<String> urls = Arrays.asList( | ||
| "https://images.dog.ceo/breeds/affenpinscher/n02110627_11858.jpg", | ||
| "https://images.dog.ceo/breeds/hound-english/n02089973_811.jpg", | ||
| "https://images.dog.ceo/breeds/shiba/shiba-14.jpg" | ||
| ); | ||
| List<Cover> coversList = new ArrayList<>(); | ||
| for (String url : urls) { | ||
| coversList.add(new Cover(url, null)); // coverArtId can be filled later if needed | ||
| } | ||
|
|
||
| // Normal logic end | ||
| coversLiveData.postValue(coversList); | ||
| } catch (Exception e) { |
Contributor
There was a problem hiding this comment.
CoverFlowViewModel is not using its repository dependency, so MVVM wiring is effectively bypassed.
Lines 39-51 hardcode mock data in the ViewModel instead of calling the injected repository. This prevents the Fragment → ViewModel → Repository path from reflecting playback queue state.
Suggested fix
ioExecutor.execute(() -> {
try {
- // Disabled, correct implementation not done just yet
- // List<Cover> list = repository.getCovers(); // Disabled as it ain't implemented
-
- /* Mock of data fecthing */
- List<String> urls = Arrays.asList(
- "https://images.dog.ceo/breeds/affenpinscher/n02110627_11858.jpg",
- "https://images.dog.ceo/breeds/hound-english/n02089973_811.jpg",
- "https://images.dog.ceo/breeds/shiba/shiba-14.jpg"
- );
- List<Cover> coversList = new ArrayList<>();
- for (String url : urls) {
- coversList.add(new Cover(url, null)); // coverArtId can be filled later if needed
- }
-
- // Normal logic end
- coversLiveData.postValue(coversList);
+ List<Cover> coversList = repository.getCovers();
+ coversLiveData.postValue(coversList);
} catch (Exception e) {
coversLiveData.postValue(Collections.emptyList());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/cappielloantonio/tempo/viewmodel/CoverFlowViewModel.java`
around lines 39 - 55, The CoverFlowViewModel is hardcoding mock cover data
instead of using the injected repository dependency, which breaks the MVVM
pattern. Replace the entire mock data creation block (the Arrays.asList and for
loop that populates coversList with hardcoded image URLs) with a call to
repository.getCovers(). The repository call should be uncommented and its result
should be posted to coversLiveData instead of the mock list. This will ensure
the ViewModel properly delegates data fetching to the repository layer.
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
2 participants
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.
Addresses #454.
This is a UI implementation of Cover Flow, it hardcodes external URLs to showcase how it works.
UI showcase
recording_20260510_141954.mp4
The lyrics button acts as a swapper between the old ViewPager2 (page 0 is album cover, page 1 is lyrics view) and the new RecyclerView (dog pictures). What's particular about the later, aside from the nice visual re-dimension effect, is that it snaps in place on each horizontal scroll rather than drifting through the entire list at once.
What's left to do to complete the feature is to sync the RecyclerView elements with the current queue. Both lists must go synced and on each swap the currently playing song must change to next/prev and move forward/back the queue.
Summary by CodeRabbit
New Features
Style