feat(): achievement sound on unlock#1671
Open
phobos665 wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAchievementWatcher now plays an unlock sound from a raw resource when new achievements are detected, gated by a new preference. The settings UI adds a toggle for that preference, and localized strings were added for the new label. XServerScreen now passes context into the watcher constructor. ChangesAchievement sound playback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsGroupInterface
participant PrefManager
participant AchievementWatcher
participant MediaPlayer
SettingsGroupInterface->>PrefManager: achievementPlaySound = toggle state
AchievementWatcher->>PrefManager: read achievementPlaySound
AchievementWatcher->>MediaPlayer: MediaPlayer.create(context, R.raw.achievement_pop)
AchievementWatcher->>AchievementWatcher: checkForNewUnlocks() finds new unlock
AchievementWatcher->>MediaPlayer: start()/seekTo(0) when enabled
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/AchievementWatcher.kt (1)
20-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid storing the screen
ContextinAchievementWatcher.
PluviaApp.achievementWatcheris a companion-object reference, andXServerScreenpassesLocalContext.currentintosetupXEnvironmentand then intoAchievementWatcher. Keeping thatContextas a field can retain the Activity for the whole session; usecontext.applicationContextforMediaPlayer.create(...)and drop the stored property.🤖 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/app/gamenative/service/AchievementWatcher.kt` around lines 20 - 35, Avoid keeping the passed screen Context as a field in AchievementWatcher, since PluviaApp.achievementWatcher can outlive the Activity and leak it. In AchievementWatcher, use context.applicationContext directly where needed, especially for MediaPlayer.create(...), and remove the stored Context property from the constructor/state so the class only holds the application-scoped context behavior.
🤖 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/app/gamenative/service/AchievementWatcher.kt`:
- Line 34: The MediaPlayer instance in AchievementWatcher is accessed from
multiple threads, so its lifecycle and playback calls need serialization. Update
achievementSoundPlayer usage so create/playback/stop/release all happen on one
thread or are protected by synchronization, including the FileObserver.onEvent
path and the stop() cleanup. Make sure the isPlaying, seekTo, start(), and
release() operations cannot race with each other by guarding the shared
MediaPlayer state consistently.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/AchievementWatcher.kt`:
- Around line 20-35: Avoid keeping the passed screen Context as a field in
AchievementWatcher, since PluviaApp.achievementWatcher can outlive the Activity
and leak it. In AchievementWatcher, use context.applicationContext directly
where needed, especially for MediaPlayer.create(...), and remove the stored
Context property from the constructor/state so the class only holds the
application-scoped context behavior.
🪄 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
Run ID: 2e3d11e5-9fb9-42b5-b2ee-4413a0ea6abf
⛔ Files ignored due to path filters (1)
app/src/main/res/raw/achievement_pop.mp3is excluded by!**/*.mp3
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/AchievementWatcher.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
1 participant
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.
Description
Highly requested feature:
Adds an achievement unlock sound on unlock of achievement.
Adds the media player and the play of the tune within the AchievementWatcher which is the best place for this. Pretty simple change and we already have the required permissions in Android to do this.
Only question is whether we'd like to change the sound to something else.
Recording
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Adds a short sound when an achievement unlocks and a settings toggle to control it. Uses a single, thread-safe
MediaPlayerinAchievementWatcherwithachievement_pop.mp3.PrefManager.achievementPlaySound, with translations.MediaPlayerinAchievementWatcher(synchronized, restarts if already playing, released on stop);XServerScreenpassescontextto initialize it.Written for commit fdd2c9b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Localization / Documentation