refactor: mount receivers #16714

Arthur-Milchior · 2024-07-10T03:51:32Z

This ensure there is no change in behaviour.

I checked whether we could try to not export the download of decks, but it does not works.
The app does not crash on api 24 on simulator.

There was a slight problem, with variables that could potentially be null. To ensure the type checker agreed with the code, without using !!, I had to rewrite some of the logic.

I also ensured that some of the code that was copy/pasted in various activity now entirely belong to AnkiActivity. As a side effect, all activities that used to listen to the "unmount" notification and finished, will now also show a toast when the card gets unmounted. Currently it was only the deck picker.

Fixes State whether register are registered #16713

Arthur-Milchior · 2024-07-10T12:41:41Z

RECEIVER_EXPORTED is not available before API 33. If I could use it outside of the CompatV33 file, there would be no reason to have a compat file.
The registerReceiver with flag appeared at API 26

If you really want the function to accept a flag, I could still accept a flag, and do a xor with the EXPORTED flag

david-allison · 2024-07-10T13:51:01Z

The patch above uses ContextCompat.RECEIVER_EXPORTED which is available on lower APIs

mikehardy

this works locally on an API23 emulator even (have to do some manual clicking because of the outdated-webview-check popup, during test)

this picks cleanly to release-2.18 in case we need it

mikehardy · 2024-07-10T15:42:45Z

@david-allison with limited time and having already reviewed it I'm not sure on the value of splitting the refactor to a commit. It appears to work in local testing, I'd merge as is but you've got a pending reject, so back to you

mikehardy

Wait, still appears to be incomplete, when I change the targetSdk in versions lib and attempt to run ./gradlew jacocoTestReport with an API34 emulator running, I see this:

Process: com.ichi2.anki.debug, PID: 7566
java.lang.RuntimeException: Unable to start activity ComponentInfo{com.ichi2.anki.debug/com.ichi2.anki.FilteredDeckOptions}: java.lang.SecurityException: com.ichi2.anki.debug: One of RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED should be specified when a receiver isn't being registered exclusively for system broadcasts
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3782)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3922)
        at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:103)
        at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:139)
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:96)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2443)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loopOnce(Looper.java:205)
        at android.os.Looper.loop(Looper.java:294)
        at android.app.ActivityThread.main(ActivityThread.java:8177)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)
Caused by: java.lang.SecurityException: com.ichi2.anki.debug: One of RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED should be specified when a receiver isn't being registered exclusively for system broadcasts
        at android.os.Parcel.createExceptionOrNull(Parcel.java:3057)
        at android.os.Parcel.createException(Parcel.java:3041)
        at android.os.Parcel.readException(Parcel.java:3024)
        at android.os.Parcel.readException(Parcel.java:2966)
        at android.app.IActivityManager$Stub$Proxy.registerReceiverWithFeature(IActivityManager.java:5684)
        at java.lang.reflect.Method.invoke(Native Method)
        at leakcanary.ServiceWatcher$install$4$2.invoke(ServiceWatcher.kt:93)
        at java.lang.reflect.Proxy.invoke(Proxy.java:1006)
        at $Proxy4.registerReceiverWithFeature(Unknown Source)
        at android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1852)
        at android.app.ContextImpl.registerReceiver(ContextImpl.java:1792)
        at android.app.ContextImpl.registerReceiver(ContextImpl.java:1780)
        at android.content.ContextWrapper.registerReceiver(ContextWrapper.java:755)
        at com.ichi2.ui.AppCompatPreferenceActivity.registerExternalStorageListener(AppCompatPreferenceActivity.kt:288)
        at com.ichi2.anki.FilteredDeckOptions.onCreate(FilteredDeckOptions.kt:211)
        at android.app.Activity.performCreate(Activity.java:8595)
        at android.app.Activity.performCreate(Activity.java:8573)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1456)
        at androidx.test.runner.MonitoringInstrumentation.callActivityOnCreate(MonitoringInstrumentation.java:766)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3764)
        ... 12 more
Caused by: android.os.RemoteException: Remote stack trace:
        at com.android.server.am.ActivityManagerService.registerReceiverWithFeature(ActivityManagerService.java:13927)
        at android.app.IActivityManager$Stub.onTransact(IActivityManager.java:2570)
        at com.android.server.am.ActivityManagerService.onTransact(ActivityManagerService.java:2720)
        at android.os.Binder.execTransactInternal(Binder.java:1339)
        at android.os.Binder.execTransact(Binder.java:1275)

david-allison · 2024-07-10T16:10:13Z

Agreed - refactor would be better as a PR

david-allison · 2024-07-10T16:12:43Z

The force push didn't seem to have applied the patch. I'll move the 'snipe' into another PR, so this PR can focus on the refactoring

fix(broadcast-receivers): specify export behavior (API 34) #16717

Arthur-Milchior · 2024-07-10T16:59:42Z

Yeah, I did the force push after seeing your request for import and before seeing that actually, your code worked on all supported version.

Arthur-Milchior · 2024-07-10T17:26:32Z

I'll wait until the urgent PR is merged to solve the merge conflict and do the refactor

david-allison · 2024-08-01T08:13:15Z

For posterity: d90bdc8

david-allison · 2024-08-18T12:20:49Z

One comment, from memory the rest of the code was good and I'll approve once onDestroy is a little cleaner

david-allison

Could you test this:

I want to confirm that moving from one receiver to two won't cause problems and that mounting/unmounting are handled in the DeckPicker

Other than testing, LGTM

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

