Conversation
app/src/main/java/app/grapheneos/pdfviewer/preferences/PdfPreferencesRepository.kt
Show resolved
Hide resolved
app/src/main/java/app/grapheneos/pdfviewer/preferences/PdfPreferencesRepository.kt
Show resolved
Hide resolved
app/src/main/java/app/grapheneos/pdfviewer/viewModel/PdfViewModel.kt
Outdated
Show resolved
Hide resolved
8bd6c4b to
128b255
Compare
|
|
||
| class SettingsDialog : DialogFragment() { | ||
|
|
||
| private var _binding: DialogSettingsBinding? = null |
There was a problem hiding this comment.
This property is redundant, it's used only inside onCreate()
| override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { | ||
| _binding = DialogSettingsBinding.inflate(layoutInflater) | ||
|
|
||
| val prefs = requireContext().getSharedPreferences("app_prefs", Context.MODE_PRIVATE) |
| object FileHash { | ||
|
|
||
| private const val HASH_BUFFER_SIZE = 8192 | ||
| private const val MAX_HASH_BYTES = 16 * 1024 // 16KB |
There was a problem hiding this comment.
Why size limit is this low? Are first 16 KiB of a PDF file guaranteed to be distinct for all PDF files?
There was a problem hiding this comment.
I expand it to 10 MB. I looked it up and on modern mobile devices this shall take at most 1 second, which sounds fine to me.
There was a problem hiding this comment.
The hash is removed. It now uses fingerprint from pdf.js directly, which reads ID from a pdf file first, and if that fails, computes MD5 hash for the first 1KB.
| totalBytesRead += bytesToHash | ||
| } | ||
|
|
||
| digest.digest().joinToString("") { "%02x".format(it) } |
There was a problem hiding this comment.
Need to use kotlin.text.HexFormat for such conversions, the current approach is very inefficient
|
|
||
| hashCalculationDeferred = viewModelScope.async { | ||
| val hash = FileHash.calculateFileHash(uri, contentResolver) | ||
| currentFileHash = hash |
There was a problem hiding this comment.
There is a race condition: currentFileHash write is not guaranteed to be cancelled when hashCalculationDeferred is cancelled
There was a problem hiding this comment.
I moved the assignment to when the Deferred is actually used.
| return pdfStateFlow.first().filePagePositions[fileHash] | ||
| } | ||
|
|
||
| suspend fun clearAll() { |
| put(key, jsonObject.getInt(key)) | ||
| } | ||
| } | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
This conversion should never fail, it's a bug if it does fail. We shouldn't hide potential bugs by swallowing exceptions
There was a problem hiding this comment.
I removed the try..catch block.
|
|
||
| fun hasUriPermission(uri: Uri?): Boolean { | ||
| return contentResolver.persistedUriPermissions | ||
| .stream() |
There was a problem hiding this comment.
Why are you using Java stream in Kotlin code?
There was a problem hiding this comment.
It was moved from the Java main class. I changed it to use Kotlin any.
| Uri uri = Uri.parse(state.getLastOpenedUri()); | ||
| if (viewModel.hasUriPermission(uri)) { | ||
| try { | ||
| getContentResolver().openInputStream(uri).close(); |
There was a problem hiding this comment.
Opening a Uri is an expensive operation, especially for some third-party content providers. Content resolver result should be reused
There was a problem hiding this comment.
I changed it to just rely on the permission check, so no need to open the actual uri.
| private fun releaseUriPermissionIfHeld(uri: Uri) { | ||
| try { | ||
| // Find if we have persisted permission for this URI | ||
| val persistedPermissions = contentResolver.persistedUriPermissions |
There was a problem hiding this comment.
It's unnecessary and slow to obtain all persisted Uri permissions.
This entire code block can be replaced with
try {
contentResolver.releasePersistableUriPermission(uri, Intent.FLAG_GRANT_READ_URI_PERMISSION or Intent.FLAG_GRANT_WRITE_URI_PERMISSION)
} catch (e: SecurityException) {
Log.w(TAG, "permission release failed", e)
}
dd73508 to
f4be0c6
Compare
| private val Context.dataStore: DataStore<Preferences> by preferencesDataStore( | ||
| name = "pdf_preferences" | ||
| ) |
There was a problem hiding this comment.
I think we should also handle data format corruption here (corruptionHandler) to explicitly replace it with empty preferences. We can keep the .catch in the flow for other errors like actual IO issues during reading
There was a problem hiding this comment.
Yes, it is a good idea. I added a ReplaceFileCorruptionHandler to clear the preferences when there is a corruption. The exception handler for reading can be kept the same for IOException.
- Saving last opened file uri - Saving last opened page for each file - Hashing files (first 64 KB) to resume regardless of file path - Add setting to toggle resume behavior on/off - Bug fix: mNumPages not cleared when opening new document
f4be0c6 to
ffc40ce
Compare
Added the following functionalities:
There is an edge case when a document is opened from
ACTION_VIEW(e.g. from Files) and then the app is killed, then it cannot re-open last document. This is due to a security limitation of Android system.closes #280