Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Updated android_alarm_manager to work after engine refactor. #642

Merged
merged 6 commits into from
Aug 7, 2018

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Jun 28, 2018

Fixes issue #17566: alarm_manager plugin stopped working

@bkonyi
Copy link
Contributor Author

bkonyi commented Jun 28, 2018

Depends on engine PR #5640.

Copy link

@marcmo marcmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm w/ more comments since this example will teach others how to write these plugins for Android.

const String _backgroundName =
'plugins.flutter.io/android_alarm_manager_background';

void _alarmManagerCallbackDispatcher() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that this is the entrypoint for the background isolate

sStarted.set(true);
}

public static void startAlarmService(Context context, long callbackHandle) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments about what is going on here will probably be helpful to 3p developers trying to write their own plugins.

@bkonyi bkonyi merged commit e351a09 into master Aug 7, 2018
@bkonyi bkonyi deleted the fixIssue17566 branch August 7, 2018 23:26
@ethael
Copy link

ethael commented Sep 10, 2018

Hello @bkonyi ,

Some hints regarding documentation at: https://pub.dartlang.org/packages/android_alarm_manager

  1. in "getting started" section, you are missing: await AndroidAlarmManager.initialize();

  2. maybe it is only my lack of dart knowledge, but i struggled a bit when I tried to move working example code to a separate class. After code migration, timer resulted with error, because callback function was null. maybe it would be nice to put into readme, that "because every execution runs in separate dart2 isolate, callback function declaration can not be anonymous and also needs to be static or globally available (e.g. declared outside of any class), otherwise it will not be available in the moment of timer execution"

  3. you can cancel specific timers with: AndroidAlarmManager.cancel(TIMER_ID);

  4. "Flutter Hot Restart" feature does not work when AndroidAlarmManager is running in the app. it hangs for a while and then ends up with an error

thanks a lot again for landing this feature into flutter. this plugin api probably could be used for ios implementation too using ios background.fetch mode. i am not an ios developer, but based on flutter/flutter#3671 (comment) it is doable and already functioning in reactnative and nativescript.

startService((JSONArray) arguments);
result.success(true);
} else if (method.equals("AlarmService.initialized")) {
AlarmService.onInitialized();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkonyi I think we're missing the invocation of the result callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like it. I don't think it causes any problems though, does it? I don't believe the background isolate cares if it gets a response here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going through the fun task of auditing all of our invokeMethod calls as part of reviewing #1365 and was confused when looking at this.

I'm guessing it will result in the future here never completing:
https://github.com/flutter/flutter/blob/ecfdd7e1ea96be3c762af6a1bd6be63d9e4d9682/packages/flutter/lib/src/services/platform_channel.dart#L295

Probably best to clean it up anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants