-
Notifications
You must be signed in to change notification settings - Fork 131
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
Support service processes #69
Conversation
Thanks, but I would prefer to keep the library written in Java. Believe me, I love Kotlin as much as the next person, but this library doesn't really need it and I'd rather not bother with it at this point. Your change is the biggest the library has seen in like a decade, and I doubt anyone will add anything for another decade once this lands. |
Alrighty, Java it is :) Any feedback? |
I didn't review the implementation, but will today. |
Cool, no hurry. Let me know if anything needs to change. |
process-phoenix/src/main/java/com/jakewharton/processphoenix/PhoenixActivity.java
Outdated
Show resolved
Hide resolved
process-phoenix/src/main/java/com/jakewharton/processphoenix/PhoenixService.java
Outdated
Show resolved
Hide resolved
* <p> | ||
* Behavior of the current process after invoking this method is undefined. | ||
*/ | ||
public static void triggerRebirth(Context context, Class<?> targetClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like this from an API design standpoint. I cannot imagine the case where you have either an activity or a service and need to call some general-purpose method to recreate it. Chances are you know what you have at the call-site. Someone that actually doesn't know can write the assignable check themselves.
If we want to offer Class
-accepting methods then we can overload the specific methods with Class<? extends Activity>
and Class<? extends Service>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I based this on your input here: #24 (comment)
But, I do agree :) The caller should know whether an Activity or Service is to be restarted when they call this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahaha ouch. Bad 7-year-ago Jake!
sample/src/main/java/com/jakewharton/processphoenix/sample/NotificationBuilder.java
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/jakewharton/processphoenix/sample/RestartService.java
Outdated
Show resolved
Hide resolved
sample/src/main/java/com/jakewharton/processphoenix/sample/RestartService.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jake Wharton <[email protected]>
# Conflicts: # process-phoenix/src/main/java/com/jakewharton/processphoenix/ProcessPhoenix.java # sample/src/main/java/com/jakewharton/processphoenix/sample/MainActivity.java
@JakeWharton The triggerRebirth(Context context, Intent... nextIntents) method already existed in previous versions, but it also added an Activity vs Service check in this pull request. I've now removed these checks and opted to keep the previously existing methods Activity-only and add methods like triggerServiceRebirth(Context context, Intent nextIntent) for Service restarting. Would this be ok? |
b6f9097
to
15440fd
Compare
Fixes #24
What was done per commit:
The ProcessPhoenix class is now Kotlin, but all public functions are annotated with @JvmStatic.
If this is undesired I can implement it in Java, but since Kotlin is my preference I started like this.
I changed from Service to IntentService (in commit 4) because this also seemed to be functional and IntentService's are meant to be short-lived, which the PhoenixService certainly is.
The sample has been updated to allow a easy test of this functionality.
One important side-node (which is also mentioned in the Service classes):
Restarting a Service multiple times will result in an increasingly long delay between restart times. Android seems to register the restart of this service as a crashed service and probably doesn't want to infinitely restart a crashing service, so that makes sense.
The increasing delay periods seem to follow the pattern: 4^x, with x being the restart attempt minus 1. I observed 1s, 4s, 16s, 64s, 256s and 1024s on the Zebra Android 11 device i used to test.