Closed shankari closed 2 years ago
wrt
local notification callbacks not invoked if app has been killed/restarted versus just resumed
this is because when we generate the notification from SensorControlBackgroundChecker.generateOpenAppSettingsNotification
, we do so with the MainActivity
.
We expect to get a callback in DataCollectionPlugin.onNewIntent
, which will then, in SensorControlForegroundDelegate.onNewIntent
schedule a second plugin compatible notification.
The problem is that the first intent invokes the main activity which already launches the app, before the second notification is generated. We then end up with two notifications with the same message.
Clicking on the original notification again generates a trigger
event since the app is already running so it works.
This also explains why the notification is not cancelled when the user clicks on it - it is not plugin-scheduled notification.
Adding additional messages to the notifications generated from the two separate locations clarifies this.
In the video below:
https://user-images.githubusercontent.com/2423263/152728246-3a7d1558-b042-4a55-a751-b608d8fadf73.mp4
Tried to fix this by generating the "from plugin" notification directly. This now shows up as soon as the motion activity permission is removed. However, clicking it still does not generate the notifications.
This is because the local notification callback
localNotify.registerRedirectHandler = function() {
Logger.log( "registerUserResponse received!" );
....
}
is called at 21:25
2022-02-06 21:25:24.870 24517-24517/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:registerUserResponse received!", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-06 21:25:25.038 24517-24517/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:registerUserResponse received!", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
after the event is fired
2022-02-06 21:25:24.609 24517-24517/edu.berkeley.eecs.emission D/CordovaWebViewImpl: >>> loadUrl(javascript:cordova.plugins.notification.local.fireEvent("click",{"id":362253744,"title":"Incorrect app settings plugin-compatible","text":"Click to view and fix app status plugin-compatible","data":{"redirectTo":"root.main.control","redirectParams":{"launchAppStatusModal":true}},"trigger":{"type":"calendar"},"progressBar":{"enabled":false}},{"event":"click","foreground":false,"queued":true,"notification":362253744}))
This is the full history
2022-02-06 21:45:53.880 4249-4249/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY onCreate called
2022-02-06 21:45:53.936 4249-4249/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY onResume called
2022-02-06 21:45:58.159 4249-4587/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, sending pending javascript
2022-02-06 21:45:58.159 4249-4587/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, finished sending pending javascript
2022-02-06 21:45:58.204 4249-4249/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY ionicPlatform is ready in the app", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-06 21:45:58.263 4249-4249/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY ionicPlatform is ready in the app", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-06 21:45:58.483 4249-4249/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY received ready, registering handlers", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-06 21:45:58.551 4249-4249/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY received ready, finished registering handlers", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
Ah, the documentation says: https://github.com/katzer/cordova-plugin-local-notifications#launch-details
It might be possible that the underlying framework like Ionic is not compatible with the launch process defined by cordova. > With the result that the plugin fires the click event on app start before the app is able to listen for the events. Therefore its possible to fire the queued events manually by defining a global variable.
window.skipLocalNotificationReady = true
Once the app and Ionic is ready, you can fire the queued events manually.
cordova.plugins.notification.local.fireQueuedEvents();
After those changes, the logs are:
2022-02-07 11:58:00.592 21780-21780/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY onCreate called
2022-02-07 11:58:00.631 21780-21780/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY onResume called
2022-02-07 11:58:04.761 21780-21780/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY ionicPlatform is ready in the app", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-07 11:58:04.820 21780-21780/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY ionicPlatform is ready in the app", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-07 11:58:05.021 21780-21780/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY received ready, registering handlers", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-07 11:58:05.081 21780-21780/edu.berkeley.eecs.emission I/chromium: [INFO:CONSOLE(49)] "DEBUG:LOCAL NOTIFY received ready, finished registering handlers", source: https://localhost/plugins/cordova-plugin-em-unifiedlogger/www/unifiedlogger.js (49)
2022-02-07 11:58:05.109 21780-22039/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, sending pending javascript
2022-02-07 11:58:05.109 21780-22039/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, finished sending pending javascript
2022-02-07 11:58:18.013 21780-21780/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, sending pending javascript
2022-02-07 11:58:18.013 21780-21780/edu.berkeley.eecs.emission I/LOCALĀ NOTIFICATION: LOCAL NOTIFY: deviceready called, finished sending pending javascript
and the operation is correct.
In particular, note the plugin specific notification is at around 0:03 and clicking on it launches the app and opens the status screen at 00:07
https://user-images.githubusercontent.com/2423263/152870599-5d3808d9-9834-480b-9337-ca38eaf63dd1.mp4
Publishing new version on android and then moving on to making the same changes on iOS... Will clean up components after iOS changes.
Turning off activity permissions works, but turning off location goes back to our infamous infinite loop. So near and yet so far...
In this case, because the location permission is turned off, we actually end up staying in the start state.
2022-02-07 23:24:30.754 12834-12834/edu.berkeley.eecs.emission I/TripDiaryStateMachineRcvr: TripDiaryStateMachineReciever onReceive(android.app.ReceiverRestrictedContext@eae6be5, Intent { act=local.transition.tracking_error flg=0x10 pkg=edu.berkeley.eecs.emission cmp=edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineReceiver }) called
2022-02-07 23:24:30.771 12834-12834/edu.berkeley.eecs.emission I/TripDiaryStateMachineService: Service destroyed. So long, suckers!
2022-02-07 23:24:30.795 12834-12834/edu.berkeley.eecs.emission I/TripDiaryStateMachineService: Service created. Initializing one-time variables!
2022-02-07 23:24:30.803 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: service started with flags = 0 startId = 1 action = local.transition.tracking_error
2022-02-07 23:24:30.812 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: after reading from the prefs, the current state is local.state.start
2022-02-07 23:24:30.841 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: handleAction(local.state.start, local.transition.tracking_error) called
2022-02-07 23:24:30.865 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: TripDiaryStateMachineReceiver handleStarted(local.transition.tracking_error) called
2022-02-07 23:24:30.872 12834-12834/edu.berkeley.eecs.emission I/TripDiaryStateMachineService: Already in the start state, so going to stay there
2022-02-07 23:24:30.880 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: newState after handling action is local.state.start
2022-02-07 23:24:30.892 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: newState saved in prefManager is local.state.start
2022-02-07 23:24:30.907 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: handleAction(local.state.start, local.transition.tracking_error) completed, waiting for async operations to complete
But we stay in the start state by calling setState(start)
which triggers the checks
Log.e(fCtxt, TAG, "error while creating geofence, staying in the current state");
Log.exception(fCtxt, TAG, task.getException());
setNewState(mCurrState, true);
...
if (doChecks) {
SensorControlBackgroundChecker.checkAppState(TripDiaryStateMachineService.this);
}
which generates an initialize
2022-02-07 23:25:05.750 12834-12834/edu.berkeley.eecs.emission I/SensorControlBackgroundChecker: in start state, sending initialize
and so on, ad inifinitum
2022-02-07 23:24:32.213 29553-29570/? W/GLMSImpl: edu.berkeley.eecs.emission doesn't have sufficient location permission to request geofence.
2022-02-07 23:24:32.221 12834-12834/edu.berkeley.eecs.emission E/TripDiaryStateMachineService: error while creating geofence, staying in the current state
2022-02-07 23:24:32.233 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: newState after handling action is local.state.start
2022-02-07 23:24:32.241 12834-12834/edu.berkeley.eecs.emission D/TripDiaryStateMachineService: newState saved in prefManager is local.state.start
...
2022-02-07 23:24:32.374 12834-12834/edu.berkeley.eecs.emission I/SensorControlBackgroundChecker: Curr status check results = loc permission, motion permission, notification, unused apps [false, true, true, true]
Calling restartFSMIfStartState
only if we have adequate permissions should solve this problem. If we know that there are problems with the permissions, there's no point in trying to initialize the FSM. Tried this, and the FSM gets stuck in the start state. Why doesn't the foreground service restart call initialize and wake it up?
Looks like the foreground service is destroyed
2022-02-08 00:16:25.182 2068-2088/? W/ActivityManager: Scheduling restart of crashed service edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineForegroundService in 1000ms
2022-02-08 00:16:26.199 2068-2848/? W/ActivityManager: Stopping service due to app idle: u0a154 -26m37s622ms edu.berkeley.eecs.emission/.cordova.tracker.location.TripDiaryStateMachineForegroundService
2022-02-08 00:16:26.242 2068-2099/? I/ActivityManager: Start proc 30782:edu.berkeley.eecs.emission/u0a154 for service {edu.berkeley.eecs.emission/edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineForegroundService}
2022-02-08 00:16:27.017 30782-30782/edu.berkeley.eecs.emission D/TripDiaryStateMachineForegroundService: onCreate called
2022-02-08 00:16:27.029 30782-30782/edu.berkeley.eecs.emission D/TripDiaryStateMachineForegroundService: onDestroy called for foreground service
2022-02-08 00:16:27.039 30782-30782/edu.berkeley.eecs.emission D/TripDiaryStateMachineForegroundService: onDestroy called, removing notification
It looks like the app is restarted when permissions are removed, but not when they are granted. So the foreground service is not killed, and is not restarted and does not initialize the FSM. We will re-initialize the FSM at the next periodic check, of course, but let's see if we can do something better.
So the problem is that in the initial FSM, we are polling continuously. Which allows us to find that the permission has been reinstated, but is terrible from a performance perspective and from an "app doesn't appear to be broken" perspective. We can remove the continuous polling by removing the reinitialize from within the check permissions. But then we need to be notified when the value has changed.
This is non-trivial because we don't always get a callback when the permissions have changed. So we need to handle the common case and move on.
Just to finish testing this:
The minor inconsistencies with the location settings (highlighted in bold above) are because it is an async call. So:
tracking_error
, which in turn means that the state remains in waiting_for_trip_start
Once we fix those, we are really done.
The challenge is that the settings check comes with a callback so it doesn't happen at the same time as the permission checks. We can generate a tracking_error
from the callback as well,
private static void checkLocationSettings(final Context ctxt) {
SensorControlChecks.checkLocationSettings(ctxt, resultTask -> {
try {
} catch (ApiException exception) {
ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); // new code
generateOpenAppSettingsNotification(ctxt);
}
});
}
but when we generate the initialize once the permission checks all pass as well,
if (allOtherChecksPass) {
Log.d(ctxt, TAG, "All permissions (except location settings) valid, nothing to prompt");
restartFSMIfStartState(ctxt);
}
which leads to our infamous infinite loop from https://github.com/e-mission/e-mission-docs/issues/680#issuecomment-1032915223
We really need to treat the location settings as a required check. Which means that we need to have the results along with the permission check results. We can probably do some task-related magic to make this happen, but the easiest option is probably to just inline the callback.
For the second issue, we need to recompute the values when we launch the screen instead of only when the controller is initialized. That means moving it out into a service. I really wanted to avoid this, but it has to be done. Actually, an alternative is a decoupled, event-based call using $broadcast
and $on
to send a message directly to the controller.
This is a bit tricky since this function is called whenever the authorization is changed either during onboarding or at any time after that.
let's stick with storing state, similar to the android version, to keep the ports consistent as much as possible.
Switched to an instance class in https://github.com/e-mission/e-mission-data-collection/pull/195/commits/b7ff5d6a3ee9df7b1cb7572f25a6625792a9d68d
Even after switching to instance classes, this is a bit tricky since the callback is in a different class, and not the same class. And our desired behavior is different depending on whether the change was part of a user-initiated operation or not.
Note that difference here is user-initiated vs. background, not onboarding vs. later. If the user attempts to fix the permission from the status screen months after installation, they don't want to see a weird notification either. This is actually not a problem if the permission is correct - we will cancel the notification first anyway.
But if the permission is incorrect, then showing another notification would be bad. Because the app is already open, this would be another "trigger", which would generate an in-app popup.
Note also that we currently get a callback on app start (before first page is shown) that I can only see with a breakpoint.
This makes me nervous about timing issues wrt state during app start. We can't afford to have false positives wrt errors. It might lead to a "cry wolf" syndrome where people ignore the notifications since they show up so often.
So ideally, the TripDiaryDelegate would store the information about whether call was from the foreground or not. Note that if it is, it would need to send a callback to the ForegroundDelegate so it could use the correct commandID to respond to the user.
Both delegates are instances, not classes, so we cannot easily make static calls back and forth.
The cleanest solution seems to be for the ForegroundDelegate to let the TripDiaryDelegate know about itself. The TripDiaryDelegate can then call it if it exists, or call the background checker otherwise.
The foreground delegate can notify the TripDiaryDelegate about itself in one of two ways:
TripDiaryDelegatePermissions
category to "register" itself. The static call would get the current TripDiaryStateMachine
instance, get its delegate, and then register itself there.Let's go with the static method approach since it seems simpler.
Actually, the static method approach will not work. The static call can get the current TripDiaryStateMachine
instance but the delegate property of the instance is not public. If it were public, we would not need the static method because we could call the TripDiaryStateMachine
directly.
The event-based architecture also has a problem around where we can register for the event. If we do so in the main class init
, then it breaks the decoupling of the permission stuff. There is no other place we can do it where we can access an instance variable.
A simpler option might be to expose the delegate from the TripDiaryStateMachine
. Since there is only one TSDM and one delegate, this should allow us to treat the delegate as a singleton and will make message passing much simpler.
Note also that we currently get a callback on app start (before first page is shown) that I can only see with a breakpoint.
I was worried about this and wondered whether we need to store state on intro_done
versus not. The truth table would then be:
from_plugin | intro_done | task |
---|---|---|
T | x | callback |
F | F | ignore (startup call before user acceptance) |
F | T | generate notification |
However, it turns out that in the (F, F) case, the user has also not accepted the notification permission, so the notification does not show up š
So our simplified truth table is:
from_plugin | intro_done | task |
---|---|---|
T | x | callback |
F | F | generate notification but it will not be visible (startup call before user acceptance) |
F | T | generate notification |
which simplifies to
from_plugin | intro_done | task |
---|---|---|
T | x | callback |
F | x | generate notification |
which simplifies to
from_plugin | task |
---|---|
T | callback |
F | generate notification |
with https://github.com/shankari/e-mission-data-collection/commit/ca979bd85f585161cdb387687bbd2acba4f3cb85, we have fixed an infinite loop when location permissions are missing. This is also consistent with the behavior on android.
But I am a bit concerned about not recomputing when we get a geofence tracking error; if the location is turned off after the app install, will we still get a notification? I guess we will when the next force sync happens. Need to test on both android and iOS and confirm that the behavior is acceptable.
moving on to the motion and fitness permissions, we need to decide how to handle the case in which the device doesn't support the feature. This should not be a big issue - we support a minimum of iOS 11, and the iPhone4 and iPhone 5c don't support it. https://support.apple.com/en-us/HT209574
The oldest supported versions are iPhone 5s and iPhone SE.
So what should we do if the fitness sensor is not available? We could say that the fitness is green, but then have people complain if it doesn't work as expected. Or we could say that the fitness is red, but then people can't continue with the install. Need to think about this...
Decision: To be consistent with https://github.com/e-mission/e-mission-docs/issues/700, we will ask users to uninstall the app if the device can't support the fitness sensors. We will add a workaround for the simulator so that we can continue testing...
The motion activity stuff doesn't have callbacks, so we tried to make some to at least cover the common case of prompting the user when the value is not determined.
Current behavior:
Terminated due to signal 9
Reproducing the bad state The app doesn't even show up as having requested access to the activity. It only shows up after having requested the access.
No activity access | In the app either | Until we request access? |
---|---|---|
Can confirm:
App install | No app entry | Prompt | App entry | On app screen |
---|---|---|---|---|
Tried resolution: prompting the user even when CMAuthorizationStatusRestricted
. Unfortunately, the call just errored out with code 105. The app wasn't added to the fitness settings list either. This continued until I stopped and restarted the app. Basically, it looks like the the "restricted" status is only refreshed when the app starts. Note also that we get the same error code (105) when the settings are valid and the permissions are not so we can't use it to distinguish between the two cases.
Final resolution: change the text to highlight that the app needs to be restarted.
After enabling notification permissions, we are almost done. Except for one really bizarre issue. On iOS13, when we open the app settings, the location permission is not visible. After a few hours spent debugging this, the situation is:
So my current thoughts are:
Root cause: We need to actually start trying to read locations. If I comment out this one line
https://github.com/e-mission/e-mission-data-collection/blob/v1.6.0/src/ios/Location/GeofenceActions.m#L58
the option no longer shows up even on the older version. Note in the old version we called markConsented
which initialized the code and generated the prompts. With the new version we first initialize everything and then call markConsented
.
Solution: The solution is to start tracking before opening the app for the location case, and stopping tracking afterwards.
Testing done: After commenting out the line [_locMgr startUpdatingLocation];
from the old version and adding it to the new version, we end up with the permissions swapped. This is a clear indication that the startUpdatingLocation
is needed before we can see the permission.
One more super-duper final fix - when we add the plugin, the newly added files don't appear to be added to the project. We had to add them manually. Let's debug that briefly and then finish this merge.
ROOT CAUSE: we were labeling the source files as header-file
. Fixed and everything compiles now.
@PatGendre @ericafenyo @lgharib I have just finished an initial version of the status screen. I will likely tweak it going forward, but wanted to get it out of the door for feedback.
I would encourage you to test it out, provide feedback and then update your versions of the apps. It is likely to make it easier to install correctly and to keep the app running correctly - e.g. solve the "iOS reset permissions" issue.
@ericafenyo this involves a significant expansion of the interface for the data collection library/plugin and a new UI screen. The new interface is fairly simple, mainly consisting of fixXXX
and isValidXXX
, but let me know if you have any questions.
@asiripanich I just pushed the new iOS version to TestFlight in case you want to take a look. Open to feedback on other status metrics to include.
@asiripanich The status screen opens from the profile. You should also experiment with turning various permissions off and seeing what happens š
...this involves a significant expansion of the interface for the data collection library/plugin and a new UI screen. The new interface is fairly simple, mainly consisting of fixXXX and isValidXXX, but let me know if you have any questions.
Ok I will test and let you know when I have questions. Thanks.
@asiripanich I just pushed the new iOS version to TestFlight in case you want to take a look. Open to feedback on other status metrics to include.
I will do that and report back today. :)
@shankari I like the new permission status screen. However, the scrolling issue is still there (we discussed it somewhere but I can't find the related issue).
@asiripanich can you elaborate more on the scrolling issue? Some context may help me find the related issue.
@asiripanich note also that the permission status screen is not only for onboarding. We also use it to track the status on an ongoing basis. If the user responds to the "XXX has been using your location in the background, do you want to continue?" with "no", then we will prompt the user that the app settings are incorrect, and open the app settings screen when they click on the notification so they can fix the issue easily.
I encourage you to experiment with turning location settings/permissions on/off, motion activity settings off, etc to see this behavior and whether it makes sense.
@PatGendre @ericafenyo @lgharib I have just finished an initial version of the status screen. I will likely tweak it going forward, but wanted to get it out of the door for feedback.
I would encourage you to test it out, provide feedback and then update your versions of the apps.
@shankari great ! Unfortunately we have updated our app 2 weeks ago and do not plan the next update until a few weeks ahead. But we will test the status screen again as soon as we can.
@asiripanich can you elaborate more on the scrolling issue? Some context may help me find the related issue.
I remember now that it was something I discussed with @atton16.
Expand to see a video
@asiripanich Is this on android or iOS?
@asiripanich Is this on android or iOS?
This was on my iPhone 13 Pro with iOS 14.3.
Alas, I don't have access to an iOS 14.3. The oldest simulator in my xcode is 14.5, and my test phones are all 12.x But in the iOS 14.5 simulator, I don't see the scrolling issue. Do you see this in the simulator as well? If so, it is easier to debug...
I'm actually not sure what the video is showing. Is it that you cannot access the "I accept" or that it takes a few tries to get there?
I haven't run it in the simulator yet.
I think it was the latter. However, I'm quite familiar with the app so I know that you need to scroll up and down a few times before it let you scroll to the end. Not sure if an average user would know how to resolve this.
Let me know if you can reproduce in the simulator.
It is primarily a question of prioritization - is getting the scrolling right more important than dealing with https://github.com/e-mission/e-mission-docs/issues/704, or https://github.com/e-mission/e-mission-docs/issues/698?
At long last, after clearing the decks with https://github.com/e-mission/e-mission-phone/pull/799 https://github.com/e-mission/e-mission-phone/pull/800 https://github.com/e-mission/e-mission-phone/pull/801 https://github.com/e-mission/e-mission-phone/pull/802
I'm ready to start the annual ritual of upgrading to the most recent version of everything. I will also plan to address https://github.com/e-mission/e-mission-docs/issues/678 and add silent push notifications to android as well to address https://github.com/e-mission/e-mission-docs/issues/677