Corrected volume max setting not loading properly #8
Conversation
|
Tested this PR: Stopping the service and starting the app again with "Allow lower than limit" enabled now stays functional as expected. I did happen to notice if you set the volume lower than the limit while stopping the service and restarting the app, the upper limit gets changed to that lower volume, meaning you'd have to go into the app and turn it back up if wanted. This could be considered a feature rather than a bug though depending who you ask since it could help prevent getting hit with overly-loud volume in some cases. |
WalkthroughThe changes in this pull request involve multiple updates across various files in the Android application. Key modifications include updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
app/src/main/java/com/klee/volumelockr/ui/VolumeLockrApplication.kt (1)
7-10: Consider adding version check and error handling.While the implementation is functional, it could be more robust:
- DynamicColors might not be available on all Android versions
- The application should gracefully handle cases where dynamic colors aren't supported
Consider this more defensive implementation:
override fun onCreate() { super.onCreate() - DynamicColors.applyToActivitiesIfAvailable(this) + try { + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.S) { + DynamicColors.applyToActivitiesIfAvailable(this) + } + } catch (e: Exception) { + android.util.Log.w("VolumeLockr", "Failed to apply dynamic colors", e) + } }app/src/main/res/values/themes.xml (1)
3-4: LGTM! Good choice using Material3 with dynamic colors.The migration to Material3 with dynamic colors support is a good modernization step. This will provide better theming capabilities and ensure proper dark mode support.
Consider documenting the theme capabilities in the README.md to help future contributors understand the theming system, especially regarding dynamic colors support.
build.gradle (2)
Line range hint
23-23: Remove deprecated jcenter() repository.The jcenter() repository is deprecated and will be shut down. Consider removing it and ensuring all dependencies are available through mavenCentral() or google().
Apply this diff:
allprojects { repositories { google() mavenCentral() - jcenter() // Warning: this repository is going to shut down soon } }
Line range hint
1-31: Consider implementing version catalogs for dependency management.The current build file uses hardcoded versions. Consider implementing Gradle version catalogs to centralize dependency management and make version updates more maintainable.
Create a new file
gradle/libs.versions.toml:[versions] kotlin = "1.8.0" agp = "8.5.0" aboutlibraries = "8.9.0" [libraries] kotlin-gradle = { module = "org.jetbrains.kotlin:kotlin-gradle-plugin", version.ref = "kotlin" } android-gradle = { module = "com.android.tools.build:gradle", version.ref = "agp" } aboutlibraries-gradle = { module = "com.mikepenz.aboutlibraries.plugin:aboutlibraries-plugin", version.ref = "aboutlibraries" }Then update the build.gradle:
buildscript { - ext.kotlin_version = '1.8.0' repositories { google() mavenCentral() maven { url = uri("https://plugins.gradle.org/m2/") } } dependencies { - classpath 'com.android.tools.build:gradle:8.5.0' - classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" - classpath "com.mikepenz.aboutlibraries.plugin:aboutlibraries-plugin:8.9.0" + classpath(libs.android.gradle) + classpath(libs.kotlin.gradle) + classpath(libs.aboutlibraries.gradle) } }app/src/main/res/layout/volume_card.xml (1)
39-44: Add contentDescription for accessibility support.The MaterialSwitch should have a content description to improve accessibility for users with screen readers.
Apply this diff:
<com.google.android.material.materialswitch.MaterialSwitch android:id="@+id/switchButton" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_gravity="center" - android:text="@string/lock" /> + android:text="@string/lock" + android:contentDescription="@string/lock" />app/src/main/java/com/klee/volumelockr/ui/VolumeAdapter.kt (1)
122-124: Fix indentation for better readability.The conditional expression could be formatted more clearly.
- holder.binding.slider.isEnabled = - mService?.getMode() == 2 && - mService?.getLocks()?.containsKey(AudioManager.STREAM_NOTIFICATION) == false + holder.binding.slider.isEnabled = mService?.getMode() == 2 && + !mService?.getLocks()?.containsKey(AudioManager.STREAM_NOTIFICATION)!!app/src/main/java/com/klee/volumelockr/service/VolumeService.kt (3)
179-182: LGTM! Consider adding documentation.The new method effectively centralizes the loading of general preferences. Consider adding KDoc documentation to explain its purpose and the impact of the
ALLOW_LOWERsetting.+ /** + * Loads general preferences including the "Allow lower than limit" setting + * which determines if volumes can be set below their locked values. + */ private fun loadGeneralPreferences() {
Line range hint
186-195: Consider additional synchronization for preference access.While
checkVolumes()is synchronized, it readsmAllowLowerwhich can be modified byloadGeneralPreferences()without synchronization. This could potentially lead to a race condition.Consider these options:
- Add synchronization to preference access:
@WorkerThread @Synchronized private fun checkVolumes() { + val allowLower = synchronized(this) { mAllowLower } for ((stream, volume) in mVolumeLock) { val current = mAudioManager.getStreamVolume(stream) - if ((current > volume) || (!mAllowLower && current != volume)) { + if ((current > volume) || (!allowLower && current != volume)) { mAudioManager.setStreamVolume(stream, volume, 0) invokeVolumeListenerCallback() } } }
- Or make mAllowLower volatile:
- private var mAllowLower = false + @Volatile + private var mAllowLower = false
Line range hint
186-195: Document the volume adjustment behavior.The current implementation matches BurnyBoi's observation: when the service restarts with a volume lower than the limit, it maintains that lower volume. While this behavior is reasonable from a user experience perspective (preventing unexpected volume increases), it should be documented.
Consider adding a comment explaining this behavior:
@WorkerThread @Synchronized private fun checkVolumes() { + // Note: When "Allow lower than limit" is enabled and the service restarts, + // if the current volume is below the limit, it will be maintained at that level. + // This prevents unexpected volume increases on service restart. for ((stream, volume) in mVolumeLock) {
🛑 Comments failed to post (2)
app/src/main/res/layout/volume_card.xml (1)
33-37:
⚠️ Potential issueAdd essential Slider attributes for volume control.
The Slider component is missing crucial attributes needed for proper volume control functionality:
Apply this diff to add the necessary attributes:
<com.google.android.material.slider.Slider android:id="@+id/slider" android:layout_width="match_parent" android:layout_height="wrap_content" - android:layout_gravity="center"/> + android:layout_gravity="center" + android:valueFrom="0" + android:valueTo="100" + android:stepSize="1" + android:value="0"/>📝 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.<com.google.android.material.slider.Slider android:id="@+id/slider" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_gravity="center" android:valueFrom="0" android:valueTo="100" android:stepSize="1" android:value="0"/>app/src/main/java/com/klee/volumelockr/ui/VolumeAdapter.kt (1)
65-73:
⚠️ Potential issueAdd validation for volume limit changes.
The current implementation allows immediate value changes without checking against previous maximum limits. This explains the reported behavior where the maximum volume limit adjusts to lower settings after restart.
Consider adding validation to:
- Check if the new value is below the previous maximum when "Allow lower than limit" is enabled
- Store the original maximum value separately to allow restoration
private fun registerSeekBarCallback(holder: ViewHolder, volume: Volume) { val listener = Slider.OnChangeListener { _, value, _ -> + val previousMax = mService?.getStoredMaxVolume(volume.stream) + val allowLower = mService?.isAllowLowerThanLimitEnabled() ?: false + + val newValue = if (!allowLower && previousMax != null) { + kotlin.math.min(value, previousMax.toFloat()) + } else { + value + } + if (volume.stream != AudioManager.STREAM_NOTIFICATION || mService?.getMode() == 2) { - mAudioManager.setStreamVolume(volume.stream, value.toInt(), 0) + mAudioManager.setStreamVolume(volume.stream, newValue.toInt(), 0) } - volume.value = value.toInt() + volume.value = newValue.toInt() } holder.binding.slider.addOnChangeListener(listener) }Committable suggestion skipped: line range outside the PR's diff.
Per discussion: #3 (comment)
Summary by CodeRabbit
Release Notes
New Features
Improvements
SeekBarwithSliderfor a more modern user experience.Theme.VolumeLockrfor better visual consistency.Bug Fixes
Chores