Skip to content

Conversation

@bjester
Copy link
Member

@bjester bjester commented Mar 20, 2024

Summary

  • Adds dependency androidx.lifecycle:lifecycle-service:2.7.0 since SystemForegroundService relies on it (possible this was the problematic oversight)
  • Removes some unused imports
  • Adds PythonProvider class which implements AutoCloseable for use with try-with-resources, to provider the android context to the python side, and automatically ensure we don't create context memory leaks
  • Updates ContextUtil to utilize the PythonProvider if it's active
  • Refactors work manager worker logic into abstract base class and makes foreground worker a regular Worker instead of RemoteListenableWorker
  • Re-adds setForegroundAsync that was mistakenly deleted, but does not call it repeatedly-- calling it repeatedly seemed to have no benefit against the issue but to spam the logs
  • Goes back to having a single android notification for tasks, and since foreground tasks require an integer ID, it converts the task request UUID to an integer using CRC32
  • Removes WorkerService which was a RemoteWorkerService for running RemoteListenableWorkers
  • Adds observer pattern to task worker implementation so task update calls go through it, which is a Java object that should only exist in the same task's thread
  • Adds onProgressUpdate hook method to worker base class which is called when the task's progress is updated, and by default calls setProgressAsync and updates the notification (this may be the true fix)
  • Fixes order of operation issue after switching to // division instead of using math.floor(...) changed in Overflow error when updating task progress #204
  • Adds android.permission.POST_NOTIFICATIONS permission and check for it since Android Studio was complaining about it missing (I don't recall this happening previously)

References

Fixes #205

@bjester bjester marked this pull request as ready for review March 20, 2024 17:34
@bjester bjester requested a review from rtibbles March 20, 2024 17:34
Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

APK tested on an Android 11 tablet, 6GBs from the CK12 channel imported without a single hickup in the logcat or UI 👏🏽 💯 :shipit:

logcat task completed
Android11-TabA7.log 2024_03_20_21 21 16

@rtibbles
Copy link
Member

Goes back to having a single android notification for tasks, and since foreground tasks require an integer ID, it converts the task request UUID to an integer using CRC32

Nice!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Thanks to the PR description, I understand this at a high level - although don't feel completely equipped to comment on specific changes. But it also works in manual testing, and feels like a decent amount of cleanup and consolidation too!

implementation 'androidx.concurrent:concurrent-futures:1.1.0'
implementation 'androidx.work:work-runtime:2.9.0'
implementation 'androidx.work:work-multiprocess:2.9.0'
implementation "androidx.lifecycle:lifecycle-service:2.7.0"
Copy link
Member

Choose a reason for hiding this comment

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

Did Android Studio warn you about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It did not! 🤦 Besides highlighting the inherited classes as red (missing), which still required you to click through and look at the class itself.

# PROGRESS_LIMIT gives sufficient precision for a % progress calculation
if total_progress > PROGRESS_LIMIT:
progress = progress // total_progress * PROGRESS_LIMIT
progress = PROGRESS_LIMIT * progress // total_progress
Copy link
Member

Choose a reason for hiding this comment

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

Oh yes... the previous version would always go to 0... I guess the math didn't check out!

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally had math.floor(...) and tested it with that, then I thought "oh yeah there's integer division // which is simpler" (self-sabotage)

@rtibbles rtibbles merged commit cc4e25f into learningequality:develop Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Long running tasks are cancelled after 10 minutes

3 participants