david-allison · 2024-08-19T13:56:39Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

-            unmountReceiver = object : BroadcastReceiver() {
+    override fun registerExternalStorageListener() {
+        super.registerExternalStorageListener()
+        if (mountReceiver == null) {


nit: invert if and return early

github-actions · 2024-09-03T11:36:42Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

Arthur-Milchior · 2024-09-07T15:45:10Z

I rewrote it so that to refactor the code. There is a map from action name to the actual action to execute , and that's the only part that is open

github-actions · 2024-09-21T16:39:24Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

This ensure that the same receiver appears in all activities and not just the few one where the code was essentially copy pasted. The Deck Picker also listen for mounting, so I renamed its receiver as a `mountReceiver` instead

Arthur-Milchior added Priority-Critical Needs Review labels Jul 10, 2024

This comment was marked as resolved.

# to view

david-allison added Needs Author Reply Waiting for a reply from the original author Review High Priority Request for high priority review labels Jul 10, 2024

Arthur-Milchior force-pushed the receiver branch from d90bdc8 to 77f766b Compare July 10, 2024 13:52

mikehardy approved these changes Jul 10, 2024

View reviewed changes

mikehardy requested a review from david-allison July 10, 2024 15:42

mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author Needs Review labels Jul 10, 2024

mikehardy requested changes Jul 10, 2024

View reviewed changes

mikehardy added Needs Author Reply Waiting for a reply from the original author Needs Review and removed Needs Second Approval Has one approval, one more approval to merge labels Jul 10, 2024

Arthur-Milchior force-pushed the receiver branch from 77f766b to f5e66cc Compare July 10, 2024 16:58

github-actions bot added the Has Conflicts label Jul 18, 2024

david-allison removed Priority-Critical Review High Priority Request for high priority review Needs Review labels Aug 3, 2024

david-allison mentioned this pull request Aug 3, 2024

refactor: Hungarian Notation rename #16818

Merged

This comment was marked as outdated.

# to view

github-actions bot added the Stale label Aug 17, 2024

Arthur-Milchior force-pushed the receiver branch from f5e66cc to 6527bef Compare August 18, 2024 12:08

Arthur-Milchior changed the title ~~NF: Mark receivers are exported~~ Refactor mount receivers to AnkiActivity Aug 18, 2024

github-actions bot removed the Stale label Aug 18, 2024

Arthur-Milchior force-pushed the receiver branch from 6527bef to b71ddcc Compare August 18, 2024 12:48

david-allison changed the title ~~Refactor mount receivers to AnkiActivity~~ refactor: mount receivers Aug 19, 2024

david-allison requested changes Aug 19, 2024

View reviewed changes

github-actions bot added the Stale label Sep 3, 2024

Arthur-Milchior force-pushed the receiver branch from b71ddcc to cf8d770 Compare September 7, 2024 15:44

github-actions bot removed the Stale label Sep 7, 2024

github-actions bot added the Stale label Sep 21, 2024

Arthur-Milchior force-pushed the receiver branch from cf8d770 to 34a2c9f Compare September 21, 2024 18:21

Arthur-Milchior added Needs Review and removed Needs Author Reply Waiting for a reply from the original author Stale Has Conflicts labels Sep 21, 2024

Refactor the unmount listener to anki activity.

38a39d5

This ensure that the same receiver appears in all activities and not just the few one where the code was essentially copy pasted. The Deck Picker also listen for mounting, so I renamed its receiver as a `mountReceiver` instead

Arthur-Milchior force-pushed the receiver branch from 34a2c9f to 38a39d5 Compare September 21, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: mount receivers #16714

refactor: mount receivers #16714

Arthur-Milchior commented Jul 10, 2024 •

edited by david-allison

Loading

This comment was marked as resolved.

Arthur-Milchior commented Jul 10, 2024

david-allison commented Jul 10, 2024

mikehardy left a comment

mikehardy commented Jul 10, 2024

mikehardy left a comment •

edited

Loading

david-allison commented Jul 10, 2024

david-allison commented Jul 10, 2024 •

edited

Loading

Arthur-Milchior commented Jul 10, 2024

Arthur-Milchior commented Jul 10, 2024

david-allison commented Aug 1, 2024

This comment was marked as outdated.

david-allison commented Aug 18, 2024

david-allison left a comment •

edited

Loading

david-allison Aug 19, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

Arthur-Milchior commented Sep 7, 2024

github-actions bot commented Sep 21, 2024

refactor: mount receivers #16714

Are you sure you want to change the base?

refactor: mount receivers #16714

Conversation

Arthur-Milchior commented Jul 10, 2024 • edited by david-allison Loading

This comment was marked as resolved.

Arthur-Milchior commented Jul 10, 2024

david-allison commented Jul 10, 2024

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Jul 10, 2024

mikehardy left a comment • edited Loading

Choose a reason for hiding this comment

david-allison commented Jul 10, 2024

david-allison commented Jul 10, 2024 • edited Loading

Arthur-Milchior commented Jul 10, 2024

Arthur-Milchior commented Jul 10, 2024

david-allison commented Aug 1, 2024

This comment was marked as outdated.

david-allison commented Aug 18, 2024

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

david-allison Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024

Arthur-Milchior commented Sep 7, 2024

github-actions bot commented Sep 21, 2024

Arthur-Milchior commented Jul 10, 2024 •

edited by david-allison

Loading

mikehardy left a comment •

edited

Loading

david-allison commented Jul 10, 2024 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison Aug 19, 2024 •

edited

Loading