Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setOnBackgroundMessage() is now a Future<void> that will await for the function to complete. #498

Closed
wants to merge 8 commits into from

Conversation

asoap
Copy link
Contributor

@asoap asoap commented Nov 30, 2023

This is in regards to this issue here:
#496

If a setOnBackgroundMessage() was fired and if it took to long to complete the isolate on Android would end before the code was completed. This was extra annoying if you were trying to debug a background messsage or if it used other packages that used async/await. Those items just wouldn't fire.

Now all code will complete before the isolate dismisses itself.

I made the call to the onPushBackgroundMessage await for the handler to complete.
I changed handleBackgroundMessage() to become a Future<void> and async/await.
I made the BackgroundMessageHandler a Future<void> instead of void so that we can async/await for it to be completed.
@asoap
Copy link
Contributor Author

asoap commented Dec 1, 2023

Do I need to go and fix these errors that have popped up? There is an AWS error for credentials, I'm not sure how I'm supposed to get past that for a github test.

Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Thank you very much for this pull request. We cannot merge it right now because it appears to introduce breaking changes. However, we are happy to work on this together.

@ttypic
Copy link
Contributor

ttypic commented Dec 5, 2023

Do I need to go and fix these errors that have popped up? There is an AWS error for credentials, I'm not sure how I'm supposed to get past that for a github test.

No need, this is because actions run on forked repository and it doesn't have access to secrets

asoap added 2 commits December 5, 2023 14:24
handleBackgroundMessage now handles when _onBackgroundMessage is defined as void or Future<void>  It checks to see if it's a Future and then awaits it if it is.
Changed the cast on BackgroundMessageHandler to dynamic, this allows it to be defined as both void and Future<void>
@github-actions github-actions bot temporarily deployed to staging/pull/499/features December 6, 2023 16:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/499/dartdoc December 6, 2023 16:15 Inactive
Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Thank you very much for updated version, looks good! But could you please run flutter format . there are some extra whitespaces and linter doesn't like it. Also I am not sure we need to do any changes in lib/src/platform/src/background_android_isolate_platform.dart

@asoap
Copy link
Contributor Author

asoap commented Dec 6, 2023

Thank you very much for updated version, looks good! But could you please run flutter format . there are some extra whitespaces and linter doesn't like it. Also I am not sure we need to do any changes in lib/src/platform/src/background_android_isolate_platform.dart

I added a comment above. I'm fairly sure that change is required. I can test without it if you like to confirm.

Also I'm a bit confused on how to run flutter format. Obviously the command line, but for these changes I made them locally in my installed packages for flutter, and I made a fork and changed them online. I guess I could pull my repo locally, run the command and then push it. But I'm not sure if that would work. Working on packages is all new to me.

@ttypic
Copy link
Contributor

ttypic commented Dec 6, 2023

@asoap

I added a comment above. I'm fairly sure that change is required. I can test without it if you like to confirm.

It will be lovely if you test, but I am pretty sure that nothing will change if you rollback changes from lib/src/platform/src/background_android_isolate_platform.dart

I guess I could pull my repo locally, run the command and then push it

Yeah, this is exactly what I meant, it will work, just push changes in main branch on your fork.

Working on packages is all new to me.

Nice, my congratulations! :) If you have any questions we are happy to help

@asoap
Copy link
Contributor Author

asoap commented Dec 6, 2023

I pushed the changes to my repo. Also I was confused looking for the command "flutter format ." It turns out it was "dart format ."

I think everything should be good to go now.

@github-actions github-actions bot temporarily deployed to staging/pull/499/features December 6, 2023 18:47 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/499/dartdoc December 6, 2023 18:47 Inactive
@asoap
Copy link
Contributor Author

asoap commented Dec 6, 2023

Huzzah! Is this done now? Or do I need to do anything to resolve the conversation?

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

lgtm

@ttypic
Copy link
Contributor

ttypic commented Dec 8, 2023

@asoap Sorry, still some checks are not passing, so I can't merge it, could you add those changes:

lib/src/platform/src/push_notification_events_internal.dart

image

lib/src/platform/src/method_call_handler.dart

image

@asoap
Copy link
Contributor Author

asoap commented Dec 8, 2023

Can do. I'm going to be in the middle of some work, but I will try to address this today and test it out to make sure it works before submitting changes.

@asoap
Copy link
Contributor Author

asoap commented Dec 8, 2023

Unfortuantely work went long today, and I'll have to address this on Monday.

@asoap
Copy link
Contributor Author

asoap commented Dec 11, 2023

I tested these changes and everything is working as expected. I then pushed them to the pull request.

@github-actions github-actions bot temporarily deployed to staging/pull/499/features December 11, 2023 23:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/499/dartdoc December 11, 2023 23:23 Inactive
Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Hello @asoap I'm sorry but formatting checks are still failing and I can't merge. Do you mind if I close the PR and add required changes in new internal PR?

@asoap
Copy link
Contributor Author

asoap commented Dec 12, 2023

Hello @asoap I'm sorry but formatting checks are still failing and I can't merge. Do you mind if I close the PR and add required changes in new internal PR?

Yeah absolutely, do what you need to do.

@ttypic
Copy link
Contributor

ttypic commented Dec 13, 2023

moved to #499

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

Successfully merging this pull request may close these issues.

3 participants