feat: Adds tasks notification #394
Conversation
jonathanbataire
changed the title
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
Okay, this is really coming together! Thank you for all your efforts here!
src/main/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java
Show resolved
Hide resolved
| private boolean hasCheckedForNotificationApi = false; | ||
| private boolean hasTaskNotificationsApi = false; | ||
|
|
||
| private static volatile AppNotificationManager instance; |
There was a problem hiding this comment.
Do we need this to be volatile/synchronized now? It should be okay if the background worker creates its own instance of this class right?
There was a problem hiding this comment.
its used in MedicAndroidJavascript class as well so thats at least 3 instances
if we want to maintain a single instance of this helper which i think is great implementation since we only have a single channel then:
- create the instance in embeddedbrowser's oncreate and keep passing it as param where possible.
- or a singleton
also with a singleton i count a max of 2 instances on for EmbeddedBrowser and one for BG worker
There was a problem hiding this comment.
So, I might be wrong here, but I thought all the Android notification stuff was synced at the system level based on the CHANNEL_ID value. So, we should never end up with some kind of "real" duplicate notification channels at the system level.
In general I think it is good to avoid duplicating instantiated objects when you can re-use them just for the slight performance benefits. But, the extra performance overhead caused by volatile/synchronized means that I don't think it is worth the extra complexity except when:
- It is really expensive to instantiate an object.
- The state of the object has to be shared between threads.
I don't think either of those apply here, right? I still think it is fine to stick with the singleton pattern for the AppNotificationManager, but I am not sure we gain anything by re-using the instance across threads. 🤷
There was a problem hiding this comment.
i dont have strong opinion about this i dont think re instantiating the object is that expensive (ive been doing this myself) will change this implementation and remove the singleton
src/main/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java
Outdated
Show resolved
Hide resolved
| void initAppNotification(WebView webView, Activity activity) { | ||
| foregroundNotificationHandler = new NotificationForegroundHandler(webView, appUrl); | ||
| manager.cancelAll(); | ||
| if (hasTaskNotificationsApi) { |
There was a problem hiding this comment.
I think we should avoid keeping this value as state. It is just adding an extra thing that we have to keep track of. I think it is fine to ask the user for the notification permission regardless of if their CHT instance supports notifications or not. (Presumably all CHT instances will eventually be upgraded to support this feature anyway.) By not caching this value, we don't have to worry about somehow busting the cache when their CHT instance gets upgraded...
There was a problem hiding this comment.
idk didnt want to start these processes at all especially GB worker if CHT has not API
There was a problem hiding this comment.
also this is used to stop notification workers when user is logged out
There was a problem hiding this comment.
idk didnt want to start these processes at all especially GB worker if CHT has not API
Why? If the concern is around performance, then it feels like we need a different mechanism for disabling notifications anyway because all supported versions of the CHT will eventually support the notifications API (but the performance implications on the client device will still exist regardless).
also this is used to stop notification workers when user is logged out
It still feels more safe/reliable/predictable to just always try to stop the notifications (without trying to keep track of the current status).
Having state like this in the AppNotificationManager forces a lot of down-stream complexity that I really do not see the benefits of. If a particular user does not want notifications, they can just deny the permission and we will not run anything. Do we need a way for a whole deployment to disable notifications?
| } | ||
| } | ||
|
|
||
| void initAppNotification(WebView webView, Activity activity) { |
There was a problem hiding this comment.
Minor, but is there a reason to have this function be distinct from startForegroundNotificationHandler? I am thinking of a linear init flow:
- Start with a call to
startForegroundNotificationHandler- This calls
requestNotificationPermission- Checks
hasNotificationPermission- iftrue. thenrequestNotificationPermissionreturnstrue. - If
false, request the permission and returnfalse.
- Checks
- This calls
- If
requestNotificationPermissionreturnsfalse, thestartForegroundNotificationHandlerreturns without doing anything because we have no notification permission (yet). - The
EmbeddedBrowserActivity.onRequestPermissionsResultthen callsstartForegroundNotificationHandleragain when the permission is granted. - In
startForegroundNotificationHandlerwhenrequestNotificationPermissionreturnstrue, then we proceed to start the handler.
| public Result doWork() { | ||
| Log.d(DEBUG_TAG, "background worker running......"); | ||
| latch = new CountDownLatch(1); | ||
| Handler handler = new Handler(Looper.getMainLooper()); |
There was a problem hiding this comment.
I did not expect us to need the Handler here in the NotificationWorker. I thought that the Android system would handle periodically executing the worker (without us needing to worry about manually looping inside the worker...)
There was a problem hiding this comment.
the handlers job here is to get the main thread and post work to it
which is the only place a webview would work without this nothing webview related will work
There was a problem hiding this comment.
Wait, what does "the main thread" mean in the context of a background worker? Can we not instantiate a new WebView directly in the NotificationWorker class (as a part of the background worker thread)?
If not, how confident are we that all of this will work reliably (especially across different Android systems/versions)? (And sorry in advance if this is some kind of common pattern for doing thing that I am just not familiar with... 😅 I just know that reliable background notifications are always problematic in Android when you are outside the Firebase paradigm and I want to be sure we are not setting ourselves up for failure here...
There was a problem hiding this comment.
webview cant run on the background thread
when the system executes the background job 2 threads exist bg thread and the main app process thread aka main thread
bg thread posts work on the main app process thread via the handler that executes it
this works well from all my tests
this process is handled by the system (not a custom implementation) so it should work across android versions
There was a problem hiding this comment.
Do you have links to any documentation that would suggest launching a handler on the main looper from a background worker is a supported operation? I understand that you have testing this and it worked for you, but I am very concerned about the reliability of this approach (especially across different Android systems/versions).
Android is already super tricky around reliable background operations (as seen by the on-going saga of most other FLOSS apps that try to support notifications without Firebase...). Adding unsupported UI thread execution is not going to help this be more reliable... 😬
I have done a bunch more research today around this and keep coming back to this:
- A WebView can only be created in the "UI Thread" (as you have noted).
- Running a handler on the main looper (as we are doing here) does effectively execute the handler on the main UI thread. As long as you have an application context (as we do here in the Background Worker).
- However, views (including WebViews) should only be created with an Activity in context. Any creation of a view without an Activity is technically unsupported.
- In almost all cases, activities cannot be launched from the background (without user input).
- Even foreground services (e.g. with a persistent notification) cannot reliably start activities (without user input).
- There is no reliable way to keep an activity running indefinitely.
We need a WebView to run the webapp and check for notifications. The only supported way to get a WebView is with an Activity in context, but there is nor reliable way to get an activity in context when the app is not open. My conclusion is that there does not seem to be any supported way to achieve notifications with our current architecture. Please, please, correct me here if I am missing something!
All this being said, it seems like a hard thing to come all this way and have notifications being successfully pushed by your current approach, but then we give up because it is not technically supported (especially when we don't really have any other viable options). @jonathanbataire I understand you have a partner that is interested in using these notifications in production. Would it be possible to build them an APK to use that is based on this branch (without merging to master)? We could then collect some telemetry data about how things are working in the real world and let that inform the viability of merging this PR in the future and providing the feature to the broader community.
On a somewhat related note, since we run this notification check in the main UI thread, does this mean that every time we check for notifications, the device will become unresponsive? When the app is in context, all we are doing is making a quick JS call, but here in the background worker we have to spin up the whole WebView/webapp before making the JS call. Just worried that the UX here may not be good if we lock the UI thread even for a few seconds...
There was a problem hiding this comment.
This is what it looks like in the device as you can see its downloaded/updated from play store not an OS update by the manufacturer
There was a problem hiding this comment.
Device compatibility issues in Android are a big enough problem for open source apps that folks are writing research papers about it.
- device doesnt have google services like huawei devices implement there own thing
We dont and shouldnt cater for any of the above
I understand where you are coming from with custom ROMS (I use GrapheneOS at my own risk 😅 and it def does not bundle the Android System Webview from the Play Store...). However, I think we definitely need to support OEM ROMs that do not bundle Google Play Services. Without global devices stats for the CHT, we cannot be sure exactly how common these are in the field, but I do know that our experience working with various programs indicates we cannot assume everyone is going to have a working copy of Google Play Services...
Hopefully this is all a nothing-burger and our usage here works fine on all devices across-the-board. There is just a lot of history around the Android System Webview that we have struggled with ever since ditching Crosswalk. And I am cautions when it comes to making any assumptions in this area....
again we can test this across different devices first if it helps
if indeed we want to pilot this first this is not the reason to base this decision on imo
I think this is a fine place to land the conversation. We don't need to agree on on the risk here. Lets move forwards with the pilot and we can collect data about how this is getting used. Then we will be well positioned to promote this feature when we release it for the rest of the community. (Or, worst case scenario, we realize this is not going to work as we hoped....)
There was a problem hiding this comment.
i see
However, I think we definitely need to support OEM ROMs that do not bundle Google Play Services.
do you have an example of this? there is no OEM that ships without play services as far as i know (except some huawei phones that dont sell outside china)
making this argument would mean CHT android as a whole wont work because the main webview uses this default android webview and is written assuming that
alot of android features assume device has play services including GPS in this app
The app is not built for this scenario (i dont think we should) even besides this feature
I use GrapheneOS at my own risk 😅 and it def does not bundle the Android System Webview from the Play Store...
there is no device that ships with these ROMs im pretty sure you installed this yourself and know the risks its comes with even beyond just webviews
but I do know that our experience working with various programs indicates we cannot assume everyone is going to have a working copy of Google Play Services...
these are system level components that require and are updated all the time thru play store they are the same for all devices
There is just a lot of history around the Android System Webview that we have struggled with ever since ditching
yes CHT android supports webview 90+ and I see it fail all the time in the field (gets stuck on loading page). This is because someone is running an old webview. the solution is to always update webview to the latest version and everything works fine.
i still dont see a genuine reason to veto this to be frank with you
There was a problem hiding this comment.
i still dont see a genuine reason to veto this to be frank with you
I don't believe that a veto is what you're seeing here @jonathanbataire , it's just caution before releasing some code we are not completely confident with, and, by consequence, making ourselves responsible for maintaining it for future releases.
I agree with @jkuester's caution. We have had smaller issues.
WebView should always be instantiated with an Activity Context. If instantiated with an Application Context, WebView will be unable to provide several features, such as JavaScript dialogs and autofill.
I have seen this note i think its just a warning it shouldnt be taken that app context wont work it just highlights the limitations
its just a limitation not that it wont/doesnt work
none of these limitation affect this implementation
It's true that some "notes" have less meaning than others, but I don't think we should disregard this so easily, just because it works on a few devices. At the least, we must know what is the list of features that are not available in this "headless" webview.
I agree with @jkuester 's suggestion to launch this as a pilot, as a "feature release" for CHT + CHT Android and see how it behaves over time. This is our general approach for things that are more on the experimental side.
| @Override | ||
| public Result doWork() { | ||
| Log.d(DEBUG_TAG, "background worker running......"); | ||
| latch = new CountDownLatch(1); |
There was a problem hiding this comment.
What do we need this latch for? I am having trouble understanding what we are protecting against here...
There was a problem hiding this comment.
i need a way to timeout this execution so it doesn't run forever whether inside JS context or Java context
the countdownlatch is used for this purpose
it returns a value to stop work manager as soon as js executes and notification sent or execution is taking more than 10 secs
in otherwords caps execution to less than 10 secs
There was a problem hiding this comment.
so it doesn't run forever whether inside JS context or Java context
But we don't have any operations here that we anticipate could possibly just run indefinitely, right? We are executing an async operation that should either succeed or fail, but it is not designed to re-try indefinitely or anything like that.... If somehow the background thread does get stuck because of an unanticipated issue, Android, itself, could end up killing it or at the very least it will get closed the next time the user opens the app and we cancel all the background workers.
There was a problem hiding this comment.
android's timeout is 10 mins
so in case anything happens eg js function cant be accessed, this worker will run for 10 mins
There was a problem hiding this comment.
in case anything happens eg js function cant be accessed
I think this is still where I am lost. 🤔 Why would the JS execution just hang indefinitely instead of just succeeding or failing at runtime? Is there something unique to our situation here that I am not considering, or is this a problem that could be faced in all the other JS calls in cht-android?
There was a problem hiding this comment.
Hmm, I see what you are saying about the callback not working (classic Android to be half-way to something useful, but not actually cover obvious cases like async JS...). 🤦
Regardless, the callback is not important. Can just switch back to an interface. This seems to work as expected for me:
private static final String NOTIFICATION_JS = """
(async () => {
const tasks = await window?.CHTCore?.AndroidApi?.v1?.taskNotifications();
NotificationWorker.onGetNotificationResult(JSON.stringify(tasks ?? []));
})();
""";
private final WebViewClient onFinishedClient = new WebViewClient() {
@Override
public void onPageFinished(WebView view, String url) {
view.evaluateJavascript(NOTIFICATION_JS, null);
}
};
@NonNull
@Override
@SuppressLint("SetJavaScriptEnabled")
public ListenableFuture<Result> startWork() {
Handler handler = new Handler(Looper.getMainLooper());
return CallbackToFutureAdapter.getFuture(completer -> {
handler.post(() -> {
WebView webView = new WebView(getApplicationContext());
webView.getSettings().setJavaScriptEnabled(true);
Object notificationInterface = new Object() {
@JavascriptInterface
public void onGetNotificationResult(String data) {
try {
AppNotificationManager appNotificationManager = new AppNotificationManager(getApplicationContext());
appNotificationManager.showNotificationsFromJsArray(data);
Log.d(DEBUG_TAG, "notification worker ran successfully!");
completer.set(Result.success());
} catch (Exception e) {
log(e, "error checking for notifications");
completer.set(Result.failure());
} finally {
destroyWebView(webView);
}
}
};
webView.addJavascriptInterface(notificationInterface, "NotificationWorker");
enableStorage(webView);
webView.setWebViewClient(onFinishedClient);
webView.loadUrl(appUrl);
});
return "Async notification check";
});
}There was a problem hiding this comment.
this throws error in the finally block when cleaning up webview
Are you able to get the same?
java.lang.Throwable: A WebView method was called on thread 'JavaBridge'. All WebView methods must be called on the same thread.
also doesnt seem to work for me (showing notifications)
it seems js checks (optional chaining) fail and it returns []
it not consistent sometimes it works sometimes not
There was a problem hiding this comment.
this throws error in the finally block when cleaning up webview
Are you able to get the same?
Yeah, I was able to repro this. Just needed to tweak the destroyWebView call to run in the main thread like you had it before:
} finally {
handler.post(() -> destroyWebView(webView));
}also doesnt seem to work for me (showing notifications)
it seems js checks (optional chaining) fail and it returns []
it not consistent sometimes it works sometimes not
So, I did some more testing here. Actually, once I fixed the destroy error it stopped showing any notifications for me at all (which seemed weird). It turns out, what is happening is the onPageFinished gets triggered multiple times when we call webView.loadUrl(appUrl);. This is because loading the webapp ends up causing the page to get re-directed multiple times. The first call to onPageFinished is just for the root page, /, and the app is not loaded yet. So, the NOTIFICATION_JS would return empty. Then, when the app is loaded, it redirects to /home which triggers another onPageFinished call. Now the app is loaded an the NOTIFICATION_JS call can return actual notifications. Then the webapp redirects me for a final time to /messages and onPageFinished is called again (and returns the same notification data!??!)... I am not sure how your original code was handling these multiple calls to onPageFinished??? (Maybe the latch was sorting it out or something else was going on....)
I got everything to play nice with an updated version of the onFinishedClient:
private final WebViewClient onFinishedClient = new WebViewClient() {
boolean triggered = false;
@Override
public void onPageFinished(WebView view, String url) {
if (triggered || (appUrl + "/").equals(url)) {
return;
}
triggered = true;
view.evaluateJavascript(NOTIFICATION_JS, null);
}
};It seems to work as expected (prevents running the NOTIFICATION_JS on the root URL (when the app is not loaded) and re-running it for the /messages redirect), but if feels a bit brittle. Maybe you have some ideas on how to do this better? So far I have not come up with a good way to "run this when the webapp finishes bootstrapping"...
There was a problem hiding this comment.
nice sleuthing!
yeah it does a lot of looping not sure why
the latch would just let it do its thing and only stop waiting once notifications are sent thats why it works
There was a problem hiding this comment.
updated....
| } | ||
|
|
||
| @JavascriptInterface | ||
| public void onGetNotificationResult(String data, String appUrl) throws JSONException { |
There was a problem hiding this comment.
This probably depends on the responses to my questions above, but I wonder if we can avoid duplicating this function here and in MedicAndroidJavascript. I am thinking maybe we could just have one version of this function on the AppNotificationManager and then the NotificationForegroundHandler could add it as a Javascript interface similar to how we do it in this file...
| wv.stopLoading(); | ||
| wv.getSettings().setJavaScriptEnabled(false); | ||
| wv.loadUrl("about:blank"); | ||
| wv.clearHistory(); | ||
| wv.clearCache(true); | ||
| wv.removeAllViews(); | ||
| wv.destroy(); | ||
| wv = null; |
There was a problem hiding this comment.
Most of this feels a bit redundant since we are calling destroy on the WebView.
Since we are not attaching the webview to anything or adding any child views, I guess maybe all we really need here is to call stopLoading and destroy. Right?
There was a problem hiding this comment.
i see
some of these are supposed to make it faster for android to take down the webview but let me look at it some more
src/main/java/org/medicmobile/webapp/mobile/NotificationForegroundHandler.java
Show resolved
Hide resolved
|
Sorry that I have not been able to review the updates here yet! I ran out of time this week, but I have not forgotten it and it is in the queue for next week. 👍 |
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
Okay, we are getting close here! 💪 Couple more mostly minor things....
| private final String appUrl = settings.getAppUrl(); | ||
| private WebView webView; | ||
| private final Handler handler = new Handler(Looper.getMainLooper()); | ||
| String notificationsJS = "(async () => { " + |
There was a problem hiding this comment.
Minor, can be private.
| String notificationsJS = "(async () => { " + | |
| private String notificationsJS = "(async () => { " + |
| @Override | ||
| public void onStopped() { | ||
| super.onStopped(); | ||
| destroyWebView(webView); |
| @SuppressLint("SetJavaScriptEnabled") | ||
| @NonNull | ||
| @Override | ||
| public ListenableFuture<Result> startWork() { |
There was a problem hiding this comment.
I think you can resolve the Sonar cognitive complexity issue with this method by just keeping the web view client as a top-level final variable (instead of creating it on-demand in this method):
private final WebViewClient webViewClient = new WebViewClient() {
boolean triggered = false;
@Override
public void onPageFinished(WebView view, String url) {
if (triggered || (appUrl + "/").equals(url)) {
return;
}
triggered = true;
view.evaluateJavascript(notificationsJS, null);
}
};| private void enableStorage(WebView wv) { | ||
| WebSettings webSettings = wv.getSettings(); | ||
| webSettings.setDomStorageEnabled(true); | ||
| webSettings.setDatabaseEnabled(true); |
There was a problem hiding this comment.
| webSettings.setDatabaseEnabled(true); |
I think we can remove this line (and fix the deprecation issue), both here and in the EmbeddedBrowserActivity. According to the docs linked from the deprecation notice, this feature is related to WebSQL. I could not find any evidence that tech was ever used in the CHT (FireFox never supported it). So, I do not think we ever actually needed this call. It seems to have been added when we ripped xwalk out, but I don't think it was necessary....
| appNotificationManager = new AppNotificationManager(this, foregroundNotificationHandler); | ||
| appNotificationManager.cancelAllNotifications(); | ||
| appNotificationManager.requestNotificationPermission(this); | ||
| appNotificationManager.startForegroundNotificationHandler(); |
There was a problem hiding this comment.
I don't think we need this here since we are already calling this later in onResume, right?
| appNotificationManager.startForegroundNotificationHandler(); |
| manager.cancelAll(); | ||
| } | ||
|
|
||
| void showMultipleTaskNotifications(JSONArray dataArray) throws JSONException { |
There was a problem hiding this comment.
Can be private:
| void showMultipleTaskNotifications(JSONArray dataArray) throws JSONException { | |
| private void showMultipleTaskNotifications(JSONArray dataArray) throws JSONException { |
| } | ||
| } | ||
|
|
||
| void showNotification(Intent intent, int id, String title, String contentText) { |
There was a problem hiding this comment.
Can be private.
| void showNotification(Intent intent, int id, String title, String contentText) { | |
| void showNotification(Intent intent, int id, String title, String contentText) { |
| String contentText = notification.getString("contentText"); | ||
| String title = notification.getString("title"); | ||
| long readyAt = notification.getLong("readyAt"); | ||
| int notificationId = (int) (readyAt % Integer.MAX_VALUE); |
There was a problem hiding this comment.
🤔 Is there a reason we should use the readyAt value as the unique id instead of just getting the hash for the actual task id?
int notificationId = notification
.getString("_id")
.hashCode();| postTask(); | ||
| return; | ||
| } | ||
| webView.setWebViewClient(new WebViewClient(){ |
There was a problem hiding this comment.
I do not think we want to call setWebViewClient here since I think that will replace the UrlHandler client set in EmbeddedBrowserActivity.
The UrlHandler already has a onPageFinished method that broadcasts a onPageFinished intent. I wonder if we could just trigger posting the task via a BroadcastReceiver like this?
|
|
||
| private final Runnable task; | ||
| private final WebView webView; | ||
| private boolean isPageLoaded = false; |
There was a problem hiding this comment.
Minor but I think isTaskPosted would be a more descriptive name of what this value is actually indicating.
|
Sorry for barging in, but I had an idea of how to change this so it's a safer Android bet. there are only three situations that would get tasks to change:
Both 2 and 3 require an internet connection and a successful sync. I don't know if the background process that this PR is launching now can achieve the sync before the tasks get sent - I believe there is no Webapp support for that - and triggering sync every x minutes while the user is not actually using the app would add a lot of load on the server, so it's unlikely we desire this. With this said, I think we would cover 95% of useful notifications using preloaded tasks in Android. @jonathanbataire and @jkuester please let me know what you think. |
so basically:
@dianabarsan all tasks are recalculated on new report(report edit?) and settings change right? just so I get you right so if we update notification store every time recalculation occurs without minding how it happens this should keep the store pretty much up to date |
|
@jonathanbataire Exactly!
So, I don't know what it means that the data is stale. I suppose that you're talking about cases where the CHT has not opened the app in 10 days - for example. In this case, I don't think we know that the data is stale and we should notify anyway. We only know the data is stale if we open the app, sync and recalculate. So this is Schrodinger's data, both stale and not stale :D |
hahaha |
FWIW, I think SharedPrefs are pretty good for basic key-value storage, but they leave something to be desired when it comes to more complex scenarios. In a different app, I have had good experiences with the Android Datastore library (though that was with Kotlin, so the Java interfaces may not be so nice...). Given that our data is coming from a types-optional JS context anyway, we can probably go with the simple Preferences DataStore (instead of adding the extra overhead of Protobuf). 🤷 |
I'm using datastore |
Description
This PR adds tasks notification capabilities
related to medic/cht-core#9255
Closes #93