Multiple improvements and refactors

[ggtlvkma356]

List of changes

1. Move UI operations to the main thread.

2. Make shared mutable state thread-safe.

3. Move file saving off UI thread.

4. Remove Loader. Document properties are loaded using ViewModel.

5. Remove onSaveInstanceState. Persistent states are managed with ViewModel.

6. Simplify Menu handling.

7. Display page number with Snackbar instead of Toast.

8. 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]

Expanding it right now would be fileName == null || !fileName.isEmpty() so it will accept null if that's intentional. (although null never happens because of properties.getOrDefault

We can consider also just using !TextUtils.isEmpty. TextUtils.isEmpty is a framework method that returns true iff the string is null or 0-length

[inthewaves]

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

[inthewaves]

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]

Good catch

[inthewaves]

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)

[inthewaves]

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

[inthewaves]

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