Add volume presets feature#25
Open
Wanyal wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded configurable per-stream volume presets, preference-mode slider editing, preset-aware locking, settings UI, navigation arguments, and Safe Args Gradle configuration. ChangesPreset volume locking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsFragment
participant ChangePresetDialog
participant VolumeSliderFragment
participant VolumeService
SettingsFragment->>ChangePresetDialog: launch preset editor
ChangePresetDialog->>VolumeSliderFragment: embed preference-mode slider
VolumeSliderFragment->>VolumeService: read volume presets
VolumeSliderFragment-->>ChangePresetDialog: return edited presets
ChangePresetDialog-->>SettingsFragment: return fragment result
SettingsFragment->>SettingsFragment: persist clamped presets
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/com/klee/volumelockr/ui/VolumeAdapter.kt (1)
119-145: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear slider touch listeners before re-registering callbacks
registerSeekBarCallback()adds a newOnSliderTouchListeneron every bind, so recycled holders can accumulate duplicateonStopTrackingTouch()calls. AddclearOnSliderTouchListeners()next toclearOnChangeListeners().🤖 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/klee/volumelockr/ui/VolumeAdapter.kt` around lines 119 - 145, Clear existing slider touch listeners at the start of registerSeekBarCallback, alongside clearOnChangeListeners(), before adding the new OnSliderTouchListener to prevent duplicate callbacks on recycled holders.
🧹 Nitpick comments (3)
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt (1)
180-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated preset-resolution logic.
getPresetVolumeForLockedStreammirrorsVolumeAdapter.getVolumePresetLevelalmost verbatim (same preference keys, same per-stream clamp), differing only in the min/max source. Consider extracting a single shared helper to keep the two paths from diverging.🤖 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/klee/volumelockr/service/VolumeService.kt` around lines 180 - 219, Extract the duplicated per-stream preset lookup and clamping logic from getPresetVolumeForLockedStream and VolumeAdapter.getVolumePresetLevel into a shared helper. The helper should accept the stream and resolve the corresponding preference key, minimum volume, and maximum-volume provider, while preserving each method’s current behavior; update both callers to use it and retain their distinct min/max sources where required.build.gradle (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVersion-catalog consistency (optional).
The other buildscript classpaths use the
libs.*version catalog while this one hardcodes2.8.9. Consider movingnavigation-safe-args-gradle-plugininto the catalog to keep the Navigation version aligned with the runtimeandroidx.navigationdependencies and avoid drift.🤖 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 `@build.gradle` at line 15, Move the hardcoded navigation-safe-args-gradle-plugin dependency from the buildscript classpath into the project’s version catalog, defining its version alongside the existing AndroidX Navigation entries, then update the buildscript declaration to reference the generated libs.* alias and keep it aligned with runtime Navigation dependencies.app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (1)
173-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCombine the four
edit {}blocks into a single commit.Each stream branch opens a separate
SharedPreferences.Editorvia?.edit { }, resulting in four independent asyncapply()calls. A singleedit {}block wrapping theforEachwould be atomic and more efficient.♻️ Proposed refactor
volumes.forEach { when (it.stream) { AudioManager.STREAM_MUSIC -> - preferenceManager.sharedPreferences?.edit { - putInt( - MEDIA_VOLUME_PRESET_PREFERENCE, - it.value.coerceIn(it.min, it.max) - ) - } + putInt( + MEDIA_VOLUME_PRESET_PREFERENCE, + it.value.coerceIn(it.min, it.max) + ) AudioManager.STREAM_VOICE_CALL -> - preferenceManager.sharedPreferences?.edit { - putInt(CALL_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) - } + putInt(CALL_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) AudioManager.STREAM_NOTIFICATION -> - preferenceManager.sharedPreferences?.edit { - putInt( - NOTIFICATION_VOLUME_PRESET_PREFERENCE, - it.value.coerceIn(it.min, it.max) - ) - } + putInt( + NOTIFICATION_VOLUME_PRESET_PREFERENCE, + it.value.coerceIn(it.min, it.max) + ) AudioManager.STREAM_ALARM -> - preferenceManager.sharedPreferences?.edit { - putInt( - ALARM_VOLUME_PRESET_PREFERENCE, - it.value.coerceIn(it.min, it.max) - ) - } + putInt( + ALARM_VOLUME_PRESET_PREFERENCE, + it.value.coerceIn(it.min, it.max) + ) } } + }Full replacement for clarity:
preferenceManager.sharedPreferences?.edit { volumes.forEach { when (it.stream) { AudioManager.STREAM_MUSIC -> putInt(MEDIA_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) AudioManager.STREAM_VOICE_CALL -> putInt(CALL_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) AudioManager.STREAM_NOTIFICATION -> putInt(NOTIFICATION_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) AudioManager.STREAM_ALARM -> putInt(ALARM_VOLUME_PRESET_PREFERENCE, it.value.coerceIn(it.min, it.max)) } } }🤖 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/klee/volumelockr/ui/SettingsFragment.kt` around lines 173 - 204, Refactor the volume persistence logic in the volumes.forEach block to open one sharedPreferences?.edit { } transaction around the entire iteration. Keep the existing stream-to-preference mappings and value coercion, but call putInt directly within each when branch so all updates use a single atomic commit.
🤖 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/klee/volumelockr/service/VolumeService.kt`:
- Around line 113-119: Update getVolumesPresets() to initialize missing
mVolumePreset entries from persisted preferences, using
getPresetVolumeForLockedStream(volume.stream) as the fallback instead of 0,
while preserving any values already present in mVolumePreset.
In `@app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt`:
- Around line 310-315: Make the Volume model implement Parcelable (for example,
via `@Parcelize` and the required plugin/dependency), then keep the "volumes"
fragment-result payload in SettingsFragment’s positive-button listener as an
ArrayList<Volume> so BundleCompat.getParcelableArrayList can round-trip it
correctly.
In `@app/src/main/java/com/klee/volumelockr/ui/VolumeAdapter.kt`:
- Line 96: Update the call to handleRingerMode in VolumeAdapter so it only runs
when !mInPreferencesMode, preventing the notification slider from being disabled
while editing preferences.
---
Outside diff comments:
In `@app/src/main/java/com/klee/volumelockr/ui/VolumeAdapter.kt`:
- Around line 119-145: Clear existing slider touch listeners at the start of
registerSeekBarCallback, alongside clearOnChangeListeners(), before adding the
new OnSliderTouchListener to prevent duplicate callbacks on recycled holders.
---
Nitpick comments:
In `@app/src/main/java/com/klee/volumelockr/service/VolumeService.kt`:
- Around line 180-219: Extract the duplicated per-stream preset lookup and
clamping logic from getPresetVolumeForLockedStream and
VolumeAdapter.getVolumePresetLevel into a shared helper. The helper should
accept the stream and resolve the corresponding preference key, minimum volume,
and maximum-volume provider, while preserving each method’s current behavior;
update both callers to use it and retain their distinct min/max sources where
required.
In `@app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt`:
- Around line 173-204: Refactor the volume persistence logic in the
volumes.forEach block to open one sharedPreferences?.edit { } transaction around
the entire iteration. Keep the existing stream-to-preference mappings and value
coercion, but call putInt directly within each when branch so all updates use a
single atomic commit.
In `@build.gradle`:
- Line 15: Move the hardcoded navigation-safe-args-gradle-plugin dependency from
the buildscript classpath into the project’s version catalog, defining its
version alongside the existing AndroidX Navigation entries, then update the
buildscript declaration to reference the generated libs.* alias and keep it
aligned with runtime Navigation dependencies.
🪄 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: 72258480-eb47-4bb7-a1f9-39be252b24c2
📒 Files selected for processing (12)
app/build.gradleapp/src/main/java/com/klee/volumelockr/service/VolumeProvider.ktapp/src/main/java/com/klee/volumelockr/service/VolumeService.ktapp/src/main/java/com/klee/volumelockr/ui/SettingsFragment.ktapp/src/main/java/com/klee/volumelockr/ui/VolumeAdapter.ktapp/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.ktapp/src/main/res/layout/dialog_presets.xmlapp/src/main/res/navigation/navigation_graph.xmlapp/src/main/res/values-ar/strings.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/root_preferences.xmlbuild.gradle
💤 Files with no reviewable changes (1)
- app/src/main/res/values-ar/strings.xml
…get level when presert preference screen closes
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.
Added a feature that allows the user to set pre-determined volumes for when they apply a lock. The user can choose if they want to apply the presets when they lock a single volume, lock all volumes, or both. This is my first contribution to an Android app, so feedback is welcome.
The new dialog in preferences to set the presets uses the existing VolumeFragment to lower potential burden of adding new sliders in the future.
Summary by CodeRabbit