Skip to content

fix: Import cloud-backed PKPASS files off the main thread - #3282

Open
mvanhorn wants to merge 4 commits into
CatimaLoyalty:mainfrom
mvanhorn:fix/2464-cloud-pkpass-main-thread
Open

fix: Import cloud-backed PKPASS files off the main thread#3282
mvanhorn wants to merge 4 commits into
CatimaLoyalty:mainfrom
mvanhorn:fix/2464-cloud-pkpass-main-thread

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Keep PkpassParser, PkpassesParser, and the existing synchronous parsing utilities as the parsing boundary, but invoke PKPASS and PKPASSES work from lifecycle-scoped coroutines on Dispatchers.IO in both production entry points. PKPASS imports opened through a cloud-backed Android document provider can require network access while ContentResolver.openInputStream is resolving the URI.

Opening a valid .pkpass through MainActivity with a content stream that records its caller reads the provider stream off the main looper, then processes the parsed result on the main thread; Returning a valid .pkpass from ScanActivity's picker reads and parses off the main looper, then delivers the selected result while UI/scanner state changes remain on the main thread.

Fixes #2464

@TheLastProject

Copy link
Copy Markdown
Member

I guess this makes some sense but I have some notes:

  1. Why explicitly only pkpass parsing? Wouldn't it make more sense to just throw all the parsing into there? You can load images from online sources too.
  2. Why remove all the alert toasts that parsing failed? Users need to know it went wrong instead of it being a silent failure.
  3. You added tests, which is nice, but they fail. Can you look into that please?

…failure toasts

- Route image/PDF/pkpass/espass imports through one background importFile
- Restore errorReadingFile / errorReadingImage / noBarcodeFound toasts via a
  main-looper-marshalling showToast helper
- Replace fixed-timeout awaits in the activity tests with a waitFor predicate
…read

launch(Dispatchers.IO) starts the coroutine on a background thread immediately,
so a paused Robolectric main looper never drives it and the activity tests time
out. Launch on the lifecycle scope's default (main) dispatcher and wrap only the
blocking read in withContext(Dispatchers.IO).
Two separate bugs, both in the tests I added:

- new Intent(ACTION_VIEW, uri).setType(...) clears the data URI (setType and
  setData clear each other), so the activity received a null Uri and the read
  never started. Use setDataAndType.
- The intermediate-state assertions (no started activity / RESULT_CANCELED)
  are not observable: waiting on the main looper drains it, and Robolectric
  drains it again during .visible(), so the import has already run to
  completion by the time the test regains control. Assert the off-main-thread
  read directly instead, which is the property these tests exist to prove.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

All three notes addressed, CI is green.

  1. Not just pkpass any more - image, PDF, pkpass, pkpasses and espass all go through one background importFile. The pkpasses plural path still routes to retrieveBarcodesFromPkPasses so multi-pass files are unaffected.

  2. Toasts restored. They now go through a small showToast helper that marshals to the main looper, since the call sites moved off the main thread.

  3. The test failures were two separate bugs, both mine:

    • new Intent(ACTION_VIEW, uri).setType(...) silently clears the data URI, because setData and setType clear each other. The activity was receiving a null Uri, so the read never started and the latch timed out. Fixed with setDataAndType.
    • The intermediate-state assertions (no started activity, RESULT_CANCELED) were never observable. Waiting on the main looper drains it, and Robolectric drains it again during .visible(), so the import has already run to completion by the time the test regains control. Those now assert the off-main-thread read directly, which is the property the tests exist to prove.

Unrelated: LoyaltyCardViewActivityTest.startWithLoyaltyCardNoExpirySetExpiry fails locally at a date rollover (expects "August 26" and gets "August 25"). It fails the same way on the base branch without my changes, so it is not from this PR, but you may want to pin a clock in that test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Catima can't read file opened from Nextcloud in file manager (NetworkOnMainThreadException)

2 participants