Notification handling and work manager tweaks #177

bjester · 2023-11-30T15:26:34Z

Summary

Reverts Notification tweaks #175
Adds a new notification channel, separating the foreground service notification and the task notifications
Updates the foreground service notification to be completely separate with static int ID
Updates task notifications to use a static int ID and a tag matching the work request ID to maintain notifications for each
Creates an App class to define configuration for WorkManager, including a fixed thread pool executor, and moves notification channel creation to it
Disables automatic initialization of WorkManager so we only initialize it in our foreground service process
Ensures we use RemoteWorkManager when queuing / checking work requests (ref)
Updates worker to use tags for noting long-running tasks
Creates Notifier interface, NotificationRef and NotificationBuilder classes for utility in handling notifications
Updates our Worker implementation to function more like the standard Worker which opens a background thread for processing automatically (ref)
Adds button to foreground service notification that takes user to settings so they can minimize it
Adds ticker text to notifications used by accessibility services

This reverts commit fac73e5, reversing changes made to 5cac903.

…ing RemoteListenableWorker

bjester

@rtibbles some comments. I think I want to clean up a few things but could be done in followup. Fine with either!

bjester · 2023-12-06T22:42:42Z

python-for-android/dists/kolibri/src/main/AndroidManifest.xml

@@ -24,7 +24,7 @@
    <application android:label="@string/app_name"
                 android:icon="@mipmap/icon"
                 android:allowBackup="true"
-
+                 android:name=".App"


@rtibbles This is for defining the main Application class. I wasn't sure if this would cause any problems with python4android, but it doesn't seem like it.

bjester · 2023-12-06T22:45:26Z

python-for-android/dists/kolibri/src/main/java/org/kivy/android/PythonWorker.java

-
-            boolean longRunning = getInputData().getBoolean(ARGUMENT_LONG_RUNNING, false);
+    public boolean isLongRunning() {
+        return getTags().contains(TAG_LONG_RUNNING);


@rtibbles I made this a tag instead because I think there might be a way to listen on the bind/unbind of these task workers to the service process, perhaps to handle notifications or some other service process behavior, and tags seem a little more straightforward to check against.

bjester · 2023-12-06T22:46:50Z

python-for-android/dists/kolibri/src/main/java/org/kivy/android/PythonWorker.java

-                @Override
-                public void run() {
-                    Log.d(TAG, "Running with python worker argument: " + serviceArg);
+        Log.d(TAG, id + " Running with python worker argument: " + arg);


I think I want to better format these log messages using the request ID. I was reading on Java's String interpolation

Yeah, the logging in general leaves a lot to be desired, but you've made it no worse!

bjester · 2023-12-06T22:48:15Z

python-for-android/dists/kolibri/src/main/java/org/kivy/android/PythonWorker.java

                    } else {
-                        completer.set(Result.failure());
+                        Log.w(TAG, id + " Exception in remote python work, scheduling retry", e);
+                        future.set(Result.retry());


@rtibbles should I just get rid of this retry behavior since we're not even sure it works?

Might make things simpler - and I had kind of assumed that WorkManager should be handling this with its retry behaviour also?

bjester · 2023-12-06T22:49:19Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/ContextUtil.java

+
+    public static boolean isWorkerContext() {
+        return PythonWorker.mWorker != null;
+    }


I was at one point going to use these externally but didn't

bjester · 2023-12-06T22:56:16Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/NotificationBuilder.java

+                        R.drawable.baseline_notifications_paused_24,
+                        context.getString(R.string.notification_service_channel_action),
+                        PendingIntent.getActivity(context, 0, intent, PendingIntent.FLAG_IMMUTABLE)
+                ).build());


This adds that Manage action button to the sticky service notification

bjester · 2023-12-06T22:57:19Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/NotificationRef.java

+            return;
+        }
+        ID_CHANNEL_DEFAULT = context.getString(R.string.notification_default_channel_id);
+        ID_CHANNEL_SERVICE = context.getString(R.string.notification_service_channel_id);


