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

Work manager integration #119

Merged
merged 39 commits into from
Apr 17, 2023
Merged

Work manager integration #119

merged 39 commits into from
Apr 17, 2023

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 9, 2022

  • Updates P4A to version that includes @dbnicholson's WorkManager integration
  • Hooks into the Kolibri Storage class to trigger WorkManager enqueueing when tasks are scheduled
  • Hooks into Kolibri Storage class to handle updates to reschedule repeating tasks that are not indefinitely repeating - as Android does not seem to support limited repeating tasks

Fixes #103
Fixes #104
Fixes #105

Depends on learningequality/kolibri#9503 to expose the hooks and updates to the task machinery

Opening this as a WIP, as I have not actually tested any of this code as yet.

@rtibbles rtibbles force-pushed the work_manager_integration branch from 19cbe3c to 119c20a Compare February 10, 2023 21:44
@rtibbles rtibbles force-pushed the work_manager_integration branch from 119c20a to 8d00eca Compare February 25, 2023 03:16
@rtibbles rtibbles force-pushed the work_manager_integration branch from 135c2a3 to f2db879 Compare March 22, 2023 22:24
@rtibbles rtibbles marked this pull request as ready for review March 22, 2023 22:25
@rtibbles rtibbles requested a review from jamalex March 22, 2023 22:25
Copy link
Member

@jamalex jamalex left a comment

Choose a reason for hiding this comment

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

Submitting some in progress comments

Makefile Outdated
@@ -44,7 +44,7 @@ needs-android-dirs:

# Clear out apks
clean:
- rm -rf dist/*.apk src/kolibri tmpenv
- rm -rf dist/*.apk src/kolibri tmpenv src/*.pyc
Copy link
Member

Choose a reason for hiding this comment

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

-rf is recursive when you delete a folder itself, but I don't think this will recursively delete from subfolders with the wildcards like that, so it may be good to adapt to be safer.

Confirmed:
image

@@ -72,9 +72,25 @@ src/kolibri: clean
# patch Django to allow migrations to be pyc files, as p4a compiles and deletes the originals
sed -i 's/if name.endswith(".py"):/if name.endswith(".py") or name.endswith(".pyc"):/g' src/kolibri/dist/django/db/migrations/loader.py

.PHONY: check-android-clean
check-android-clean:
@git diff --quiet --exit-code python-for-android || (echo "python-for-android directory has uncommitted changes in the working tree" && exit 1)
Copy link
Member

Choose a reason for hiding this comment

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

Is this just displayed as a warning? Or does the exit 1 make it bail, I guess? Would one not want to be able to build while the P4A is still being iterated.

Probably I'm also misinterpreting these new make targets. Could you add a one-line comment on each as to what it does?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can build while the Java files are being actively edited - we just can't reimport from the bootstrap while that is the case. Will add some comments to make it clearer.

@@ -33,14 +33,25 @@ Follow the instructions from the command to set the additional environment varia

Copy link
Member

Choose a reason for hiding this comment

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

Worth above these pieces suggesting a venv?

@rtibbles
Copy link
Member Author

Comments from review have been addressed. A follow up issue for the remaining fix for task running is here: #132

@rtibbles rtibbles merged commit 97d6c80 into develop Apr 17, 2023
@rtibbles rtibbles deleted the work_manager_integration branch April 17, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants