bkonyi / FlutterGeofencing

Rough work for Flutter geofencing plugin
BSD 3-Clause "New" or "Revised" License
338 stars 220 forks source link

Android fixes #25

Closed JFreakDK closed 4 years ago

JFreakDK commented 4 years ago

There are a few fixes here:

bkonyi commented 4 years ago

Awesome, thanks for the cleanup! A couple of questions:

JFreakDK commented 4 years ago

Why do we want to explicitly execute calls to the background channel on the main thread? Is there a performance reason?

I saw this exception on https://github.com/KarimAbdo/FlutterGeofencing:

E/AndroidRuntime(10771): Caused by: java.lang.RuntimeException: Methods marked with @UiThread must be executed on the main thread. Current thread: AsyncTask #1
E/AndroidRuntime(10771):    at io.flutter.embedding.engine.FlutterJNI.ensureRunningOnMainThread(FlutterJNI.java:807)
E/AndroidRuntime(10771):    at io.flutter.embedding.engine.FlutterJNI.dispatchPlatformMessage(FlutterJNI.java:697)
E/AndroidRuntime(10771):    at io.flutter.embedding.engine.dart.DartMessenger.send(DartMessenger.java:80)
E/AndroidRuntime(10771):    at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:187)
E/AndroidRuntime(10771):    at io.flutter.view.FlutterNativeView.send(FlutterNativeView.java:128)
E/AndroidRuntime(10771):    at io.flutter.plugin.common.MethodChannel.invokeMethod(MethodChannel.java:98)
E/AndroidRuntime(10771):    at io.flutter.plugin.common.MethodChannel.invokeMethod(MethodChannel.java:84)
E/AndroidRuntime(10771):    at io.flutter.plugins.geofencing.GeofencingService.onHandleWork(GeofencingService.kt:147)
E/AndroidRuntime(10771):    at androidx.core.app.JobIntentService$CommandProcessor.doInBackground(JobIntentService.java:392)
E/AndroidRuntime(10771):    at androidx.core.app.JobIntentService$CommandProcessor.doInBackground(JobIntentService.java:383)
E/AndroidRuntime(10771):    at android.os.AsyncTask$3.call(AsyncTask.java:378)
E/AndroidRuntime(10771):    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
E/AndroidRuntime(10771):    ... 3 more

I can see that this is not an issue on your project. So I removed that change again.

I see you've published this on pub. While I wasn't planning on publishing this as a production quality plugin, clearly there's demand for it so I should check with the plugins team to see if we want to support it officially. Would you mind adding me as an uploader / admin for the project in the meantime?

I am not the one who published it. From what I can see this: https://pub.dev/packages/geofencing was published by axella.gerald@gmail.com. Furthermore the fork from https://github.com/KarimAbdo/FlutterGeofencing seems to be published as well: https://pub.dev/packages/flutter_geofencing

I have reverted a few extra minor things to minimize the diff.

JFreakDK commented 4 years ago

I tried registering a geofence again. After a little time I got the exception below. I will add the change that executes on the main thread again:

E/AndroidRuntime(12533): Process: io.flutter.plugins.geofencingexample, PID: 12533
E/AndroidRuntime(12533): java.lang.RuntimeException: An error occurred while executing doInBackground()
E/AndroidRuntime(12533):    at android.os.AsyncTask$4.done(AsyncTask.java:399)
E/AndroidRuntime(12533):    at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:383)
E/AndroidRuntime(12533):    at java.util.concurrent.FutureTask.setException(FutureTask.java:252)
E/AndroidRuntime(12533):    at java.util.concurrent.FutureTask.run(FutureTask.java:271)
E/AndroidRuntime(12533):    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
E/AndroidRuntime(12533):    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
E/AndroidRuntime(12533):    at java.lang.Thread.run(Thread.java:919)
E/AndroidRuntime(12533): Caused by: java.lang.RuntimeException: Methods marked with @UiThread must be executed on the main thread. Current thread: AsyncTask #1
E/AndroidRuntime(12533):    at io.flutter.embedding.engine.FlutterJNI.ensureRunningOnMainThread(FlutterJNI.java:807)
bkonyi commented 4 years ago

Why do we want to explicitly execute calls to the background channel on the main thread? Is there a performance reason?

I saw this exception on https://github.com/KarimAbdo/FlutterGeofencing:

E/AndroidRuntime(10771): Caused by: java.lang.RuntimeException: Methods marked with @UiThread must be executed on the main thread. Current thread: AsyncTask #1
E/AndroidRuntime(10771):  at io.flutter.embedding.engine.FlutterJNI.ensureRunningOnMainThread(FlutterJNI.java:807)
E/AndroidRuntime(10771):  at io.flutter.embedding.engine.FlutterJNI.dispatchPlatformMessage(FlutterJNI.java:697)
E/AndroidRuntime(10771):  at io.flutter.embedding.engine.dart.DartMessenger.send(DartMessenger.java:80)
E/AndroidRuntime(10771):  at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:187)
E/AndroidRuntime(10771):  at io.flutter.view.FlutterNativeView.send(FlutterNativeView.java:128)
E/AndroidRuntime(10771):  at io.flutter.plugin.common.MethodChannel.invokeMethod(MethodChannel.java:98)
E/AndroidRuntime(10771):  at io.flutter.plugin.common.MethodChannel.invokeMethod(MethodChannel.java:84)
E/AndroidRuntime(10771):  at io.flutter.plugins.geofencing.GeofencingService.onHandleWork(GeofencingService.kt:147)
E/AndroidRuntime(10771):  at androidx.core.app.JobIntentService$CommandProcessor.doInBackground(JobIntentService.java:392)
E/AndroidRuntime(10771):  at androidx.core.app.JobIntentService$CommandProcessor.doInBackground(JobIntentService.java:383)
E/AndroidRuntime(10771):  at android.os.AsyncTask$3.call(AsyncTask.java:378)
E/AndroidRuntime(10771):  at java.util.concurrent.FutureTask.run(FutureTask.java:266)
E/AndroidRuntime(10771):  ... 3 more

I can see that this is not an issue on your project. So I removed that change again.

That's interesting, I'm surprised I've never run into that myself (unless it's happening and it's only being logged without bringing up a crash dialog). It looks like you're right though as my colleague made a similar change to the android_alarm_manager plugin that I maintain, which also utilizes background execution (see here).

I see you've published this on pub. While I wasn't planning on publishing this as a production quality plugin, clearly there's demand for it so I should check with the plugins team to see if we want to support it officially. Would you mind adding me as an uploader / admin for the project in the meantime?

I am not the one who published it. From what I can see this: https://pub.dev/packages/geofencing was published by axella.gerald@gmail.com. Furthermore the fork from https://github.com/KarimAbdo/FlutterGeofencing seems to be published as well: https://pub.dev/packages/flutter_geofencing

I have reverted a few extra minor things to minimize the diff.

Ah, sorry about the confusion. GitHub names don't always match to Pub developer emails so I just made a bad assumption :-).