@rtibbles I saw some code examples where these were just static variables. Maybe we don't need them in that string configs and these references can be simplified?

Yeah, I was also a little confused why the examples I was seeing drew from the resource manifest - I guess the main thing is to have them be referenceable in other files.

bjester · 2023-12-06T22:58:28Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/Task.java

+        RemoteWorkManager workManager = RemoteWorkManager.getInstance(context);
+        WorkQuery workQuery = WorkQuery.Builder
+                .fromTags(tags)
+                .build();


The RemoteWorkManager only has getWorkInfos so I had to use the WorkQuery here

bjester · 2023-12-06T22:59:08Z

python-for-android/dists/kolibri/src/main/res/drawable/baseline_notifications_paused_24.xml

+    android:viewportHeight="24" android:viewportWidth="24"
+    android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android">
+    <path android:fillColor="@android:color/white" android:pathData="M12,22c1.1,0 2,-0.9 2,-2h-4c0,1.1 0.89,2 2,2zM18,16v-5c0,-3.07 -1.64,-5.64 -4.5,-6.32L13.5,4c0,-0.83 -0.67,-1.5 -1.5,-1.5s-1.5,0.67 -1.5,1.5v0.68C7.63,5.36 6,7.93 6,11v5l-2,2v1h16v-1l-2,-2zM14.5,9.8l-2.8,3.4h2.8L14.5,15h-5v-1.8l2.8,-3.4L9.5,9.8L9.5,8h5v1.8z"/>
+</vector>


This is a bell icon with snooze 'z'. It doesn't seem to show anywhere 🤷‍♂️

Hrm, it's meant to show up in the notification intent launch though, right?

Yes, and the API requires it but I never saw it show

src/android_app_plugin/kolibri_plugin.py

rtibbles

No real concerns, thanks for the walk through!

rtibbles · 2023-12-06T23:16:50Z

python-for-android/dists/kolibri/src/main/java/org/kivy/android/PythonWorker.java

-                @Override
-                public void run() {
-                    Log.d(TAG, "Running with python worker argument: " + serviceArg);
+        Log.d(TAG, id + " Running with python worker argument: " + arg);


Yeah, the logging in general leaves a lot to be desired, but you've made it no worse!

rtibbles · 2023-12-06T23:18:05Z

python-for-android/dists/kolibri/src/main/java/org/kivy/android/PythonWorker.java

                    } else {
-                        completer.set(Result.failure());
+                        Log.w(TAG, id + " Exception in remote python work, scheduling retry", e);
+                        future.set(Result.retry());


Might make things simpler - and I had kind of assumed that WorkManager should be handling this with its retry behaviour also?

rtibbles · 2023-12-06T23:19:10Z

...n-for-android/dists/kolibri/src/main/java/org/learningequality/Kolibri/TaskworkerWorker.java

+        if (mWorker != null) {
+            // We could also update progress on the worker here, if we need info about it on
+            // the Android side
+            // @see setProgressAsync


Yeah - I guess the only thing would be if we wanted to handle the progress updating of the notification via Java rather than doing send notification here.

rtibbles · 2023-12-06T23:21:15Z

...ndroid/dists/kolibri/src/main/java/org/learningequality/Kolibri/TaskworkerWorkerService.java

        PythonUtil.loadLibraries(
                new File(context.getApplicationInfo().nativeLibraryDir)
        );
+        // Initialize the work manager
+        WorkManager.getInstance(getApplicationContext());
+        super.onCreate();


Yeah, to be honest, I really have no idea - if it works, it works!

rtibbles · 2023-12-06T23:22:53Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/NotificationBuilder.java

+
+        // defaults for service notification channel
+        if (channelId.equals(NotificationRef.ID_CHANNEL_SERVICE)) {
+            setOngoing(true);


Yeah, looking at the docs for it, it does seem like it makes the notification more rather than less prominent: https://developer.android.com/reference/android/app/Notification.Builder#setOngoing(boolean)

rtibbles · 2023-12-06T23:24:34Z

python-for-android/dists/kolibri/src/main/java/org/learningequality/NotificationRef.java

+            return;
+        }
+        ID_CHANNEL_DEFAULT = context.getString(R.string.notification_default_channel_id);
+        ID_CHANNEL_SERVICE = context.getString(R.string.notification_service_channel_id);


Yeah, I was also a little confused why the examples I was seeing drew from the resource manifest - I guess the main thing is to have them be referenceable in other files.

rtibbles · 2023-12-06T23:25:52Z

python-for-android/dists/kolibri/src/main/res/drawable/baseline_notifications_paused_24.xml

+    android:viewportHeight="24" android:viewportWidth="24"
+    android:width="24dp" xmlns:android="http://schemas.android.com/apk/res/android">
+    <path android:fillColor="@android:color/white" android:pathData="M12,22c1.1,0 2,-0.9 2,-2h-4c0,1.1 0.89,2 2,2zM18,16v-5c0,-3.07 -1.64,-5.64 -4.5,-6.32L13.5,4c0,-0.83 -0.67,-1.5 -1.5,-1.5s-1.5,0.67 -1.5,1.5v0.68C7.63,5.36 6,7.93 6,11v5l-2,2v1h16v-1l-2,-2zM14.5,9.8l-2.8,3.4h2.8L14.5,15h-5v-1.8l2.8,-3.4L9.5,9.8L9.5,8h5v1.8z"/>
+</vector>


Hrm, it's meant to show up in the notification intent launch though, right?

src/android_app_plugin/kolibri_plugin.py

bjester added 3 commits November 28, 2023 09:28

Revert "Merge pull request #175 from learningequality/notifications"

f5e4222

This reverts commit fac73e5, reversing changes made to 5cac903.

Notification reference handling for task workers

eece48a

Notification and worker task retry

7c278aa

bjester mentioned this pull request Nov 30, 2023

Revert "Merge pull request #175 from learningequality/notifications" #176

Closed

bjester added 3 commits December 1, 2023 07:08

Make our worker implementation function more like Worker, but inherit…

21de1b2

…ing RemoteListenableWorker

Remove unneeded imports and Linting fixes

99d4ae2

Remove commented code and import

ba888ad

bjester requested a review from rtibbles December 1, 2023 15:18

bjester added 2 commits December 1, 2023 07:28

Remove python-side use of Notifications

a77e3ef

Decrease thread pool to 6

e08ba06

bjester commented Dec 6, 2023

View reviewed changes

rtibbles reviewed Dec 6, 2023

View reviewed changes

bjester merged commit 017faa3 into develop Dec 12, 2023
4 checks passed

rtibbles deleted the revert-175 branch January 16, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification handling and work manager tweaks #177

Notification handling and work manager tweaks #177

bjester commented Nov 30, 2023 •

edited

Loading

bjester left a comment

bjester Dec 6, 2023 •

edited

Loading

bjester Dec 6, 2023 •

edited

Loading

bjester Dec 6, 2023

rtibbles Dec 6, 2023

bjester Dec 6, 2023

rtibbles Dec 6, 2023

bjester Dec 6, 2023

bjester Dec 6, 2023

bjester Dec 6, 2023

rtibbles Dec 6, 2023

bjester Dec 6, 2023

bjester Dec 6, 2023

rtibbles Dec 6, 2023

bjester Dec 7, 2023

rtibbles left a comment

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

rtibbles Dec 6, 2023

Notification handling and work manager tweaks #177

Notification handling and work manager tweaks #177

Conversation

bjester commented Nov 30, 2023 • edited Loading

Summary

bjester left a comment

Choose a reason for hiding this comment

bjester Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

bjester Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjester commented Nov 30, 2023 •

edited

Loading

bjester Dec 6, 2023 •

edited

Loading

bjester Dec 6, 2023 •

edited

Loading