Multiple improvements and refactors by ggtlvkma356 · Pull Request #616 · GrapheneOS/PdfViewer

ggtlvkma356 · 2026-03-27T16:56:29Z

List of changes

Move UI operations to the main thread.
Make shared mutable state thread-safe.
Move file saving off UI thread.
Remove Loader. Document properties are loaded using ViewModel.
Remove onSaveInstanceState. Persistent states are managed with ViewModel.
Simplify Menu handling.
Display page number with Snackbar instead of Toast.
Remove Hungarian notation.

inthewaves

As a general note: Technically, JavaScript is single-threaded and the JS Bridge code blocks when calling bridge methods, so concurrency between bridge calls is less of an issue (i.e., JS will never concurrently call bridge methods). Of course, the JS bridge background thread in Java still can have concurrency issue with pure Android threads

From Chromium source on android_webview/docs/java-bridge.md: "Calls to interface methods are synchronous—JavaScript VM stops and waits for a result to be returned from the invoked method. In Chromium, this means that the IPC message sent from a renderer to the browser must be synchronous."

inthewaves · 2026-04-07T18:40:00Z

+    fun retrieveDocumentProperties(properties: String, numPages: Int, uri: Uri) {
+        viewModelScope.launch(Dispatchers.IO) {
+            val loader = DocumentPropertiesRetriever(getApplication(), properties, numPages, uri)
+            val result = loader.retrieve()
+            withContext(Dispatchers.Main) {
+                if (documentPropertiesLoading) {
+                    _documentProperties.value = result
+                }
+            }
+        }
+        documentPropertiesLoading = true


I think documentPropertiesLoading = true should be before viewModelScope.launch(Dispatchers.IO). Technically doesn't matter since this seems to be on main thread anyway, so withContext(Dispatchers.Main) would always catch this

This is no longer needed after the job cancellation changes below.

inthewaves · 2026-04-07T18:46:13Z

+    private volatile float zoomFocusY = 0f;
+    private boolean documentLoaded;
+    private volatile InputStream inputStream;
+    private volatile boolean documentPropertiesLoaded;


I don't think volatile works here for this pattern. For this compound operation of read-and-then-set, it only helps with read visibility

@JavascriptInterface public void setDocumentProperties(final String properties) { if (documentPropertiesLoaded) { throw new SecurityException("setDocumentProperties already called"); } documentPropertiesLoaded = true; ... }

https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html

Atomic actions cannot be interleaved, so they can be used without fear of thread interference. However, this does not eliminate all need to synchronize atomic actions, because memory consistency errors are still possible.

AtomicBoolean#compareAndSet would be more correct here.

Just as a note from my own looking: even if this bridge method itself is synchronous with respect to JS code, documentPropertiesLoaded is still technically read/written by Android main thread in loadPdf(), so doesn't seem correct to drop this pattern entirely

inthewaves · 2026-04-07T18:57:40Z

        } else if (itemId == R.id.action_save_as) {
            saveDocument();
+            return true;


inthewaves · 2026-05-12T21:30:36Z

+        viewModelScope.launch(Dispatchers.IO) {
+            val loader = DocumentPropertiesRetriever(getApplication(), properties, numPages, uri)
+            val result = loader.retrieve()
+            val name = resolveDocumentName(result)
+            withContext(Dispatchers.Main) {
+                if (documentPropertiesLoading) {
+                    _documentProperties.value = result
+                    _documentName.value = name
+                }
+            }
+        }


There's technically a race here if document property loading takes a long time. If you load properties for a doc and then switch to another doc and the previous job is still running, it could replace it with previous doc properties

When a new document is loaded, I think this job should probably be cancelled, or at the very least the job should check that the input uri parameter matches the uri before replacing _documentProperties.value and _documentName.value

Note that DocumentPropertiesRetriever and its usage of PDFJsPropertiesToDocumentPropertyConverter are not suspending functions and they don't call cancellable functions (they seem to be pure Kotlin functions), so cancelling the job would only stop the job after it has finished converting when it enters withContext(Dispatchers.Main)

I think the final results are still correct in this case, because if the first document property is still loading when you open a second document, it will start a new coroutine and eventually will overwrite the results from the first one (unless the second coroutine somehow finishes before the first one, which is very unlikely). But it is good to cancel the job in any case so I will add the cancellation.

inthewaves · 2026-05-12T21:42:31Z

+    private void handleNewUri(Uri newUri) {
+        Uri oldUri = viewModel.getUri();
+        if (oldUri != null) {
+            try {
+                getContentResolver().releasePersistableUriPermission(oldUri, Intent.FLAG_GRANT_READ_URI_PERMISSION);
+            } catch (SecurityException ignored) {}
        }
+        try {
+            getContentResolver().takePersistableUriPermission(newUri, Intent.FLAG_GRANT_READ_URI_PERMISSION);
+        } catch (SecurityException ignored) {}
+        viewModel.setUri(newUri);


I think this is from the page persistence branch, we should remove it

inthewaves · 2026-05-12T21:44:45Z

-            mZoomRatio = Math.max(Math.min(ratio, MAX_ZOOM_RATIO), MIN_ZOOM_RATIO);
+            runOnUiThread(() ->
+                    viewModel.setZoomRatio(Math.max(Math.min(ratio, MAX_ZOOM_RATIO), MIN_ZOOM_RATIO))
+            );


Note that this pattern technically makes setting the zoom ratio asynchronous which changes the semantics. JS Bridge code should block JS exectuion when calling bridge methods, so offloading to Android main thread could result in later bridge getZoomRatio calls having a stale value

This is probably unlikely in practice depending on how far away subsequent getZoomRatio calls are from setZoomRatio calls. But I think we can just remove this runOnUiThread call and rely on zoomRatio being volatile

Yes the runOnUiThread is removed now.

inthewaves · 2026-05-18T19:06:28Z

+    @Volatile
+    var encryptedDocumentPassword: String = ""


encryptedDocumentPassword is not stored in some saved state so it won't be restored after process death, but I think that's actually fine to not keep a password around for that long. Still saved after activity recreation like rotation, etc.

Yes, it is changed intentionally so that passwords are not saved after process is dead.

inthewaves

Looks fine, instrumented tests passed locally

This should be squashed when it's merged

thestinger · 2026-05-18T19:25:29Z

@ggtlvkma356 Can you rebase this yourself to reduce the number of commits?

- Rename DocumentPropertiesLoader and relevant functions - Remove toast and use snackbar - Simplify menu handling - Remove SaveInstanceState and use ViewModel instead - Remove unnecessary Loader from PdfViewer - Move diskIO off main thread - Improve thread-safety on mutable fields - Update PasswordPromptFragment on UI thread - Remove the usage of Hungarian Notation

ggtlvkma356 · 2026-05-18T19:36:51Z

I have squashed the commits into one.

inthewaves suggested changes Apr 7, 2026

View reviewed changes

ggtlvkma356 mentioned this pull request Apr 7, 2026

Allow jumping to previous/next page with horizontal swipe gesture #603

Merged

ggtlvkma356 force-pushed the pdfviewer-improvements branch 2 times, most recently from 5aa4a3c to 0984f3c Compare May 12, 2026 17:07

inthewaves suggested changes May 12, 2026

View reviewed changes

ggtlvkma356 force-pushed the pdfviewer-improvements branch 2 times, most recently from 5e93dae to b1901cf Compare May 18, 2026 16:36

inthewaves reviewed May 18, 2026

View reviewed changes

inthewaves approved these changes May 18, 2026

View reviewed changes

ggtlvkma356 force-pushed the pdfviewer-improvements branch from b1901cf to 22d78a6 Compare May 18, 2026 19:36

inthewaves approved these changes May 24, 2026

View reviewed changes

Uh oh!

Conversation

ggtlvkma356 commented Mar 27, 2026

List of changes

Uh oh!

inthewaves left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inthewaves May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inthewaves left a comment

Choose a reason for hiding this comment

Uh oh!

thestinger commented May 18, 2026

Uh oh!

ggtlvkma356 commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

inthewaves May 12, 2026 •

edited

Loading