-
Notifications
You must be signed in to change notification settings - Fork 34
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
⬆️ [phone] Upgrade to the latest everything + some RFC 🎉 #838
Comments
I looked into the android 12 changes during the last upgrade.
Changes to investigate further:
|
for iOS, apple has approved our app, but has raised some questions on other apps on the same platform. Potential changes:
|
@TTalex @lgharib @asiripanich @sebastianbarry @JGreenlee @jf87 @ericafenyo: I have some notes here on the changes required to support the new APIs and would like feedback on the user interface for the new changes:
|
Most recent versions of cordova seem to be:
Template has been updated; let's see if we need to change anything to match it https://cordova.apache.org/news/2021/10/31/template-release.html |
@asiripanich @TTalex @lgharib @ericafenyo @jf87 I plan to remove the following plugins, which are (to my knowledge) currently unused, and are getting more complex to maintain
|
@shankari Android: I've seen that other apps with similar needs (Life360 is a good example) do take the route of disabling battery optimization. It might just have to be that way. For android I see a few location priority options in the API - not sure what we currently use but if iOS: provisional notifications whenever possible, seems worth implementing. I can't comment much else bc I'm still learning what the app pushes and when Agreed that having two popups to get location permission is annoying - but either way we can't reduce the number of clicks to less than 2. All things considered I think the best route from a UX perspective is to just comply with apple's recommended flow. It will be what iOS users become used to as other apps adopt it. If they deny, we just send them back and and ask them to allow from the system prefs |
@JGreenlee I agree that there is an inherent cost of background location processing. Chapter 4 of my PhD thesis (https://www2.eecs.berkeley.edu/Pubs/TechRpts/2019/EECS-2019-180.pdf) explores various background sensing options in detail, and section 7.2 of my thesis performs a rigorous evaluation of of the power/accuracy tradeoff by using various different sensing regimes on identical phones. I would encourage you to read them first. Note that for stock android phones, we currently do not require users to disable background optimizations. For non-stock android phones, we may need to have users disable optimizations (e.g. https://dontkillmyapp.com/) but the instructions are different for each make and model. The question here is: "do we need to have users disable optimizations on stock android as well?". I would like to avoid doing that, because the fewer settings we ask users to modify, the better. If the foreground service (the persistent notification) is only rarely killed while the user is on a trip, then we don't need to ask users to change. If it is killed often, or if the foreground service is killed when the user removes the notification in android 13 (https://developer.android.com/guide/components/foreground-services#user-dismiss-notification), then we will have to ask users to change the setting. So to reiterate: Should we err on the side of better data collection by asking the user to change this additional setting, or should we err on the side of usability by not asking them to change the setting and hoping that the service is not killed very often? |
On the subject of plugins: By the way, one overall feedback I've got from the team is to try to reduce the interdependencies between plugins. And to try to isolate native parts as much as possible. This is linked to a specific need of "modulability", where only some parts of the app are used. On the subject of battery optimizations: On the subject of Android cloud backups: I hope that helps. |
|
Sounds good to me, thanks for the details. From my side, starting from scratch with new phones is what we're currently doing in our use cases. This has not been an issue for users as far as I know (note: the sample size is limited). |
After finishing up
|
Fixed some pending issues with the native + fixed CI issues. In the meanwhile, seeing if we can also lock down the version of ruby |
Next step, let's upgrade all the dependencies in
|
To the most recent versions e-mission/e-mission-docs#838 (comment)
Per e-mission/e-mission-docs#838 (comment) This is the first step in which I remove all the controls related to it ``` $ grep -ri notify www/js/control | wc -l 0 $ grep -ri notify www/templates/control/ | wc -l 0 ``` Testing done: Profile screen loads correctly
…or authentication This is the first step for removing support for other methods of authentication. This is as per the community decision in the issue: e-mission/e-mission-docs#838 (comment) Testing done: - Checked that the functions are not invoked from the UI ``` $ grep -r signIn www/js $ grep -r getJWT www/js $ grep -r launchPromptedAuth www/js $ git status ``` - Launched the app on android, no errors
As outlined in e-mission/e-mission-docs#838 (comment) - we are not using the ionic-deploy functionality any more. It was always unclear whether that constituted "code download" - it is not currently authorized by NREL cyber - having separate branches without a lot of oversight just led to a lot of divergent, throw-away code - NREL cyber will not allow us to deploy code written by others without review and merge anyway - so we are going to switch to a small number of features, checked into master as modules, and enabled/disabled via a simple static config file This is the last of the three cleanups in the issue. The other two were fixed in e-mission#925 and e-mission/cordova-jwt-auth#46 We can now start the real API upgrade
Turning off battery optimizations:
Should we use |
Difference between them:
Acceptable use cases include: https://developer.android.com/training/monitoring-device-state/doze-standby#exemption-cases
Going with |
Note that |
While trying to launch the optimization settings, running into this infinite loop
|
The chain is
So setActivityResultCallBack calls onActivityResult which calls setActivityResultCallback which calls onActivityResult and so on. I'm pretty sure that the issue is that I am setting the callback twice while setting it up - once in |
I can also use ADB to directly generate broadcasts to services/broadcast receiver. Note that this requires root access and will only work on images with
Calling initialize
We get that message about the foreground service started from the background, but it doesn't seem to affect the geofencing creation at least. Let's now exit the geofence and start location tracking
And we do get the locations read correctly
So it looks like it all works fine on Android 12 (the "Foreground service started from background" does not seem to be significant) Will kill the app on the physical device and verify |
It's great that we can send broadcast notifications to test, but it seems like that might be a problem if we refactor to a bound service. I have not been able to figure out how to make calls to a bound service using adb, but we can call an intent service like so on a rooted shell (using the googleapis without the play services)
We do have to figure out the exact format of the various intent services, though
|
Everything seems to be working properly |
Final check for an issue that was failing yesterday. In the background checks, we treat the optimization issue as a location failure and go to the start state. So when the user does turn on optimization, we are need to restart the FSM. Made that change and verified that it works. restart_after_optimization.mov |
At this point, I think that most of the high level changes are done. The pending issues are:
Neither of these sounds like a showstopper. I am going to push out a staging version with all the changes so far. |
This allows us to start foreground services from the background Which allows us to keep our original model and kick the can down the road e-mission/e-mission-docs#838 (comment) I have explored options to do it the right way, but am concerned about testing overhead given our short time frame. This change: - adds interface functions to check and fix battery optimizations - implements the check with the powermanager call - implements the fix by launching the optimization window Testing done: - Turn off the data optimizations - Kill the app (including the foreground service) using adb e-mission/e-mission-docs#838 (comment) - Send the initialize transition using adb - Foreground service is restarted e-mission/e-mission-docs#838 (comment) - Kill the app on a physical device using adb - Foreground service restarting in a few hours - Location tracking worked even without app being launched e-mission/e-mission-docs#838 (comment)
This is a companion commit to e-mission/e-mission-data-collection@60f6fa4 from e-mission/e-mission-data-collection#202 This allows us to start foreground services from the background Which allows us to keep our original model and kick the can down the road e-mission/e-mission-docs#838 (comment) I have explored options to do it the right way, but am concerned about testing overhead given our short time frame. This change: - adds interface functions to check and fix battery optimizations - implements the check with the powermanager call - implements the fix by launching the optimization window Testing done: - Turn off the data optimizations - Kill the app (including the foreground service) using adb e-mission/e-mission-docs#838 (comment) - Send the initialize transition using adb - Foreground service is restarted e-mission/e-mission-docs#838 (comment)
The `google_apis_playstore` images are signed and cannot be rooted If we want to send broadcast messages using adb for testing, we need root support e-mission/e-mission-docs#838 (comment) Add a couple of rootable system images as well so we can test appropriately
I can confirm that the iOS warning no longer appears during upload Pending tasks:
|
Ok, so this is turning out to be more complicated than I had thought. Checking the documentation on the original plugin; otherwise will have to fall back to the phonegap plugin |
I looked at the documentation on the original plugin, and I needed to call
I'm not going to spend time figuring out which element is not transparent and then making it be transparent (and then restoring when we stop scanning). Switching back to the old phonegap plugin instead |
We tried to migrate from `phonegap-plugin-barcodescanner` to the better supported `cordova-plugin-barcode-qrscanner`. However, I was not able to get the new plugin to work. The OS indicated that the app was accessing the camera, but we were not able to see the preview, even after I added a `show` to the code. e-mission/e-mission-docs#838 (comment) e-mission/e-mission-docs#838 (comment) There doesn't seem to be an option to file issues or to communicate with the creator. So let's fall back to the older `phonegap-plugin-barcodescanner` We restore the `android_change_compile_implementation` hook so that we can change the `compile` to `implementation` and get the plugin to compile. Bonus fix: Add the `NSCameraUsageDescription` via the config.xml so that we don't get a permission issue while running on iOS. Testing done: - Builds on both android and iOS - Scanning launches on both android and iOS - QR codes are scanned on both android and iOS
To be consistent with 2023 phone upgrades Related issue: e-mission/e-mission-docs#838 Related PR: e-mission/e-mission-phone#952 Since the rest of the app is not the same, I applied the changes by diffing the config.xml and package.json and the setup directory with the corresponding locations in the e-mission-phone repo Additional minor changes: + comment out the `configure_xml_and_json` step, since we don't have the cordovabuild versions in this release + change the package name to `edu.berkeley.eecs.emission.devapp` + bump up the versions Testing done: - devapp builds successfully - after making phone app changes, devapp displays phone app successfully
Related issue: e-mission/e-mission-docs#838 Related PR for cordovabuild: e-mission#952 Related PR for devapp: e-mission/e-mission-devapp#18 Main changes: - need to add "Ventura" to the list of releases to fix: ``` /Users/kshankar/e-mission/phone-rciti-branch/node_modules/macos-release/index.js:27 const [name, version] = nameMap.get(release); ^ TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator)) at macosRelease (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/macos-release/index.js:27:26) at osName (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/os-name/index.js:21:18) at new Insight (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/insight/lib/index.js:37:13) at Object.<anonymous> (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova/src/telemetry.js:26:15) at Module._compile (node:internal/modules/cjs/loader:1246:14) at Module._extensions..js (node:internal/modules/cjs/loader:1300:10) at Module.load (node:internal/modules/cjs/loader:1103:32) at Module._load (node:internal/modules/cjs/loader:942:12) at Module.require (node:internal/modules/cjs/loader:1127:19) at require (node:internal/modules/helpers:112:18) ``` - we do this by copying over a file that includes Ventura per https://stackoverflow.com/questions/68318289/ionic-fails-building-in-macos-12-monterey - I tried to do this in a more principled fashion by upgrading to the most recent version of cordova - but we are already at the most recent version of cordova, and it still doesn't work - I then tried to compare it to the same file in the cordovabuild version and it was the same ``` $ grep Ventura ~/e-mission/native_code_upgrade/node_modules/macos-release/index.js | wc -l 0 ``` - on further invesigation, it turns out that the issue is not that the more recent version of cordova has a newer version of the macos-release library, but that it doesn't use `Insight` (at least by default), which is the module that calls it - phonegap, which we need for `phonegap serve`, does ``` $ grep -r "new Insight" node_modules/ node_modules//cordova/node_modules/insight/readme.md:const insight = new Insight({ node_modules//cordova/node_modules/insight/readme.md:const insight = new Insight({ node_modules//cordova/node_modules/insight/lib/push.js: const insight = new Insight(message); node_modules//insight/readme.md:const insight = new Insight({ node_modules//insight/readme.md:const insight = new Insight({ node_modules//insight/lib/push.js: const insight = new Insight(msg); node_modules//phonegap/node_modules/cordova/src/telemetry.js:var insight = new Insight({ node_modules//phonegap/lib/cli/analytics.js:var insight = new Insight({ ``` - so there is really no alternative to hot-patching the file - add the cordova `browser` platform to fix: ``` CordovaError: No platforms added to this project. Please use `phonegap platform add <platform>`. at Object.preProcessOptions (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/util.js:313:15) at /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/prepare.js:49:40 at process.processTicksAndRejections (node:internal/process/task_queues:95:5) ``` - incidentally, if we don't add the platform, cordova tries to add it itself, and that completely messes up the `node_modules` There are 720 packages ``` $ ls -al node_modules/ | wc -l 720 ``` we try to add the browser, which fails ``` Using cordova-fetch for cordova-browser@^6.0.0 (node:24473) Warning: Accessing non-existent property 'browser' of module exports inside circular dependency (Use `node --trace-warnings ...` to show where the warning was created) Failed to fetch platform cordova-browser@^6.0.0 Probably this is either a connection problem, or platform spec is incorrect. Check your connection and platform name/version/URL. Could not determine package name from output: added 24 packages, changed 1 package, and audited 131 packages in 10s ``` we end up with 111 packages, and `phonegap` is uninstalled as well ``` $ ls -al node_modules | wc -l 111 ``` - and the `phonegap` command is not found either - add shelljs to the dependencies to fix ``` [warning] Unable to load PlatformApi from platform. Error: Cannot find module 'shelljs' [warning] Require stack: [warning] - /Users/kshankar/e-mission/phone-rciti-branch/platforms/browser/cordova/Api.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/util.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/platforms/platforms.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/plugman/install.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/plugman/plugman.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/cordova-lib.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova/cordova.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/cordova/index.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/phonegap/cordova.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/phonegap.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/main.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/cli.js [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/bin/phonegap.js ``` Testing done: ``` $ rm -rf node_modules $ bash setup/setup_serve.sh $ npm run serve ``` the serve process starts up and the devapp is able to connect to it
At long last, after clearing the decks with the label changes
I'm ready to start the annual ritual of upgrading to the most recent version of everything.
I will also plan to make the experimental trip start feature the default #785
and add silent push notifications to android to see if it can address background operation issues #677
@TTalex @asiripanich @lgharib
The text was updated successfully, but these errors were encountered: