Fix storage permission related crash in Android 10 #91
Conversation
tuancoltech
force-pushed
the
bug/crash_storage_permission_android_10_new
branch
from
f508430 to
f0581db
Compare
tuancoltech
force-pushed
the
bug/crash_storage_permission_android_10_new
branch
from
f0581db to
f32c4d4
Compare
|
@coderabbitai Do a code review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you conduct a code review for PR #91. Let me first gather information about the pull request and examine the changes. 🏁 Script executed: Length of output: 33115 @tuancoltech Here's my comprehensive code review for PR #91: 🎯 Overall AssessmentThis is a well-structured PR that addresses a critical Android 10 storage permission issue. The introduction of centralized permission management and state restoration is solid, but there are several areas that need attention. ✅ Positive Changes
|
@coderabbitai: It's ok because we don't need extra handling when view is restored in case of text note. |
|
Tip For best results, initiate chat on the files or code changes. @tuancoltech Thank you for the clarification! I understand now that this is an intentional design decision. You're right that text notes don't require the same level of state restoration as other note types - users can easily continue editing text notes without needing complex state recovery. I appreciate you providing this context about the different restoration requirements between note types. This makes the implementation much clearer from a UX perspective. ✏️ Learnings added
|
@coderabbitai We'll never go into the |
|
Tip For best results, initiate chat on the files or code changes. @tuancoltech You're absolutely right! I apologize for the confusion. The safe call chain Since we're inside the Thank you for the clarification - the code is properly safe as written! 🧠 Learnings used |
@coderabbitai: We're doing nothing heavy inside the |
|
Tip For best results, initiate chat on the files or code changes. @tuancoltech You're absolutely correct! Thank you for the clarification. If the I appreciate you providing this context about the ViewModel's threading model - it shows that the lifecycle management is actually well-designed to avoid UI blocking. ✏️ Learnings added
🧠 Learnings used |
* Update permission request flow to make sure storage permission is asked before accessing data
tuancoltech
force-pushed
the
bug/crash_storage_permission_android_10_new
branch
from
f32c4d4 to
a7069ce
Compare
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Fragment/Activity
participant PermissionManager
participant Permission
participant Repo as NotesRepo
participant ViewModel as NotesViewModel
UI->>PermissionManager: askForWriteStorage(callback)
PermissionManager->>Permission: hasStoragePermission(context)
alt Permission granted
PermissionManager-->>UI: callback(true)
else Permission not granted
PermissionManager->>UI: Launch permission request
UI->>PermissionManager: Permission result
PermissionManager-->>UI: callback(result)
end
sequenceDiagram
participant Fragment
participant ViewModel
participant Repo as NotesRepo
Fragment->>ViewModel: findNote(id, type, onResult)
ViewModel->>Repo: findNote(id)
Repo-->>ViewModel: Note?
ViewModel-->>Fragment: onResult(Note?)
alt Note found
Fragment->>Fragment: onViewRestoredWithNote(Note)
else Not found
Fragment->>Fragment: Handle missing note
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/src/main/java/dev/arkbuilders/arkmemo/models/NoteType.kt (1)
3-7: Clean enum implementation for note type categorization.The enum provides a clear abstraction for the three supported note types and integrates well with the unified note retrieval system.
Consider using a sealed class instead of an enum for better type safety and extensibility:
-enum class NoteType { - TEXT, - VOICE, - GRAPHIC, -} +sealed class NoteType { + object Text : NoteType() + object Voice : NoteType() + object Graphic : NoteType() +}app/src/main/java/dev/arkbuilders/arkmemo/utils/PermissionManager.kt (2)
24-37: Efficient permission request flow with early return optimization.The method correctly checks existing permissions first and returns early if granted, avoiding unnecessary permission dialogs. The version-specific parameter passing (package URI vs permission string) is handled properly.
Consider adding thread safety for concurrent permission requests:
class PermissionManager(val activity: ComponentActivity) { - private var permissionResultCallback: ((granted: Boolean) -> Unit)? = null + private var permissionResultCallback: ((granted: Boolean) -> Unit)? = null + private var isRequestInProgress = false fun askForWriteStorage(onResult: ((granted: Boolean) -> Unit)? = null) { if (Permission.hasStoragePermission(activity)) { onResult?.invoke(true) return } + + if (isRequestInProgress) { + onResult?.invoke(false) + return + } + isRequestInProgress = true permissionResultCallback = onResultAlso consider adding a cleanup method to clear the callback reference when the activity is destroyed to prevent memory leaks.
11-22: Consider potential memory leak with callback storage.The
permissionResultCallbackis stored as an instance variable, which could potentially lead to memory leaks if the callback holds references to activities or fragments that outlive the PermissionManager instance.Consider clearing the callback after use to prevent potential memory leaks:
private val permissionLauncher: ActivityResultLauncher<String> = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { activity.registerForActivityResult(PermissionContract()) { isGranted -> permissionResultCallback?.invoke(isGranted) + permissionResultCallback = null } } else { activity.registerForActivityResult(ActivityResultContracts.RequestPermission()) { isGranted -> permissionResultCallback?.invoke(isGranted) + permissionResultCallback = null } }app/src/main/java/dev/arkbuilders/arkmemo/ui/dialogs/FilePickerDialog.kt (2)
37-37: Good integration with centralized permission management.The nullable
permissionManagerproperty allows proper initialization from the calling activity while maintaining flexibility.Consider adding a documentation comment to clarify that this must be set before calling
show():+ /** Must be set before calling show() to handle permission requests */ var permissionManager: PermissionManager? = null
37-37: Consider making permissionManager non-null for better safety.The nullable
permissionManagercould lead to silent failures if not properly initialized. Since this is essential functionality, consider making it non-null or adding proper null checks.Consider one of these approaches:
Option 1: Make it non-null and require initialization
-var permissionManager: PermissionManager? = null +lateinit var permissionManager: PermissionManagerOption 2: Add defensive null check with logging
permissionManager?.askForWriteStorage { granted -> if (granted) { show() } else { activity.finish() } +} ?: run { + Log.w(TAG, "PermissionManager not initialized, finishing activity") + activity.finish() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/src/main/java/dev/arkbuilders/arkmemo/contracts/PermissionContract.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/models/NoteType.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepo.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/dialogs/FilePickerDialog.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/BaseEditNoteFragment.kt(3 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditGraphicNotesFragment.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditTextNotesFragment.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt(1 hunks)app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/utils/Permission.kt(2 hunks)app/src/main/java/dev/arkbuilders/arkmemo/utils/PermissionManager.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/BaseEditNoteFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/dialogs/FilePickerDialog.kt (1)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditGraphicNotesFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditTextNotesFragment.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
app/src/main/java/dev/arkbuilders/arkmemo/models/NoteType.kt (2)
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.382Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.372Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.
🧬 Code Graph Analysis (5)
app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (1)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (1)
showNoNoteStorageDialog(670-689)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt (1)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (1)
setNote(151-153)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/BaseEditNoteFragment.kt (4)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (2)
getCurrentNote(51-53)onViewRestoredWithNote(63-64)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditGraphicNotesFragment.kt (2)
getCurrentNote(159-161)onViewRestoredWithNote(175-179)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditTextNotesFragment.kt (2)
getCurrentNote(186-188)onViewRestoredWithNote(169-170)app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt (2)
getCurrentNote(375-377)onViewRestoredWithNote(404-407)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (3)
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (3)
setLastLaunchSuccess(231-233)init(52-65)readAllNotes(67-78)app/src/main/java/dev/arkbuilders/arkmemo/preferences/MemoPreferences.kt (1)
setLastLaunchSuccess(20-20)app/src/main/java/dev/arkbuilders/arkmemo/preferences/MemoPreferencesImpl.kt (1)
setLastLaunchSuccess(47-49)
app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (1)
app/src/main/java/dev/arkbuilders/arkmemo/utils/Utils.kt (1)
extractDuration(128-141)
🪛 detekt (1.23.8)
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditTextNotesFragment.kt
[warning] 169-170: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (29)
app/src/main/java/dev/arkbuilders/arkmemo/utils/Permission.kt (1)
18-27: Excellent implementation of version-aware storage permission checking.The function correctly handles the storage permission model changes introduced in Android 11 by using
Environment.isExternalStorageManager()for API 30+ and falling back toWRITE_EXTERNAL_STORAGEpermission check for earlier versions. This addresses the core issue mentioned in the PR objectives for Android 10 storage permission handling.app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/GraphicNotesViewModel.kt (1)
50-53: Good null safety improvement.The change from force unwrapping (
!!) to safe calls (?.) withletproperly handles nullablenote.svgand prevents potential null pointer exceptions. The implementation ensures SVG paths are only processed when the SVG exists.app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt (1)
63-64: Appropriate empty implementation for media player fragment.The empty override of
onViewRestoredWithNoteis correct for this media player fragment, which doesn't require special note restoration logic beyond what the base class provides.app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditTextNotesFragment.kt (1)
169-170: Intentional empty implementation aligns with text note restoration design.Based on the retrieved learnings, this empty implementation is by design - text notes don't require special handling during view restoration as users can easily continue editing them without complex state recovery mechanisms. The static analysis warning about the empty block is a false positive in this architectural context.
app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepo.kt (1)
3-3: LGTM! Clean interface extension for note retrieval.The addition of the
findNotemethod withResourceIdparameter provides a clean way to retrieve individual notes by ID, supporting the new state restoration mechanism. The method signature follows proper conventions with suspend function and nullable return type.Also applies to: 20-20
app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt (1)
60-62: LGTM! Intentional design decision for text note restoration.Returning
nullis an intentional design choice for text notes, as they don't require special handling during view restoration. Users can easily continue editing text notes without complex state recovery mechanisms, making this approach both simpler and user-friendly.app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt (1)
404-407: LGTM! Clean implementation of note restoration callback.The implementation correctly handles voice note restoration by casting to
VoiceNote, setting the note, and initializing the existing note UI. The flow is logical and reuses existing methods appropriately.app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt (1)
274-291: LGTM! Proper permission-gated initialization prevents Android 10 crash.The implementation correctly addresses the storage permission crash by:
- Using centralized
PermissionManagerfor consistent permission handling- Finishing the activity when permission is denied (preventing storage operations that would crash)
- Preserving all existing initialization logic when permission is granted
- Using
isVisiblecheck to avoid inappropriate activity finishingThis directly addresses the Android 10 storage permission crash mentioned in the PR objectives.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/EditGraphicNotesFragment.kt (1)
175-179: LGTM! Appropriate graphic note restoration with canvas redraw.The implementation correctly handles graphic note restoration by:
- Casting to
GraphicNoteand updating the fragment's note- Notifying the view model via
onNoteOpened()to load the SVG data- Invalidating the canvas to trigger a redraw with the restored graphic content
The flow is well-suited for graphic notes that require visual refresh after restoration.
app/src/main/java/dev/arkbuilders/arkmemo/contracts/PermissionContract.kt (1)
17-19: LGTM: Appropriate flag addition for settings intent.Adding
FLAG_ACTIVITY_CLEAR_TOPis a good practice for permission settings intents as it prevents activity stack buildup and ensures clean navigation flow back to the app.app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/NotesViewModel.kt (1)
80-97: LGTM: Well-designed unified note retrieval method.The
findNotemethod properly:
- Uses appropriate coroutine dispatchers (IO for repository work, Main for callback)
- Delegates to the correct repository based on
NoteType- Follows established patterns in the codebase
- Supports the new note restoration mechanism
The implementation aligns well with existing methods like
readAllNotes.app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt (2)
40-43: LGTM: Proper PermissionManager initialization.The centralized permission manager setup is well-designed, creating a single instance for the activity and properly configuring
FilePickerDialogto use it.
61-83: LGTM: Excellent permission handling refactoring.The new permission flow is well-structured and addresses the Android 10 crash issue effectively:
- Uses centralized
PermissionManagerinstead of scattered permission logic- Properly handles permission denial by finishing the activity (appropriate for critical permission)
- Maintains all existing storage availability check logic
- Clean callback-based approach improves readability
This refactoring aligns perfectly with the PR objective of fixing storage permission crashes on Android 10.
app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt (2)
126-134: LGTM: Well-implemented note retrieval method.The
findNotemethod properly:
- Uses appropriate coroutine context switching with IO dispatcher
- Leverages existing
listFileslogic with filtering- Returns the first matching note with non-null SVG
- Follows established patterns in the codebase
This supports the note restoration mechanism effectively.
200-225: LGTM: Excellent refactoring with good separation of concerns.The
toGraphicNoteextension function is a well-designed extraction that:
- Improves code modularity by separating SVG processing logic
- Maintains all existing functionality (SVG parsing, resource creation, properties reading, thumbnail generation)
- Enables reuse between
readStorage()andfindNote()methods- Handles error cases appropriately (null SVG warning)
This refactoring enhances maintainability while supporting the new note restoration feature.
app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/BaseEditNoteFragment.kt (4)
29-31: LGTM: Proper constant definition for bundle key.Good practice to define the bundle key as a constant in the companion object for maintainability and consistency.
184-187: LGTM: Proper state persistence implementation.The
onSaveInstanceStatemethod correctly saves the note's resource ID to the bundle, which is essential for the restoration mechanism.
189-212: LGTM: Robust note restoration implementation.The
onViewStateRestoredmethod is well-designed and addresses the core Android 10 permission issue:
- Uses safe
getParcelableCompatfor bundle retrieval- Critically important: Requests storage permission before restoration, preventing Android 10 crashes
- Leverages
notesViewModel.initcallback which properly handles threading (confirmed by retrieved learnings)- Determines note type pragmatically using fragment class checks
- Uses the new unified
findNotemethod with proper null handling- Delegates to abstract method for subclass-specific restoration
This implementation directly addresses the PR's core objective of fixing storage permission crashes.
222-222: LGTM: Appropriate abstract method for extensibility.The abstract
onViewRestoredWithNotemethod properly enforces subclasses to implement their specific restoration logic. All relevant subclasses (confirmed via relevant code snippets) implement this method appropriately.app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt (6)
43-51: Well-implemented findNote method with proper async handling.The method correctly uses
withContext(iODispatcher)for thread safety, applies consistent filtering for notes with non-empty duration, and efficiently finds the first matching note by resource ID. The implementation aligns well with the existingreadStoragepattern.
121-139: Excellent refactoring to extract voice note creation logic.The
toVoiceNoteextension function properly encapsulates the voice note creation logic, making it reusable betweenfindNoteandreadStorage. The implementation correctly computes resource IDs, reads user properties, and constructs the VoiceNote with all required fields including duration extraction.
117-117: Good refactoring to use the extracted extension function.The updated call to
path.toVoiceNote(helper)maintains the same functionality while leveraging the newly extracted extension function, reducing code duplication and improving maintainability.
43-51: LGTM! Proper implementation of the unified note retrieval interface.The
findNotemethod correctly implements the interface requirement, using appropriate coroutine context switching and filtering logic. The implementation efficiently searches for a voice note by ID while ensuring only notes with valid duration are returned.
121-139: Good refactoring with proper error handling.The extraction of voice note creation logic into the
toVoiceNoteextension function eliminates code duplication and improves maintainability. The function properly handles resource creation and properties retrieval using the helper.
117-117: LGTM! Clean refactoring using the extracted extension function.The call to the new
toVoiceNoteextension function maintains the same functionality while reducing code duplication.app/src/main/java/dev/arkbuilders/arkmemo/utils/PermissionManager.kt (2)
11-22: Well-designed centralized permission manager with proper version handling.The class effectively abstracts Android version differences by using
PermissionContractfor Android R+ and standardRequestPermissionfor older versions. The conditional launcher registration is handled correctly.
24-37: LGTM! Well-structured permission request flow.The method properly checks existing permissions before requesting, handles version-specific logic cleanly, and provides appropriate callback handling. The early return for already-granted permissions is efficient.
app/src/main/java/dev/arkbuilders/arkmemo/ui/dialogs/FilePickerDialog.kt (2)
48-58: Excellent refactoring to use centralized permission utilities.The updated permission handling properly uses
Permission.hasStoragePermission()for consistent checking and delegates toPermissionManagerfor requests. The callback correctly maintains existing behavior by showing the dialog on grant or finishing the activity on denial.
48-58: LGTM! Clean integration with centralized permission management.The refactored permission handling properly delegates to the PermissionManager while maintaining the same user experience. The callback logic correctly handles both granted and denied cases.
Ticket: https://app.asana.com/0/1207829522063916/1208903376779601
Summary by CodeRabbit
New Features
Improvements
Bug Fixes