feat: Foldable support, Edge-to-Edge, and Fixes #13
Conversation
The `PolicyAccessDialog` `DialogFragment` has been moved from `MainActivity.kt` into its own dedicated file, `PolicyAccessDialog.kt`, to improve code organization. In `MainActivity.kt`, `enableEdgeToEdge()` is now used, and the insets handling method has been renamed from `handleEdgeToEdgeInsets` to `setupWindowInsets` for clarity.
Adds a `BottomNavigationView` to the main screen for easier navigation between the main volume sliders, settings, and the about page. This includes: - Adding a `MaterialToolbar` and `BottomNavigationView` to `activity_main.xml`. - Creating a new `bottom_nav_menu.xml` with icons for home, settings, and info. - Updating `MainActivity.kt` to set up the toolbar and bottom navigation with the navigation controller. - Removing the now-redundant options menu from `VolumeSliderFragment`. - Applying a `NoActionBar` theme to the activity.
- Introduces a two-pane layout for screens with a width of 600dp or more, using `NavigationRailView`. - The volume sliders are now displayed in a grid with two columns on larger screens. - Updates string resources for clarity and consistency.
- Add a new "About" screen to display open-source library information. - Use a `GridLayoutManager` on the about screen for larger displays. - Replace `AlertDialog` with `MaterialAlertDialogBuilder` for a consistent look. - Improve dialog layout by adding margins. - Center the bottom navigation menu on wider screens.
Inflate password dialog from XML layout instead of creating it programmatically. This also adds a lock icon and uses a centered Material 3 theme for the dialog.
WalkthroughAdds navigation (toolbar + bottom navigation / navigation rail) and edge-to-edge insets; AboutFragment and PolicyAccessDialog; encrypted password dialog flow using EncryptedSharedPreferences; grid layouts for wide screens; conditional service start/stop and notification behavior; new drawables, layouts, resources, and .gitignore entries. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant MA as MainActivity
participant NH as NavHostFragment
participant NC as NavController
participant NAV as BottomNav / NavRail
participant FR as Fragment
participant SVC as VolumeService
participant BR as BootReceiver
User->>MA: launch app
MA->>MA: enableEdgeToEdge()
MA->>MA: setupNavigation()
MA->>MA: setupWindowInsets()
MA->>NH: locate NavHostFragment
NH->>NC: provide NavController
MA->>MA: configure Toolbar with NC
MA->>NAV: setupWithNavController(NC)
User->>NAV: select menu item
NAV->>NC: navigate(dest)
NC->>FR: attach destination fragment (e.g., About, Volume, Settings)
FR->>FR: initialize UI (libs injection / grid layout / dialogs)
Note over BR,SVC: Boot-time / service lifecycle
BR->>SVC: start only if locks exist
SVC->>SVC: create notification channel
alt no locks present
SVC->>SVC: stopForeground(STOP_FOREGROUND_REMOVE) and stopSelf()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
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/service/VolumeService.kt (1)
284-290: Critical: Incorrect argument passed to stopForeground().Line 289 passes
NOTIFICATION_ID(4455) tostopForeground(), but this method expects flags, not a notification ID. Valid flags areService.STOP_FOREGROUND_REMOVE(1) orService.STOP_FOREGROUND_DETACH(2).This is existing code (not introduced in this PR), but it's a bug that should be fixed.
Apply this diff to fix the issue:
@SuppressLint("WrongConstant") @RequiresApi(Build.VERSION_CODES.N) @Synchronized fun tryHideNotification() { if (mVolumeLock.isNotEmpty()) { return } - stopForeground(NOTIFICATION_ID) + stopForeground(Service.STOP_FOREGROUND_REMOVE) }Also, remove the
@SuppressLint("WrongConstant")annotation as it's masking this bug.
🧹 Nitpick comments (4)
app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt (1)
58-59: Consider using a boolean resource for the responsive breakpoint.The hardcoded
600dpthreshold works but could be extracted to a boolean resource for better maintainability and consistency across the app. This pattern is repeated inAboutFragment.kt.Create
res/values/bools.xml:<resources> <bool name="use_two_columns">false</bool> </resources>Create
res/values-w600dp/bools.xml:<resources> <bool name="use_two_columns">true</bool> </resources>Then update the code:
-val spanCount = if (resources.configuration.screenWidthDp >= 600) 2 else 1 +val spanCount = if (resources.getBoolean(R.bool.use_two_columns)) 2 else 1app/src/main/java/com/klee/volumelockr/ui/AboutFragment.kt (2)
26-33: Consider usingcommit()instead ofcommitNow().Using
commitNow()inonViewCreatedforces synchronous execution and can throwIllegalStateExceptionin certain fragment lifecycle scenarios. The standardcommit()is safer and allows the transaction to execute asynchronously.Apply this diff:
childFragmentManager.beginTransaction() .add(R.id.about_libs_container, libsFragment) - .commitNow() + .commit()
35-43: Document the fragile view hierarchy dependency.The
view.post+findRecyclerViewapproach tightly couples this code to the internal structure of theLibsFragmentfrom the AboutLibraries library. If the library updates and changes its view hierarchy, this will silently fail.Consider one of these approaches:
- Document this dependency with a comment explaining the workaround and why it's necessary
- Check if the AboutLibraries library provides a public API to access or configure its RecyclerView
- Add a runtime warning if the RecyclerView isn't found:
view.post { val recyclerView = findRecyclerView(view) + if (recyclerView == null) { + // Log warning: AboutLibraries RecyclerView not found - layout may have changed + } recyclerView?.let { rv ->app/src/main/java/com/klee/volumelockr/ui/MainActivity.kt (1)
73-94: Consider extracting common inset logic.The bottom navigation and navigation rail inset listeners share nearly identical logic, differing only in which edges receive padding. Extracting this to a helper function would improve maintainability.
private fun setupWindowInsets() { applyWindowInsets(binding.appBarLayout, left = true, top = true, right = true, bottom = false) binding.bottomNavigation?.let { applyWindowInsets(it, left = true, top = false, right = true, bottom = true) } binding.navigationRail?.let { applyWindowInsets(it, left = true, top = true, right = false, bottom = true) } } private fun applyWindowInsets(view: View, left: Boolean, top: Boolean, right: Boolean, bottom: Boolean) { ViewCompat.setOnApplyWindowInsetsListener(view) { v, windowInsets -> val bars = windowInsets.getInsets( WindowInsetsCompat.Type.systemBars() or WindowInsetsCompat.Type.displayCutout() ) v.setPadding( if (left) bars.left else 0, if (top) bars.top else 0, if (right) bars.right else 0, if (bottom) bars.bottom else 0 ) WindowInsetsCompat.CONSUMED } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitignore(1 hunks)app/src/main/java/com/klee/volumelockr/service/VolumeService.kt(1 hunks)app/src/main/java/com/klee/volumelockr/ui/AboutFragment.kt(1 hunks)app/src/main/java/com/klee/volumelockr/ui/MainActivity.kt(4 hunks)app/src/main/java/com/klee/volumelockr/ui/PolicyAccessDialog.kt(1 hunks)app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt(0 hunks)app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt(1 hunks)app/src/main/res/drawable/ic_home.xml(1 hunks)app/src/main/res/drawable/ic_info.xml(1 hunks)app/src/main/res/drawable/ic_settings.xml(1 hunks)app/src/main/res/layout-w600dp/activity_main.xml(1 hunks)app/src/main/res/layout/activity_main.xml(2 hunks)app/src/main/res/layout/fragment_about.xml(1 hunks)app/src/main/res/menu/bottom_nav_menu.xml(1 hunks)app/src/main/res/navigation/navigation_graph.xml(2 hunks)app/src/main/res/values/strings.xml(1 hunks)app/src/main/res/values/themes.xml(1 hunks)
💤 Files with no reviewable changes (1)
- app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt
🔇 Additional comments (16)
.gitignore (1)
4-5: Good additions to .gitignore.Both
build/and.kotlin/are standard directories in Android/Kotlin projects that should be excluded from version control. This follows best practices for project configuration.app/src/main/java/com/klee/volumelockr/service/VolumeService.kt (1)
269-279: LGTM: Notification shown unconditionally to prevent ANR.The method correctly calls
startForeground()without checking lock state, which is necessary to avoid ANR when the service is started viastartForegroundService(). The subsequent shutdown logic inonCreate()handles the case when no locks exist.app/src/main/java/com/klee/volumelockr/ui/PolicyAccessDialog.kt (1)
13-27: LGTM! Clean extraction of the permission dialog.The dialog is properly structured with correct API annotations. The non-cancelable behavior (line 22) forces users to navigate to settings, which is appropriate for a critical permission like Do Not Disturb access.
app/src/main/res/navigation/navigation_graph.xml (2)
8-11: Good refactoring to use AboutFragment directly.Replacing the
aboutlibs_navigationinclude with a direct fragment declaration simplifies the navigation structure and gives more control over the About screen implementation.
16-16: Excellent use of string resources for labels.Using
@string/app_nameand@string/optionsinstead of hardcoded strings improves localization support and maintainability.Also applies to: 28-28
app/src/main/res/layout/activity_main.xml (3)
9-23: Well-structured app bar layout.The AppBarLayout with MaterialToolbar provides a clean, Material Design-compliant navigation structure.
34-35: Verify edge-to-edge inset handling in MainActivity.The FragmentContainerView is correctly constrained between the app bar and bottom navigation. However, the PR mentions edge-to-edge support with manual window insets handling. Ensure that MainActivity properly applies insets to the app bar, bottom navigation, and fragment container to prevent content overlap with system bars.
Based on the PR objectives stating "Manually handles window insets to prevent content overlap with system bars," verify that the following are handled in MainActivity.kt:
WindowCompat.setDecorFitsSystemWindows(window, false)is called- Status bar insets are applied to the app bar
- Navigation bar insets are applied to the bottom navigation
- The fragment container respects both status and navigation bar insets
40-47: Clean bottom navigation implementation.The BottomNavigationView is properly constrained and wired to the navigation menu.
app/src/main/res/layout-w600dp/activity_main.xml (3)
9-18: Excellent responsive design for large screens.The NavigationRailView provides an appropriate side navigation pattern for tablets and foldables. The
menuGravity="center"attribute (line 15) aligns with the PR objective to center navigation items on the rail.
20-34: App bar properly constrained for rail layout.The AppBarLayout correctly spans the remaining width next to the navigation rail, creating a cohesive two-pane layout.
36-48: Fragment container well-positioned in the responsive layout.The FragmentContainerView is correctly constrained to occupy the space beside the navigation rail and below the app bar, completing the responsive design pattern.
app/src/main/res/values/themes.xml (1)
3-5: LGTM! Theme changes support the new navigation architecture.The
NoActionBarparent is necessary for the custom toolbar setup inMainActivity, and settingwindowBackgroundto?attr/colorSurfaceis the correct Material3 pattern for edge-to-edge layouts.app/src/main/java/com/klee/volumelockr/ui/MainActivity.kt (3)
22-29: LGTM! Edge-to-edge setup follows best practices.Calling
enableEdgeToEdge()beforesuper.onCreate()is the recommended approach, and the setup flow is well-organized.
39-51: LGTM! Navigation setup is correct.The navigation configuration properly handles both
bottomNavigationandnavigationRaillayouts. All three destinations are configured as top-level destinations, which creates a flat navigation hierarchy (no up button on any screen) - this is appropriate for a bottom navigation pattern.
64-71: Verify that consuming window insets doesn't affect fragment content.Returning
WindowInsetsCompat.CONSUMEDprevents the insets from reaching child views, including theNavHostFragmentand its fragments. This might cause content to be hidden behind the bottom navigation bar or system bars if fragments don't handle insets independently.Verify that fragments properly handle their own padding/margins to avoid being covered by the navigation bar. Test on devices with:
- Gesture navigation (to verify bottom nav bar overlap)
- Notches/cutouts (to verify proper top padding)
- Large screens with navigation rail
Consider passing insets through to children instead of consuming them entirely:
// Instead of returning CONSUMED, return the original insets // so children can also consume what they need windowInsetsapp/src/main/res/values/strings.xml (1)
3-24: LGTM! String resource updates improve clarity.The updated strings provide better context (especially the policy access dialog title) and support the new navigation structure. The naming is consistent and user-friendly.
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt (1)
92-96: Good fix for ANR, but consider stopping service on all Android versions when no locks exist.The immediate call to
startForeground()correctly prevents ANR on Android O+. However, the check for stopping the service whenmVolumeLock.isEmpty()is nested inside the SDK version check, meaning on pre-O devices the service will continue running even with no locks.Consider moving the empty-lock check outside the version guard:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { tryShowNotification() - - if (mVolumeLock.isEmpty()) { +} + +if (mVolumeLock.isEmpty()) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { stopForeground(Service.STOP_FOREGROUND_REMOVE) - stopSelf() } + stopSelf() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt(3 hunks)app/src/main/res/values/strings.xml(1 hunks)
🔇 Additional comments (1)
app/src/main/res/values/strings.xml (1)
1-25: AI summary contains inaccurate string naming descriptions.The AI summary claims strings
pref_password_protection,pref_create_password,pref_change_password, andpref_allow_lower_volumewere added, but these do not exist in the file. The file instead contains semantically equivalent strings with different naming:password_header,password_protected,change_password, andallow_lower.However, all new strings in the file are properly referenced and in use throughout the codebase (e.g., in
root_preferences.xmlandSettingsFragment.kt). The inconsistency appears to be only in the AI summary's description; the actual code changes are correct.
| val prefs = context.getSharedPreferences(VolumeService.APP_SHARED_PREFERENCES, Context.MODE_PRIVATE) | ||
| val locks = prefs.getString(VolumeService.LOCKS_KEY, "") | ||
|
|
||
| if (!locks.isNullOrEmpty() && locks != "{}") { | ||
| VolumeService.start(context) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all references to LOCKS_KEY to understand usage patterns
rg -n 'LOCKS_KEY' --type kotlin -B 2 -A 2Repository: jonathanklee/VolumeLockr
Length of output: 2392
Remove redundant null check.
Since getString(VolumeService.LOCKS_KEY, "") always returns a non-null string, the null check in isNullOrEmpty() is redundant. Simplify to locks.isNotEmpty() && locks != "{}".
🤖 Prompt for AI Agents
In app/src/main/java/com/klee/volumelockr/BootReceiver.kt around lines 12 to 17,
the condition uses locks.isNullOrEmpty() though
getString(VolumeService.LOCKS_KEY, "") always returns a non-null String; replace
the check with locks.isNotEmpty() && locks != "{}" to remove the redundant null
check and keep the same behavior.
| private fun askForPassword() { | ||
| val editText = EditText(context) | ||
| editText.inputType = InputType.TYPE_CLASS_TEXT or InputType.TYPE_TEXT_VARIATION_PASSWORD | ||
| val view = LayoutInflater.from(context).inflate(R.layout.dialog_password, null) |
There was a problem hiding this comment.
have you missed pushing the resources for this commit ?
There was a problem hiding this comment.
I have reverted the changes to SettingsFragment.kt and removed the custom dialog resources in the latest commit to maintain consistency and resolve the styling/resource issues. The specific code block you pointed out (mixing programmatic EditText with layout inflation) was part of an intermediate state that has now been removed. The current version of SettingsFragment.kt uses the standard AlertDialog implementation.
|
|
||
| MaterialAlertDialogBuilder(requireContext(), com.google.android.material.R.style.ThemeOverlay_Material3_MaterialAlertDialog_Centered) | ||
| .setIcon(R.drawable.ic_lock) | ||
| AlertDialog.Builder(context) |
There was a problem hiding this comment.
Any reason for doing this ?
There was a problem hiding this comment.
The intention was to force the dialog to use the Material 3 style (rounded corners), which wasn't being picked up automatically by the theme in this context. However, to avoid hardcoded styles and potential inconsistencies, I have reverted this specific change in the latest commit. The app now uses the standard AlertDialog.
|
@KoukeNeko Thank you for your contribution!. I have some minor remarks. |
Replace EditTextPreference with Preference and custom MaterialAlertDialogBuilder to ensure password dialogs follow Material Design 3 styling with rounded corners.
|
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/com/klee/volumelockr/ui/SettingsFragment.kt (1)
123-128: Fix potential null pointer exception.Line 127 has a null safety issue. The
getString()call can returnnull, and callingisNotEmpty()onnullwill throw aNullPointerException. The!!operator afterwards doesn't help as the NPE would have already occurred.Apply this diff to fix the null safety issue:
private fun isPasswordSet(): Boolean { return PreferenceManager.getDefaultSharedPreferences(requireContext()).getString( PASSWORD_CHANGE_PREFERENCE, "" - )?.isNotEmpty()!! + )?.isNotEmpty() ?: false }
♻️ Duplicate comments (1)
app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (1)
78-84: Password storage security concern already noted.The plain text storage issue in
SharedPreferenceshas been flagged in theshowChangePasswordDialogcomment above.
🧹 Nitpick comments (3)
app/src/main/res/drawable/ic_lock.xml (1)
7-7: Consider making the color theme-aware if this icon appears in adaptive UI contexts.The lock icon uses a hardcoded black fill color (#FF000000). If this icon is used in contexts that support dark mode or theme-aware coloring, consider referencing a theme color attribute (e.g.,
?attr/colorOnSurface) or a color resource instead of hardcoding black.If theme support is needed, update the fillColor to reference a theme attribute:
- android:fillColor="#FF000000" + android:fillColor="?attr/colorOnSurface"Alternatively, if this should remain black regardless of theme, the hardcoded approach is acceptable and no change is needed.
app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt (1)
15-15: Remove unused import.
LinearLayoutManageris imported but no longer used after switching toGridLayoutManager. Consider removing it for cleaner code.Apply this diff:
-import androidx.recyclerview.widget.LinearLayoutManager +import androidx.recyclerview.widget.GridLayoutManagerapp/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (1)
86-104: Consider extracting duplicated dialog setup logic.The view inflation and keyboard handling logic (lines 87-93) is duplicated from
showChangePasswordDialog(lines 60-66). Extracting this into a helper method would improve maintainability.Consider adding a helper method:
private fun createPasswordInputView(): Pair<View, EditText> { val view = LayoutInflater.from(context).inflate(R.layout.dialog_password, null) val editText = view.findViewById<EditText>(android.R.id.edit) editText.setOnFocusChangeListener { _, _ -> editText.postDelayed({ showKeyboard(editText) }, DELAY_IN_MS) } editText.requestFocus() return Pair(view, editText) }Then use it in both dialogs:
val (view, editText) = createPasswordInputView()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt(3 hunks)app/src/main/java/com/klee/volumelockr/ui/AboutFragment.kt(1 hunks)app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt(4 hunks)app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt(1 hunks)app/src/main/res/drawable/ic_lock.xml(1 hunks)app/src/main/res/layout/dialog_password.xml(1 hunks)app/src/main/res/values-w600dp/bools.xml(1 hunks)app/src/main/res/values/bools.xml(1 hunks)app/src/main/res/values/dimens.xml(1 hunks)app/src/main/res/values/themes.xml(1 hunks)app/src/main/res/xml/root_preferences.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/bools.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/klee/volumelockr/service/VolumeService.kt
- app/src/main/res/values/themes.xml
- app/src/main/java/com/klee/volumelockr/ui/AboutFragment.kt
🔇 Additional comments (7)
app/src/main/res/values-w600dp/bools.xml (1)
1-4: LGTM! Clean resource qualifier implementation.The boolean resource follows Android best practices for responsive design. The
use_two_columnsnaming is clear, and the value is appropriate for wide screens (600dp+).app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt (1)
59-60: LGTM! Solid responsive grid implementation.The span count logic correctly adapts to screen width using the
use_two_columnsresource, and the GridLayoutManager setup is appropriate for the two-column layout on wide screens.app/src/main/res/values/dimens.xml (1)
1-4: LGTM!The dimension resource is well-defined and follows Android conventions. The 24dp margin is appropriate for dialog padding.
app/src/main/res/layout/dialog_password.xml (1)
1-23: LGTM!The password dialog layout is well-structured using Material Design 3 components. The use of
@android:id/editas the ID is appropriate for compatibility with the code that references it.app/src/main/res/xml/root_preferences.xml (1)
18-20: LGTM!The change from
EditTextPreferenceto plainPreferencecorrectly aligns with the refactored dialog-based password change flow inSettingsFragment.kt.app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (2)
5-5: LGTM!The imports and field type changes appropriately support the refactored dialog-based password change flow.
Also applies to: 9-9, 13-13, 27-27
43-46: LGTM!The preference click listener correctly triggers the password change dialog, replacing the previous inline
EditTextPreferencebehavior.
- Enforce minimum 6-character password length with inline error display - Use custom OnClickListener to prevent dialog dismiss on validation failure - Replace SharedPreferences with EncryptedSharedPreferences for secure storage - Add error handling for MasterKey/storage creation with user-facing Toast
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/build.gradle(1 hunks)app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt(5 hunks)app/src/main/res/values/strings.xml(1 hunks)gradle/libs.versions.toml(2 hunks)
🔇 Additional comments (7)
app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (5)
1-30: LGTM!The imports and constants are well-organized and appropriate for the encrypted password implementation. The
MIN_PASSWORD_LENGTHof 6 characters provides a reasonable balance between security and usability.
68-84: LGTM!The encrypted preferences initialization is properly implemented with:
- Secure key scheme (AES256_GCM) for the MasterKey
- Appropriate encryption schemes (AES256_SIV for keys, AES256_GCM for values)
- Safe error handling that gracefully disables password features if initialization fails
86-120: Excellent password validation implementation!The dialog properly validates input before accepting:
- Custom positive button handler prevents auto-dismiss on invalid input
- Inline error display using TextInputLayout
- Minimum length enforcement via
validatePassword()- Only saves and dismisses on valid password
This effectively addresses the password validation concerns from previous reviews.
129-146: LGTM!The password storage correctly uses
EncryptedSharedPreferenceswith proper error handling:
- Null check prevents NPE if encryption initialization failed
- Try-catch with user feedback on save errors
- UI state updated based on password presence
- Boolean return value allows caller to handle success/failure
This addresses the security concern from previous reviews about plain text password storage.
152-182: LGTM!The password verification flow is well-implemented:
- Material UI components with appropriate icon
- Non-cancelable dialog prevents bypassing password protection
- Clear password comparison and UI state updates
Note: The password comparison uses simple equality (
==), which is acceptable for local device protection. For network authentication scenarios, constant-time comparison would be preferred, but that's not applicable here.app/src/main/res/values/strings.xml (1)
1-27: LGTM!The string resource updates properly support the UI changes across the PR:
- Updated navigation labels (Home, Options, VolumeLockr)
- Added password validation and error messages used in SettingsFragment
- Updated volume category titles for improved clarity
- AboutLibraries configuration strings
All changes are consistent with the reported functionality.
gradle/libs.versions.toml (1)
19-41: LGTM!The version catalog entry for
security-cryptois properly structured and follows the established pattern in the file.Note: The version verification concern for
1.1.0-alpha06is covered in theapp/build.gradlereview.
| implementation libs.aboutlibraries.core | ||
| implementation libs.aboutlibraries | ||
| implementation libs.preference.ktx | ||
| implementation libs.security.crypto |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
androidx.security security-crypto latest stable version 1.1.0-alpha06 production ready
💡 Result:
No — 1.1.0-alpha06 is an alpha (pre‑release) and not intended for production. The stable 1.1.0 release was published on July 30, 2025; prefer that (or a later stable) for production. Note: the security-crypto artifacts have been marked deprecated — consider following the deprecation guidance in the release notes. [1][2]
Sources:
[1] AndroidX Security release notes (shows 1.1.0 release on July 30, 2025 and alpha/rc history).
[2] AndroidX versions/all-channel & deprecation note for security-crypto.
🏁 Script executed:
find . -name "libs.versions.toml" -o -name "build.gradle" | head -20Repository: jonathanklee/VolumeLockr
Length of output: 129
🏁 Script executed:
cat gradle/libs.versions.toml | grep -A 2 -B 2 "security-crypto\|security\.crypto"Repository: jonathanklee/VolumeLockr
Length of output: 387
🏁 Script executed:
cat gradle/libs.versions.toml | grep "securityCrypto"Repository: jonathanklee/VolumeLockr
Length of output: 199
Upgrade security-crypto version and address library deprecation.
The dependency uses version 1.1.0-alpha06, an alpha pre-release. A stable version 1.1.0 has been available since July 30, 2025 and should be used for production.
More importantly, androidx.security:security-crypto has been deprecated. Follow the deprecation guidance in the AndroidX Security release notes to migrate to the recommended alternative.
Update gradle/libs.versions.toml line with securityCrypto = "1.1.0" at minimum, and plan migration away from the deprecated library.
- Catch specific exceptions (GeneralSecurityException, IOException) instead of generic Exception - Log caught exceptions to avoid SwallowedException warnings - Remove trailing whitespace in BootReceiver.kt - Add missing newline at end of VolumeService.kt - Remove unused LinearLayoutManager import in VolumeSliderFragment.kt - Fix import ordering in SettingsFragment.kt
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/com/klee/volumelockr/ui/VolumeSliderFragment.kt (1)
80-88: Critical listener leak: registering volume listener but unregistering mode listener
handleServiceConnected()registersregisterOnVolumeChangeListener(...)at line 84, butunbindServiceIfNeeded()callsunregisterOnModeChangeListener()at line 91. The matchingunregisterOnVolumeChangeListener()method exists in VolumeService but is never called, causing the listener to leak across fragment resume/pause cycles and accumulate callbacks. Additionally, the nesteditvariable at line 81 is shadowed by the lambda parameter at line 85, which is brittle and confusing.Fix:
private fun handleServiceConnected() { - mService?.let { - setupRecyclerView(it) - - mService?.registerOnVolumeChangeListener(Handler(Looper.getMainLooper())) { - mAdapter?.update(it.getVolumes()) - } - } + mService?.let { service -> + setupRecyclerView(service) + service.registerOnVolumeChangeListener(Handler(Looper.getMainLooper())) { + mAdapter?.update(service.getVolumes()) + } + } } private fun unbindServiceIfNeeded() { - mService?.unregisterOnModeChangeListener() + mService?.unregisterOnVolumeChangeListener() if (isServiceBound) { context?.let { runCatching { it.unbindService(connection) } } isServiceBound = false } mService = null mAdapter = null }
🧹 Nitpick comments (4)
app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt (1)
57-62: GridLayoutManager switch looks good; avoid recreating adapter/layoutManager unless needed
This responsivespanCountgate is solid. Consider only settinglayoutManager/creatingVolumeAdapterwhenmAdapter == nullto avoid resetting scroll position on rebind/resume.app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (2)
72-92: Initialization and error handling look solid.The MasterKey setup with AES256_GCM and the exception handling are appropriate. The code gracefully degrades if initialization fails, with null checks in downstream methods (savePassword line 139, getStoredPassword line 162).
Consider using a more specific error message for initialization failures (separate from save errors) to help users understand if there's a device-level security issue versus a transient save problem.
190-199: Password verification logic is correct.The straightforward equality check and password presence detection work well for the use case.
Consider documenting password recovery process.
If a user forgets their password, they'll need to clear app data to reset. Consider adding a hint in settings or documentation about this limitation, or implementing a "Reset Password" option that clears all locks when triggered.
app/src/main/java/com/klee/volumelockr/service/VolumeService.kt (1)
93-99: Minor inefficiency: Check locks before showing notification.The service calls
tryShowNotification()at line 91, then immediately checks ifmVolumeLockis empty and stops. This briefly displays a notification that is removed right away when no locks exist at startup.Consider checking
mVolumeLock.isEmpty()before callingtryShowNotification()to avoid the unnecessary notification flash:if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - tryShowNotification() -} - -if (mVolumeLock.isEmpty()) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - stopForeground(Service.STOP_FOREGROUND_REMOVE) + if (mVolumeLock.isEmpty()) { + stopSelf() + return } - stopSelf() + tryShowNotification() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/klee/volumelockr/BootReceiver.kt(1 hunks)app/src/main/java/com/klee/volumelockr/service/VolumeService.kt(2 hunks)app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt(4 hunks)app/src/main/java/com/klee/volumelockr/ui/VolumeSliderFragment.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/klee/volumelockr/BootReceiver.kt
🔇 Additional comments (5)
app/src/main/java/com/klee/volumelockr/ui/SettingsFragment.kt (5)
15-16: LGTM! Security dependencies properly integrated.The addition of EncryptedSharedPreferences with MasterKey provides appropriate protection for password storage. The minimum password length of 6 characters is reasonable for this use case.
Also applies to: 27-27, 32-33
94-128: Excellent dialog implementation with proper validation flow.The pattern of overriding the positive button click listener in
setOnShowListenercorrectly prevents dismissal until validation passes. Inline error display in theTextInputLayoutprovides good UX. This addresses the previous review concern about password validation.
130-159: Password validation and encrypted storage correctly implemented.The validation enforces the minimum length requirement, and
savePasswordproperly handles encryption failures with appropriate logging and user feedback. The boolean return value allows the dialog to control dismissal based on save success.Note: Line 148's
isNotEmpty()check is redundant after validation but serves as defensive programming.
161-163: LGTM!The null-safe retrieval with a sensible default is appropriate.
165-183: LGTM! Consistent Material Design implementation.The password verification dialog appropriately uses MaterialAlertDialogBuilder with a lock icon and is non-cancelable, which fits the security requirements. The lack of validation during verification is appropriate to avoid leaking password policy information.
Add stopSelf() after stopForeground() in tryHideNotification() to properly terminate the service when no locks remain active.
|
Done! Tested on my Pixel 8 Pro. |
2 participants
This PR introduces several improvements and fixes:
Foldable & Large Screen Support:
NavigationRailViewfor screens wider than 600dp.Edge-to-Edge UI:
Navigation:
BottomNavigationView(standard screens) orNavigationRailView(large screens) for easier access to Home, Settings, and About.Bug Fixes:
startForegroundis called immediately.AboutFragmentto properly load library information usingLibsBuilder.Refactoring:
PolicyAccessDialogto a separate file.SettingsFragmentto use cleaner layout handling (though dialog styling remains native for consistency).Summary by CodeRabbit
New Features
Improvements
Behavior
Chores
✏️ Tip: You can customize this high-level summary in your review settings.