Fix export duplicate file names and improve progress dialog#674
Fix export duplicate file names and improve progress dialog#674
Conversation
WalkthroughThis PR refactors progress tracking by converting the Changes
Sequence DiagramsequenceDiagram participant UI as Activity/Fragment participant Progress as Progress LiveData participant Dialog as Progress Dialog participant ViewModel as ViewModel UI->>ViewModel: trigger action (export/delete) ViewModel->>Progress: post(ExportNotesProgress/DeleteProgress/...) Progress->>Dialog: observe titleId from progress Dialog->>Dialog: update title using progress.titleId Dialog->>Dialog: render progress bar ViewModel->>Progress: post(updated progress state) Progress->>Dialog: notify with new titleId Dialog->>Dialog: update display Note over ViewModel: Title now derived from Progress subclass<br/>instead of passed as separate parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve a consistent refactoring pattern across multiple files (abstract class introduction and subclass creation), but include heterogeneous modifications spanning UI extensions, view models, utilities, and resources. The duplicate filename prevention logic in export utilities requires careful verification against the linked issue (#637), and API signature changes across public methods necessitate thorough tracing of call sites. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Crustack force-pushed the fix/export-notes branch from f57e87d to e7902bb Compare ![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/src/main/res/values/strings.xml (1)
156-159: Plural placeholders and casing consistencyUse %1$d for counts and match casing with existing plurals (“note/notes”). Suggested change:
- <plurals name="exported_notes"> - <item quantity="one">Exported %1$s Note</item> - <item quantity="other">Exported %1$s Notes</item> - </plurals> + <plurals name="exported_notes"> + <item quantity="one">Exported %1$d note</item> + <item quantity="other">Exported %1$d notes</item> + </plurals>app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/ExportNotesProgress.kt (1)
6-11: Prefer gerund progress title (“Exporting notes”)To match “Adding files”/“Deleting files”, use a gerund. If acceptable, add a new string and switch:
- ) : Progress(R.string.export, current, total, inProgress, indeterminate) + ) : Progress(R.string.exporting_notes, current, total, inProgress, indeterminate)Add to strings.xml:
<string name="exporting_notes">Exporting notes</string>app/src/main/java/com/philkes/notallyx/presentation/view/misc/Progress.kt (1)
3-9: Annotate titleId and (optionally) validate boundsAdd @stringres for compile-time checking; optional guards keep state sane.
package com.philkes.notallyx.presentation.view.misc -abstract class Progress( - val titleId: Int, +import androidx.annotation.StringRes + +abstract class Progress( + @StringRes val titleId: Int, val current: Int = 0, val total: Int = 0, val inProgress: Boolean = true, val indeterminate: Boolean = false, )Optional init checks:
init { require(current >= 0) { "current must be >= 0" } require(total >= 0) { "total must be >= 0" } if (!indeterminate) require(current <= total) { "current must be <= total" } }app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/BackupProgress.kt (1)
6-11: Use “Exporting backup” for in-progress titleAlign with gerund style used elsewhere:
-) : Progress(R.string.export_backup, current, total, inProgress, indeterminate) +) : Progress(R.string.exporting_backup, current, total, inProgress, indeterminate)app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt (1)
624-641: LGTM with optional optimization suggestion.The duplicate filename resolution logic is correct and handles the recursive export calls properly. The function returns the base filename without extension, which the callers then use appropriately.
Optional optimization
For large folders, listing all files on each duplicate check could be expensive. Consider caching the file list if exporting many notes:
private fun findFreeDuplicateFileName( folder: DocumentFile, fileName: String, fileExtension: String, existingNames: Set<String>? = null, ): String { val names = existingNames ?: folder.listFiles().mapNotNull { it.name }.toSet() if ("$fileName.$fileExtension" !in names) return fileName var index = 0 var newName: String do { index++ newName = "$fileName ($index).$fileExtension" } while (newName in names) return "$fileName ($index)" }However, the current implementation is acceptable for typical export volumes.
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/DeleteAttachmentProgress.kt (1)
6-11: Consider whether theopenmodifier is necessary.The implementation correctly follows the new Progress pattern with a dedicated titleId. However, the
openmodifier allows subclassing. If no subclasses ofDeleteAttachmentProgressare planned or exist, consider making this classfinal(removingopen) to prevent unintended inheritance.Apply this diff if no subclasses exist or are planned:
-open class DeleteAttachmentProgress( +class DeleteAttachmentProgress( current: Int = 0, total: Int = 0, inProgress: Boolean = true, indeterminate: Boolean = false, ) : Progress(R.string.deleting_files, current, total, inProgress, indeterminate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (16)
app/src/main/java/com/philkes/notallyx/data/imports/ImportProgress.kt(2 hunks)app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt(2 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt(2 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/view/misc/Progress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt(7 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt(3 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/AddFilesProgress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/BackupProgress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/DeleteAttachmentProgress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/DeleteProgress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/ExportNotesProgress.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt(3 hunks)app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt(11 hunks)app/src/main/res/values/strings.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt (2)
exportPdfFile(509-566)exportPlainTextFile(568-622)
🔇 Additional comments (17)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/DeleteProgress.kt (1)
6-11: LGTM — consistent progress titleUsing R.string.deleting_files aligns with existing gerund progress strings. No issues.
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (3)
8-8: LGTM!The new imports are necessary for the unified progress tracking and PDF export functionality.
Also applies to: 56-57
129-129: LGTM!Consolidating multiple progress trackers into a single unified
progressobject simplifies the API while maintaining type safety through specialized Progress subclasses.
529-546: LGTM!The deletion flow properly uses
DeleteProgressfor progress tracking and correctly passes it through todeleteAttachments.app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt (3)
50-50: LGTM!Consistent use of
BackupProgressthroughout export operations provides better type safety and clearer progress semantics.Also applies to: 268-268, 295-295, 327-329, 341-341, 449-449, 553-553, 619-619
509-566: LGTM with minor TOCTOU consideration.The duplicate filename handling correctly uses
findFreeDuplicateFileNameand recursively retries with a new name.Note: There's a potential time-of-check-time-of-use (TOCTOU) gap between checking for duplicates (line 522) and creating the file (line 537). However, this is acceptable because:
- Export operations are typically single-threaded per user
DocumentFile.createFilelikely has internal collision handling- The recursive approach will eventually find a free name
If you encounter issues with concurrent exports to the same folder, consider using a synchronized block or checking the result of
createFilefor null.
775-787: LGTM!Using
elseinstead of explicitly listingTXT,JSON,HTMLmakes the code more maintainable and ensures future MIME types are handled correctly.app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt (1)
50-50: LGTM!Setting up the progress dialog in
onCreateensures it's ready to display progress for export, delete, and other operations throughout the activity lifecycle.Also applies to: 132-132
app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt (1)
1205-1205: LGTM!Removing the
titleIdparameter is correct sinceAddFilesProgressnow carries its own title resource (R.string.adding_files). This aligns with the broader Progress refactoring.app/src/main/java/com/philkes/notallyx/data/imports/ImportProgress.kt (1)
3-3: LGTM!Adding the
titleIdparameter to theProgresssuperclass constructor alignsImportProgresswith the unified progress hierarchy.Also applies to: 12-12
app/src/main/java/com/philkes/notallyx/utils/IOExtensions.kt (1)
17-17: LGTM!Consistent use of
DeleteAttachmentProgressfor attachment deletion operations provides better type safety and clearer progress semantics.Also applies to: 121-121, 138-138, 144-144
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt (1)
42-42: LGTM!Using
AddFilesProgressfor file addition operations provides better type safety and progress tracking semantics. The progress values correctly track file addition: initial state (0, total), per-file progress (index+1, total), and completion (inProgress=false).Also applies to: 160-160, 170-170, 173-173
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/AddFilesProgress.kt (1)
1-11: LGTM!The
AddFilesProgressclass is well-defined and follows the pattern of other specialized Progress subclasses. The default parameters are appropriate, and the fixedtitleIdofR.string.adding_filescorrectly represents file addition operations.app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (1)
403-403: LGTM! Progress dialog setup simplified.The refactoring correctly removes the explicit title parameter, now deriving it from the Progress instance's
titleIdproperty. This improves encapsulation and consistency across the codebase.app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (3)
414-431: LGTM! Import progress dialog correctly uses specialized rendering.The
setupImportProgressDialogmethod properly delegates to the basesetupProgressDialogwith a customrenderProgresslambda that handles theImportStageenum. The title is correctly derived fromprogress.titleIdin the underlying implementation.
447-485: Verification confirmed: Progress class is abstract with required titleId property.The Progress class at
app/src/main/java/com/philkes/notallyx/presentation/view/misc/Progress.ktis correctly defined as abstract withtitleId: Intas a constructor parameter (line 4). The code at line 462 correctly usesprogress.titleIdto set the dialog title. All assumptions underlying this refactoring are valid.
402-412: All call sites have been correctly updated for the breaking API change.The verification confirms the refactoring is complete and consistent:
- External call sites (EditActivity.kt:1205, MainActivity.kt:132) use the new single-parameter signature
- Internal delegation calls correctly invoke the new 3-parameter private method signature without
titleId- No old 4-parameter calls with
titleIdremain in the codebase- Title is properly derived from
progress.titleIdat line 462
| ExportMimeType.PDF -> { | ||
| for (note in notes) { | ||
| exportPdfFile( | ||
| app, | ||
| note, | ||
| selectedExportMimeType, | ||
| DocumentFile.fromTreeUri(app, folderUri)!!, | ||
| progress = exportProgress, | ||
| progress = progress, | ||
| counter = counter, | ||
| total = notes.size, | ||
| pdfPrintListener = | ||
| object : PdfPrintListener { | ||
| override fun onSuccess(file: DocumentFile) { | ||
| actionMode.close(true) | ||
| progress.postValue(ExportNotesProgress(inProgress = false)) | ||
| val message = | ||
| app.getQuantityString( | ||
| R.plurals.exported_notes, | ||
| counter.get(), | ||
| ) | ||
| app.showToast("$message to '${folderUri.lastPathSegment}'") | ||
| } | ||
| | ||
| override fun onFailure(message: CharSequence?) { | ||
| app.log(TAG, stackTrace = message as String?) | ||
| actionMode.close(true) | ||
| progress.postValue(ExportNotesProgress(inProgress = false)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| | ||
| ExportMimeType.PDF -> { | ||
| exportPdfFile( | ||
| } |
There was a problem hiding this comment.
Fix PDF export completion logic to avoid redundant callbacks.
The completion check at line 555 (if (counter!!.get() == total!!)) is inside the success callback of every PDF export. This means that when the counter reaches the total, the callback will trigger actionMode.close(), post completion progress, and show a toast. However, since this is checked in every file's callback, there's a race condition where multiple files completing simultaneously could all pass this check.
Consider adding synchronization or using compareAndSet:
override fun onSuccess(file: DocumentFile) { app.contentResolver.openOutputStream(it.uri)?.use { outStream -> app.contentResolver.openInputStream(file.uri)?.copyTo(outStream) } progress?.postValue( BackupProgress(current = counter!!.incrementAndGet(), total = total!!) ) - if (counter!!.get() == total!!) { + if (counter.get() == total) { pdfPrintListener?.onSuccess(file) } }Also note the redundant null assertions (!!) on lines 553, 555-557 - counter and total are already dereferenced with !! earlier in the function.
Committable suggestion skipped: line range outside the PR's diff.
1 participant
Fixes #637
Summary by CodeRabbit
New Features
Improvements