Run all manual operations in a worker #268
Conversation
|
This PR was way too big. I've broken down the changes into a bunch of much smaller (individually compilable, but not necessarily fully complete) commits. Hopefully the smaller commits makes this more reviewable. Thanks! |
|
Hello, I haven't had a chance to review the code changes and test, but this looks fantastic! I've been toying with doing something like this for a while, but was dreading the work involved, and some of what you've done is beyond what I'd have thought to do. Thanks so much! I'm really looking forward to merging this, once I've done my due diligence :) |
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
An upcoming commit will move manual operations to the worker, which includes imports too. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
An upcoming commit will add a new tag value for manual operations. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This leaves the worker only responsible for scheduling and status reporting. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
An upcoming commit will add another scheduling function for manual operations. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
The call log does not make use of the SMS/MMS distinction in the previous MessageTotal return type. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This is the first step in decoupling the UI from the operations. An upcoming commit will move the invocation of each operation to the worker and report progress through the WorkInfo API. Note that this commit temporarily breaks the functionality for hiding the progress bar after an operation is complete. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
… function This also prepares the infrastructure for allowing notification updates with the operation's progress. These updates are throttled to one per second to avoid hitting Android's notification rate limit, which if hit, prevents showing further notifications. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
The new messages stored in the Result instance will now be used to fill in the completion notification text. In an upcoming commit, these messages will also be used to populate the status text in MainActivity. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
… immediately Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
By default, if no arguments are provided to the worker, it will perform a scheduled export. Otherwise, the action argument determines which (manual) action to perform. Representing the scheduled export by the lack of an action argument is done to avoid needing to reschedule the next scheduled run with a new WorkRequest when upgrading from a previous version of sms-ie. By keeping the scheduled WorkRequest as simple as possible, future refactoring is also simpler. Aside from the action argument, there's also a file argument for the manual import and export (but not wipe) actions. This is just the URI that the user selected. All of these operations use the previously introduced infrastructure for reporting progress and success/failure entirely through the androidx work APIs. This commit also adds new strings so that we have text for the in-progress and failure states of every operation. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This is no longer used with the new unified way of reporting errors for every operation. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This is obsoleted by the new action logging in ImportExportWorker. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This way, errors of any kind will be funneled through the work Result where it can be used for a notification or observed by MainActivity (in an upcoming commit). Some of these errors were intentionally meant to be user-facing. For these errors, a new UserFriendlyException has been introduced. When this is thrown, the error message will include both the friendly error message and the first cause of the exception, if one exists. This results in the same error messages as we do now. With this change, none of the operations need the Context to be an Activity context anymore, since they don't directly launch an AlertDialog. The dialog handling will be readded in an upcoming commit when MainActivity switches over to using the worker for manual operations. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
No code changes here. Just removing no-op catch block and deindenting everything. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Since many of the actions can conflict with one another, we'll prevent them from running at the same time. Once MainActivity switches over to using the worker, the user will no longer be able to start a conflicting manual operation while the scheduled export is running. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
ActivityResultContracts is Android's new preferred API for requesting permissions and starting activities for obtaining a result. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This also replaces the previous method of storing `operation` so that we store an enum value that's persistent in the Activity state instead. This removes one obstacle that prevents the Activity from being safely recreated without resorting to android:configChanges. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This replaces the last of the deprecated startActivityForResult() calls in MainActivity. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
The activity will now observe the workers and update the progress bar and status text based on their state. With this change, MainActivity is now also able to show the state of the scheduled export while it is running. This commit doesn't make MainActivity launch the worker for manual operations yet. That will be done in an upcoming commit. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
By default, TextView does not preserve the text when the activity is recreated. This allows it to do so, removing the final barrier to getting rid of android:configChanges and letting MainActivity get recreated seamlessly. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This is the final step is decoupling the activity from the operations and using the worker. In a previous commit, MainActivity has already been updated to observe the state of the workers, so MainActivity can now detach and reattach to the workers without impacting the actual operations. The user can even swipe away the activity. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
There are no more blockers to allowing the Activity to get recreated. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This restores the old behavior that was temporarily removed when switching to using the worker for manual operations. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
Although ImportExportWorker already has a global lock to prevent conflicting operations from running at the same time, this makes it obvious to the user that they shouldn't even try to do so. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This isn't strictly required, but is useful so that the user can see the progress of operations when they're not in the app. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
|
This looks amazing - thank you so much! I didn't follow every single thing you've done, but I got much / most of it, and you've documented it well. One suggestion: under the old behavior, leaving the app would cancel an ongoing (manual) operation. That might be seen as a bug, but it was also sort of a feature, since it provided a simple way to cancel operations. Under the new behavior (which is certainly a major improvement in general), there seems to be no way to cancel an operation. I think we need to add some sort of cancel button - you can add it to the PR, or I can add one. Thoughts? Thanks again. Edit: Another couple of points:
|
|
Thanks for reviewing the PR!
That's a good point. I think it should be relatively simple to do this via the worker API. I'll look into this after work today.
Yep, that's basically it. We could easily have 3 separate rwlocks for contacts, call logs, and messages to allow every kind of non-conflicting operation to run simultaneously. For progress reporting via notifications, it should be very easy since the androidx work library allows multiple notifications to be shown (https://android-review.googlesource.com/c/platform/frameworks/support/+/1181216). For MainActivity, I think we'd have to stop defining the status text box and progress bar in the XML and dynamically create them for each running task (and probably add a cancel button for each one too). I'm happy to implement this if you'd like. |
I'm really not sure it's necessary or worth your time. I only brought it up to make sure that I understood how things currently stood. You can certainly do it if you want, but I don't know whether there would really be any interest in having this functionality. Thanks again for being so helpful and gracious. |
This uses the androidx work library's builtin mechanism for signaling the cancellation of the worker. Each operation will periodically check if it should stop running. This just requires an ensureActive() call if the function does not frequently call any other suspend function. Since cancellation is just throwing a CancellationException, the cleanup is orderly (eg. all finally blocks will run). The only operation that cannot be cancelled is the message wiping operation. That uses ContentResolver.delete(), which has no variant that accepts a CancellationSignal. The only way to interrupt that is to kill the app, so cancellation is blocked for this operation instead. Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
|
No problem! I just pushed 727d98e to implement cancellation. Cancellation can be done via a new button in the notification or in MainActivity.
If that's the case, I'd prefer to just keep it simple for now and leave things the way they are. |
Thanks much! Once again, I'm (unpleasantly) amazed at how much code and how many workarounds are necessary to implement something as simple as a cancellation button :|
I think that's the right call. I'm about to merge the PR, and I'll try to do a release within a few days. |
This fixes the device playing the notification source or vibrating every time the notification is updated (for progress bar changes). I didn't catch this when implementing tmo1#268 because I had a Pixel Watch connected, which always forces the `setOnlyAlertOnce(true)` behavior. Additionally, this changes the default importance of the background services notifications to low (referred to as "Silent" in Android's UI). Even with the original bug fixed, it doesn't make much sense to play a sound or vibrate one time when an operation starts. Fixes: tmo1#276 Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
This fixes the device playing the notification source or vibrating every time the notification is updated (for progress bar changes). I didn't catch this when implementing tmo1#268 because I had a Pixel Watch connected, which always forces the `setOnlyAlertOnce(true)` behavior. Additionally, this changes the default importance of the background services notifications to low (referred to as "Silent" in Android's UI). Even with the original bug fixed, it doesn't make much sense to play a sound or vibrate one time when an operation starts. This change in defaults only takes effect for new installs. It is not possible to change existing notification channel settings. Fixes: tmo1#276 Signed-off-by: Andrew Gunnerson <accounts+github@chiller3.com>
(Apologies for the big PR!
I can try to break this into smaller pieces if needed.EDIT: Done! Also happy to completely change the approach here if desired.)The main change in this commit is to perform all import, export, and wipe operations inside of the worker. Instead of having each operation update the progress bar and status text directly, the progress, success, and failure reporting is now all done through the androidx work APIs. This completely decouples the UI from the operation, which allows the activity to be detached and reattached to the worker as needed.
This makes it possible for
MainActivityto be placed in the background, rotated withoutandroid:configChanges, or get interrupted in any other way without impacting the operations. This is most useful for doing a large import that takes several minutes while using other apps on the device.A nice side effect of having both manual operations and scheduled exports handled the same way in the worker is that the progress reporting is now unified. If the user opens the app while the scheduled export is running, it will show the status of the scheduled export. This also makes it simple to have a lock that prevents multiple operations from running at the same time and possibly conflicting with each other.
Some more details:
ImportExport.ktso that the worker is only concerned with scheduling, progress reporting, and result handling.MainActivityare now disabled while an operation is running. Although the global mutex already prevents two operations from running at the same time, this hopefully makes the behavior clearer to the user.MainActivitywill watch the worker status and open a dialog if the result indicates a failure. This removes the need for the import/export operations to hold a reference to anActivityContext.MainActivityshows. However, unlikeMainActivity, the reporting rate is throttled to 1 per second to ensure that Android doesn't rate limit the app and block it from showing further notifications.FOREGROUND_SERVICE_IMMEDIATEflag is set to make the notification appear immediately, instead of after a potential 10 second delay.MainActivityand notifications.android:configChangessinceMainActivitycan detach and reattach to the worker now.startActivityForResult()calls inMainActivityhave been replaced with the newActivityResultContractsAPI.ExportService.PDU_HEADERS_FROMandMessageTotalfromMainActivitytoImportExportMessages.kt.