Control showing of notifications on iOS#2156
Control showing of notifications on iOS#2156rajveermalviya wants to merge 8 commits intozulip:mainfrom
Conversation
dc78826 to
7b54648
Compare
|
This is ready for review now! |
|
In the pariring call today, Greg metioned that there are some commits here that could be squashed — I'll do that tommorow. (Namely: |
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, this is exciting!! Comments below, from reading the commits. I haven't yet done manual testing.
| <key>NSExtensionPointIdentifier</key> | ||
| <string>com.apple.usernotifications.service</string> | ||
| <key>NSExtensionPrincipalClass</key> | ||
| <string>$(PRODUCT_MODULE_NAME).NotificationService</string> |
There was a problem hiding this comment.
ios: Add Notification service app extension target via Xcode
Commit-message nit: I think you mean "NotificationService" or "notification service", not "Notification service"? Depending on whether or not you want to refer to the specific name that appears in the code.
| // NotificationService.swift | ||
| // NotificationService | ||
| // | ||
| // Created by Rajesh Malviya on 18/02/26. |
ios/Runner.xcodeproj/project.pbxproj
Outdated
| INFOPLIST_KEY_CFBundleDisplayName = NotificationService; | ||
| INFOPLIST_KEY_NSHumanReadableCopyright = ""; | ||
| IPHONEOS_DEPLOYMENT_TARGET = 26.2; | ||
| IPHONEOS_DEPLOYMENT_TARGET = 15.0; |
There was a problem hiding this comment.
The "iOS Deployment Target" of "Runner" is something we mention in a comment at the top of ios/Podfile:
# This should match the iOS Deployment Target
# (in Xcode, that's in project > Runner > Info)
# and MinimumOSVersion in ios/Flutter/AppFrameworkInfo.plist.
platform :ios, '15.0'Should that comment grow a mention of this new thing that needs to be set to the same value? I believe this is where we declare the minimum supported iOS version, so it's definitely something we expect to change in the future.
ios/Runner.xcodeproj/project.pbxproj
Outdated
| ); | ||
| runOnlyForDeploymentPostprocessing = 0; | ||
| shellPath = /bin/sh; | ||
| shellScript = "/bin/sh \"$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh\" build\n"; |
There was a problem hiding this comment.
ios: In Xcode, add Run Script build phase for the extension target
Hmm, just above this B378A52C2F45C31A0031EFA1 /* ShellScript */ = { block is a nearly identical block 9740EEB61CF901F6004384FC /* Run Script */ = { which has the same shellScript value. Are they both needed? I wonder if the problem was just that the old way of expressing the desired build phase wasn't working.
There was a problem hiding this comment.
The previous 9740EEB61CF901F6004384FC /* Run Script */ block is referenced by the Runner target, and B378A52C2F45C31A0031EFA1 /* ShellScript */ is being added by this workaround/step for the NotificationService target (Not sure why the (commented) name generated here is different, in Xcode both phases, under TARGETS > [Runner/NotificationService] > Build Phases (tab) > Run Script, are called "Run Script".
lib/notifications/ios_service.dart
Outdated
| // `assert()` statements are not working in the extension even when running in | ||
| // the debug mode. But fortunately `kDebugMode` does correctly return true in | ||
| // the debug mode and false in the release mode. So, use this helper instead of | ||
| // `debugLog` from `log.dart`. |
There was a problem hiding this comment.
Maybe worth a // TODO debug asserts not working on this.
| // current thread is being polled by the system. Which is not the case in | ||
| // Notification Service Extension, so here we manually poll the event loop. | ||
| // See discussion: | ||
| // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/Running.20Dart.20code.20in.20iOS.20Notification.20Service.20Extension/with/2370721 |
There was a problem hiding this comment.
Will there be a change to be made here if a PR like flutter/flutter#181645 lands? If so, let's add a TODO. (I'm not familiar with all the details but I found that PR by following links.)
lib/main.dart
Outdated
| // This library defines the Dart entrypoint function for headless FlutterEngine | ||
| // used in iOS Notification Service Extension. We need to import it here to | ||
| // avoid it being treeshaked during build process. | ||
| // ignore: unused_import | ||
| import 'notifications/ios_service.dart'; |
There was a problem hiding this comment.
I wonder if this is the standard way to prevent something from being treeshaken, or if there's some config in the build process for this?
There was a problem hiding this comment.
I believe @pragma('vm:entry-point') decorator prevents the top-level function from being tree-shaked but AIUI there's no way to prevent the whole unimported dart library from being tree-shaked. (I realize tree-shaked is not the correct term, it's more that the dart library is not imported anywhere so it's not included in the build.)
| guard let bestAttemptContent = bestAttemptContent else { | ||
| return | ||
| } |
There was a problem hiding this comment.
What is this early return doing? I'm not sure if this needs a comment or if I just need to learn more Swift. 🙂
| switch result { | ||
| case .success(let mutatedNotificationContent): | ||
| os_log("didReceivePushNotification: success") | ||
| bestAttemptContent.title = mutatedNotificationContent.title | ||
| if let body = mutatedNotificationContent.body { | ||
| bestAttemptContent.body = body | ||
| } | ||
| contentHandler(bestAttemptContent) | ||
|
|
||
| case .failure(let error): | ||
| os_log("didReceivePushNotification: failed: error=\(error.localizedDescription)") | ||
| contentHandler(bestAttemptContent) | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, this switch/case indentation looks odd to me.
pigeon/ios_notifications.dart
Outdated
| final Map<Object?, Object?> payload; | ||
| } | ||
|
|
||
| class MutatedNotificationContent { |
There was a problem hiding this comment.
I wonder if "mutated" is the name we want here, or if there's something more specific we want to emphasize? The goal is to make "better"/"improved" notification content, right, not just different/changed content. 😛 #1265 says "improve the wording and formatting of notifications".
There was a problem hiding this comment.
I had used MutatedNotificationContent name to correspond to UNMutableNotificationContent. Changed to "ImprovedNotificationContent".
lib/model/binding.dart
Outdated
| Future<void> toggleWakelock({required bool enable}); | ||
|
|
||
| /// The iOS App Group identifier specified in the Xcode config. | ||
| static const iosAppGroupIdentifier = 'group.zulip.test'; |
There was a problem hiding this comment.
This will eventually not say "test", right? Do we need a comment on the consequences of changing this in future (e.g. data loss?)
|
|
||
| IosNotifFlutterApi iosNotifFlutterApi() => TestZulipBinding.instance.iosNotifFlutterApi!; | ||
|
|
||
| test('smoke', () async { |
There was a problem hiding this comment.
This needs a addTearDown(testBinding.reset);, I think.
| IosNotifFlutterApi iosNotifFlutterApi() => TestZulipBinding.instance.iosNotifFlutterApi!; | ||
|
|
||
| test('smoke', () async { | ||
| IosNotificationService.init(); |
There was a problem hiding this comment.
IosNotificationService has mutable state (isExecutingInExtension) which we don't want to leak between tests; probably it should have a reset method that tests call on teardown?
lib/model/store.dart
Outdated
|
|
||
| /// The file path to use for the app database. | ||
| static Future<File> _dbFile() async { | ||
| if (defaultTargetPlatform == TargetPlatform.iOS) return _maybeCopyIosDbFile(); |
There was a problem hiding this comment.
Below this new early return, the comment starting with "What directory should we use?" now looks confusing:
- It's a long comment, so it seems like the question it's asking doesn't have a simple answer.
- …But the question has apparently been answered anyway, at least for iOS.
- The answer still includes "on iOS" cases.
| if (IosNotificationService.isExecutingInExtension) { | ||
| // In iOS Notification Service Extension we can't copy the file, as the | ||
| // extension will not have access to main app target's filesystem. | ||
| // We also can't return the App Group container path here when running in | ||
| // the extension, because it will cause creation of new empty DB file, | ||
| // which then the main app target will prefer because of the code above. | ||
| // So, abort if we are running in iOS Notification Service Extension and | ||
| // the database is not already migrated to the App Group container. | ||
| throw Exception('The database file in iOS app group container doesn\'t ' | ||
| 'exists, and when running in iOS Notification Service Extension the ' | ||
| 'database file can\'t be migrated'); | ||
| } |
There was a problem hiding this comment.
I think some of this comment and error message can be removed/simplified if we make the dartdoc more explicit about why the things it does are needed.
Can we make the dartdoc clear that
- This is essentially a one-time migration that enables the extension to work. The extension won't work without it because [etc.].
- The migration can only be run by the app, not the app extension, because [etc.].
- If this runs in the app extension before the migration has been done, [etc.] happens.
This reminds me of the data migration from the legacy app, right? Though less complex, because we're just straightforwardly copying a file instead of restructuring data and so on. Still, are there details from that former migration that would be helpful to think about here? (Like how to organize the code?) Do we need to set a flag somewhere tracking whether the migration is complete, like we do for other migrations? Would some test coverage be feasible? Etc. It would be embarrassing to cause data loss for people by mishandling a migration somehow. 🙂
There was a problem hiding this comment.
Added the required context to the dartdoc.
Do we need to set a flag somewhere tracking whether the migration is complete, like we do for other migrations?
I don't think this is needed because it is only a single operation (copying the file), and would only be executed from Runner target, so no race condition between Runner and NotificationService.
Would some test coverage be feasible
Not sure how best to unit test this type of file copy migrations, if we did mocks for file system api that it would not really be testing actual code.
It would be embarrassing to cause data loss for people by mishandling a migration somehow.
Agreed.
lib/model/store.dart
Outdated
| 'to iOS app group container path: $containerDbFile')); | ||
| await nonContainerDbFile.copy(containerDbFile.path); | ||
|
|
||
| // TODO do we want to delete nonContainerDbFile here? |
There was a problem hiding this comment.
This seems like the kind of detail that we'd want to have a plan for, even if we don't do it right now, before merging this migration.
09ffdb5 to
3fbbe3b
Compare
gnprice
left a comment
There was a problem hiding this comment.
Thanks! Here's a handful of comments from an initial skim. (I realize this hasn't been marked as fully revised for Chris's comments above.)
| @@ -0,0 +1,239 @@ | |||
| // Autogenerated from Pigeon (v26.1.7), do not edit directly. | |||
There was a problem hiding this comment.
This new generated file should be added to .gitattributes so that it gets collapsed in git diff.
(Better: a pattern that covers it and similar files.)
lib/model/store.dart
Outdated
| // TODO rename the group identifier. | ||
| const iosAppGroupIdentifier = 'group.zulip.test'; |
There was a problem hiding this comment.
Maybe group.zulip.app? Seems like this group is basically "the Zulip app, as a whole".
lib/model/store.dart
Outdated
| if (defaultTargetPlatform == TargetPlatform.iOS) { | ||
| file = await _maybeCopyIosDbFile(); | ||
| } else { | ||
| file = await _dbFile(); |
There was a problem hiding this comment.
This complication seems like something that's not part of this caller's job to worry about. It should go inside the method this code is calling, so either _dbFile or some new method.
lib/model/store.dart
Outdated
| 'database file can\'t be migrated'); | ||
| } | ||
|
|
||
| final nonContainerDbFile = await _dbFile(); |
There was a problem hiding this comment.
Echoing and expanding a bit on what Chris said at #2156 (comment) : definitely take the code in _dbFile as open to revision with the changes you're making here. The filename that the existing logic there produces is no longer really "the DB file" for the app on iOS; so either the method's name, or its logic, or both, should change. Likely the comment in it should then change too. The goal is for the new code after your changes to have a logic and structure that's clear to the reader, even without them having seen what the code looked like previously.
948d69c to
8e328fb
Compare
|
Thanks for the reviews @chrisbobbe, @gnprice! Pushed a new revision, PTAL. |
|
Would you try the following:
Possibly that step 4 should be changed. It seems odd to add the pre-action only on the scheme dedicated to building the app extension in isolation. In our case, at least, I think we can just delete that (the auto-generated "NotificationService" scheme)… Our default scheme, "Runner", which has become a confusing name, builds both the "Runner" target and the "NotificationService" target, which is probably always what we want. The goal is to avoid running the heavier See flutter/flutter#146256 (which is basically the same solution to this problem as it appears in a different context):
|
|
If that works, then try reverting step 3 of https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter :
That seems to be a security feature that applies to script build phases: you limit the script's permissions to specific input and output files. But the instructions don't ask you to add a script build phase, and if my suggestion above worked, you won't have added any. I don't think there's a version of this feature that applies to a pre-action script. In the UI to configure a pre-action script, there's no way to specify input/output files. |
|
For step 5 of https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter:
would you make a separate commit for that (or maybe squash it into the commit that creates the new target)? It's an interesting change because:
Probably the app extension's Flutter behavior depends on some of that config. But we might find out later that these defaults aren't a perfect fit for app extensions; it won't be top-of-mind when Flutter makes changes to it. |
|
|
This moves IPHONEOS_DEPLOYMENT_TARGET config to Zulip.xcconfig, instead of it being specified for different Xcode config sections. This vastly simplifies changing the iOS deployment target later, allowing us to change a single variable, instead of navigating through the Xcode UI to change for multiple targets (two currently; Runner and RunnerTests). See: zulip#2156 (comment)
This carries out the steps 3-4 from guide for "Add iOS app extension" section in the docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#add-extension Then ran 'swift-format' over the generated Swift code. (In the Xcode menu bar, Editor -> Structure -> Format file with 'swift-format'.) Finally, this also carries out step 5 from "Open a Flutter app in an iOS app extension" section in the docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter which shares the build configurations between extension target and Runner target. Allowing the extension target to: - Inherit the IPHONEOS_DEPLOYMENT_TARGET config value, from Zulip.xcconfig which {Debug,Release}.xcconfig imports. - Also it pulls in some build config specified by Flutter, including some CocoaPods-related stuff; see for example ios/Flutter/Release.xcconfig. See: zulip#2156 (comment)
…xtension Implement initial setup for headless FlutterEngine for running Dart code in NotificationService extension. This carries out the step 5 from guide for "Open a Flutter app in an iOS app extension" section in the docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter This carries out the steps 2-3 from guide for "Register plugins" section in the docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#register-plugins Additionally adds the following pre-action script for the Runner scheme: /bin/sh "$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh" prepare This script copies the Flutter framework to a place where Xcode will find it when building the NotificationService target. See discussion: zulip#2156 (comment)
8e328fb to
3bb22d2
Compare
Fixes most of zulip#1764, namely for Android. Fixes zulip#322. This will cause the app to start receiving and displaying end-to-end encrypted notifications (zulip#1764), on Android, when the server is new enough to support the feature. The new protocol also fixes zulip#322, deduping sending the token to the server, by design. This commit also stops showing any notifications we receive over the legacy plaintext protocol when E2EE notifications are supported. This avoids duplicates. It potentially causes a brief interruption in getting notifications when either client or server is first upgraded to support the new protocol. The remaining required piece before zulip#1764 is complete is to implement it on iOS too. I believe that will be a fairly straightforward extension of this change (and all the changes that led up to it) together with zulip#2156, which gives us control over iOS notifications in the first place (zulip#1265).
ios/Flutter/Zulip.xcconfig
Outdated
| // TODO change group name | ||
| ZULIP_APP_GROUP_IDENTIFIER=group.zulip.test |
There was a problem hiding this comment.
I've just registered a group group.com.zulip.app (much like we were saying on a call last week). Let's use that.
gnprice
left a comment
There was a problem hiding this comment.
Thanks @rajveermalviya for building this, and @chrisbobbe for the detailed previous reviews! Here's a round of comments. I've read the first 5 commits:
386b162 ios [nfc]: Extract setting IPHONEOS_DEPLOYMENT_TARGET to Zulip.xcconfig
f19211b ios [nfc]: Extract setting DEVELOPMENT_TEAM to Zulip.xcconfig
c2b04af ios [nfc]: In Xcode, run swift-format
2379cba ios: Add NotificationService app extension target via Xcode
18cc590 notif ios: Initialize headless FlutterEngine in NotificationService extension
Still ahead are the remaining 3 commits:
62c1bcd ios: Implement ability to intercept APNs push notifications from Dart
4a31bbc TODO(group name) ios: Add both Runner and extension target to the same App Group
440e2e6 store ios: Migrate database file to iOS App Group container path
| import Flutter | ||
| import UserNotifications | ||
|
|
||
| class NotificationService: UNNotificationServiceExtension { |
There was a problem hiding this comment.
The commit message says:
This carries out the steps 3-4 from guide for
"Add iOS app extension" section in the docs:
https://docs.flutter.dev/platform-integration/ios/app-extensions#add-extension
Those instructions call for a "Share Extension", though. This was presumably from a variation choosing "Notification Service Extension".
| override func didReceive( | ||
| _ request: UNNotificationRequest, | ||
| withContentHandler contentHandler: @escaping (UNNotificationContent) -> Void | ||
| ) { |
There was a problem hiding this comment.
Then ran 'swift-format' over the generated Swift code.
(In the Xcode menu bar, Editor -> Structure -> Format file
with 'swift-format'.)
When I try this, it formats everything at 4-space indents. (Which also causes this parameter's declaration to wrap onto two lines.) Is there a setting somewhere that controls that? Can we check that into the repo so it's shared?
| CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)"; | ||
| ENABLE_USER_SCRIPT_SANDBOXING = YES; | ||
| GCC_C_LANGUAGE_STANDARD = gnu17; | ||
| GENERATE_INFOPLIST_FILE = YES; | ||
| INFOPLIST_FILE = NotificationService/Info.plist; | ||
| INFOPLIST_KEY_CFBundleDisplayName = NotificationService; | ||
| INFOPLIST_KEY_NSHumanReadableCopyright = ""; | ||
| LD_RUNPATH_SEARCH_PATHS = ( | ||
| "$(inherited)", | ||
| "@executable_path/Frameworks", | ||
| "@executable_path/../../Frameworks", | ||
| ); | ||
| LOCALIZATION_PREFERS_STRING_CATALOGS = YES; | ||
| MARKETING_VERSION = "$(FLUTTER_BUILD_NAME)"; |
There was a problem hiding this comment.
When I try reproducing the steps in this commit message:
2379cba ios: Add NotificationService app extension target via Xcode
I get some lines different here, including:
CURRENT_PROJECT_VERSION = 1;
IPHONEOS_DEPLOYMENT_TARGET = 18.2;
MARKETING_VERSION = 1.0;
I see some discussion in the thread above about fixing two of those to be $(FLUTTER_BUILD_NUMBER) and $(FLUTTER_BUILD_NAME) and the third to be absent (and implicitly inherited). All those fixes sound good to me. Did you do them by clicking around somewhere in the Xcode GUI, or something else?
Even if it's just "I clicked around in the Xcode GUI to adjust these", that fact would be helpful to mention in the commit message, with these three names of variables you adjusted. Since the values you chose are here in the diff, that's enough for someone to reproduce a similar change in the future — for example, for one of us to follow when adding a ShareExtension, when we go to implement that feature.
| // Modify the notification content here... | ||
|
|
There was a problem hiding this comment.
This comment was in the template that Xcode used for this file. It's useful in that context as a guide to someone beginning to write the extension. Now that you've written the extension, though, it doesn't seem helpful: the code below does the modifying.
| <BuildAction | ||
| parallelizeBuildables = "YES" | ||
| buildImplicitDependencies = "YES"> | ||
| <PreActions> |
There was a problem hiding this comment.
The commit message says:
Additionally adds the following pre-action script for the Runner scheme:
/bin/sh "$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh" prepare
This script copies the Flutter framework to a place where Xcode
will find it when building the NotificationService target.
See discussion: https://github.com/zulip/zulip-flutter/pull/2156#issuecomment-4008936845
This is somewhat mysterious about just how one goes about adding a pre-action script to a scheme, though. It also doesn't make clear what prompted us to take such a step.
After following the link and reading there, I think the key points here are:
- We're doing step 4 of https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter .
- But we're doing a variation that's cleaner: instead of adding it to the new extension's scheme, we're adding to the main Runner scheme.
- See that comment for discussion.
lib/notifications/ios_service.dart
Outdated
| final iosNotifFlutterApiImpl = _IosNotifFlutterApiImpl(); | ||
| ZulipBinding.instance.setupIosNotifFlutterApi(iosNotifFlutterApiImpl); |
lib/notifications/ios_service.dart
Outdated
| return ImprovedNotificationContent( | ||
| title: parsed.title, | ||
| body: parsed.body); |
lib/notifications/ios_service.dart
Outdated
| // debug mode. But fortunately `kDebugMode` does correctly return true in debug | ||
| // mode and false in the release mode. So, use this helper instead of | ||
| // `debugLog` from `log.dart`. | ||
| // TODO debug asserts not working |
There was a problem hiding this comment.
Let's mark that this is presumably some sort of upstream issue:
| // TODO debug asserts not working | |
| // TODO(upstream) debug asserts not working |
|
|
||
| @pragma('vm:entry-point') | ||
| void iosNotificationServiceMain() { | ||
| WidgetsFlutterBinding.ensureInitialized(); |
There was a problem hiding this comment.
Let's have this explicitly do nothing outside iOS, something like this:
| WidgetsFlutterBinding.ensureInitialized(); | |
| void iosNotificationServiceMain() { | |
| if (defaultTargetPlatform != TargetPlatform.ios) throw Error(); | |
| WidgetsFlutterBinding.ensureInitialized(); |
That way the Android build can tree-shake everything downstream of this.
There was a problem hiding this comment.
(In principle the effect we want here is to make the @pragma('vm:entry-point') conditional on iOS, because the truth is that this will only be an entry point from the VM on iOS. There's no mechanism to express that for annotations, though, at least not an obvious simple one. So we can express a similar point this way instead.)
lib/notifications/ios_service.dart
Outdated
| // `assert()` statements are not working in the extension even when running in | ||
| // debug mode. But fortunately `kDebugMode` does correctly return true in debug | ||
| // mode and false in the release mode. So, use this helper instead of | ||
| // `debugLog` from `log.dart`. |
There was a problem hiding this comment.
nit: "in release mode", similarly to #2156 (comment)
This moves IPHONEOS_DEPLOYMENT_TARGET config to Zulip.xcconfig, instead of it being specified for different Xcode config sections. This vastly simplifies changing the iOS deployment target later, allowing us to change a single variable, instead of navigating through the Xcode UI to change for multiple targets (two currently; Runner and RunnerTests). See: zulip#2156 (comment)
Similar to previous commit, this moves DEVELOPMENT_TEAM config to Zulip.xcconfig, instead of it being specified for different Xcode config sections.
In the Xcode menu bar, Editor -> Structure -> Format file with 'swift-format'.
Carries out the steps 3-4 from "Add iOS app extension" section in the Flutter docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#add-extension In step 3, the Flutter docs walk through adding a "Share Extension" target, we instead add a "Notification Service Extension" target, as described in Apple's documentation: https://developer.apple.com/documentation/usernotifications/modifying-content-in-newly-delivered-notifications#Add-a-service-app-extension-to-your-project The Swift file generated in step 3 was formatted using `swift-format` (In Xcode menu bar, Editor -> Structure -> Format file with 'swift-format'). Also carries out step 5 from the "Open a Flutter app in an iOS app extension" section in the Flutter docs: https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter this will share the build configurations between the extension target and the Runner target, allowing the extension target to: - Inherit the IPHONEOS_DEPLOYMENT_TARGET config value, from Zulip.xcconfig which {Debug,Release}.xcconfig imports. - Also it pulls in some build config specified by Flutter, including some CocoaPods-related stuff; see for example ios/Flutter/Release.xcconfig. See discussion: zulip#2156 (comment) Also made following other changes to Xcode build settings for the generated NotificationService target: - Deleted "iOS Deployment Target" config specified for target, so that when building the target, the inherited value from Zulip.xcconfig will be used. By navigating to project navigator (View > Navigators > Project) > NotificationService under TARGETS > Build Settings (tab) > Deployment, then pressed the Delete key on keyboard after selecting "iOS Deployment Target". - Updated the versioning information to match the Runner target. By navigating to project navigator (View > Navigators > Project) > NotificationService under TARGETS > Build Settings (tab) > Versioning, and changing the following values: * Changed "Current Project Version" to "$(FLUTTER_BUILD_NUMBER)" (in Xcode text field, typed without the double-quotes), where the generated value was "1". * Changed "Marketing Version" to "$(FLUTTER_BUILD_NAME)" (same as previous one, no double-quotes), where the default value was "1.0". * Changed "Versioning System" to "Apple Generic", where the default was set to "None". See discussion: zulip#2156 (comment)
…xtension Implement initial setup of a headless FlutterEngine for running Dart code in the NotificationService extension. This involves some Swift code to start the engine and drive its event loop, and a trivial Dart function to serve as the entry point. It also involves some build setup. For the build setup, this carries out the steps 2-3 from the "Register plugins" section in Flutter's guide to iOS app extensions: https://docs.flutter.dev/platform-integration/ios/app-extensions#register-plugins and adds the following pre-action script for the Runner scheme: /bin/sh "$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh" prepare That script copies the Flutter framework to a place where Xcode will find it when building the NotificationService target. See discussion: zulip#2156 (comment)
9e4f78d to
020b68f
Compare
…xtension Implement initial setup of a headless FlutterEngine for running Dart code in the NotificationService extension. This involves some Swift code to start the engine and drive its event loop, and a trivial Dart function to serve as the entry point. It also involves some build setup. For the build setup, this carries out the steps 2-3 from the "Register plugins" section in Flutter's guide to iOS app extensions: https://docs.flutter.dev/platform-integration/ios/app-extensions#register-plugins and adds the following pre-action script for the Runner scheme: /bin/sh "$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh" prepare by following same steps listed under step 4 in "Open a Flutter app in an iOS app extension" section in Flutter's guide to iOS app extensions: https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter but for the Runner scheme instead of the extension scheme. And also selected the Runner target instead of the extension target for the "Provide build settings" drop-down list. That script copies the Flutter framework to a place where Xcode will find it when building the NotificationService target. See discussion: zulip#2156 (comment)
020b68f to
b186d17
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
b186d17 to
314283a
Compare
gnprice
left a comment
There was a problem hiding this comment.
Thanks for the revision! This all looks good, except one comment below. And then I still have to read those last 3 commits:
0982024 ios: Implement ability to intercept APNs push notifications from Dart
36fa1fb ios: Add both Runner and NotificationService target to the same app group
314283a store ios: Migrate database file to iOS App Group container path
| CODE_SIGN_ENTITLEMENTS = NotificationService/NotificationService.entitlements; | ||
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)"; | ||
| ENABLE_USER_SCRIPT_SANDBOXING = YES; |
314283a to
437ba9e
Compare
…xtension Implement initial setup of a headless FlutterEngine for running Dart code in the NotificationService extension. This involves some Swift code to start the engine and drive its event loop, and a trivial Dart function to serve as the entry point. It also involves some build setup. For the build setup, this carries out the steps 2-3 from the "Register plugins" section in Flutter's guide to iOS app extensions: https://docs.flutter.dev/platform-integration/ios/app-extensions#register-plugins and adds the following pre-action script for the Runner scheme: /bin/sh "$FLUTTER_ROOT/packages/flutter_tools/bin/xcode_backend.sh" prepare by following same steps listed under step 4 in "Open a Flutter app in an iOS app extension" section in Flutter's guide to iOS app extensions: https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter but for the Runner scheme instead of the extension scheme. And also selected the Runner target instead of the extension target for the "Provide build settings" drop-down list. That script copies the Flutter framework to a place where Xcode will find it when building the NotificationService target. See discussion: zulip#2156 (comment) We are skipping the step 3 of "Open a Flutter app in an iOS app extension": https://docs.flutter.dev/platform-integration/ios/app-extensions#creating-app-extension-uis-with-flutter because disabling user script sandboxing is only needed if we had added a "Run Script" build phase without specifying input and output files. It seems it is unnecessary for the pre-action script because Xcode UI for adding the script doesn't have the options for specifying input and output files. See discussion: zulip#2156 (comment)
Fixes zulip#1265. This uses Pigeons's handy `@FlutterApi` API to define an interface method that allows passing arguments from Swift to Dart and also get a return value back from Dart to Swift.
…roup This is required to share any files like database between both app (Runner) and the service extension targets.
This will allow the Dart code executing in the notification service extension to access the same database file as the main app target.
437ba9e to
a857dcd
Compare
|
Updated the commit message for 6938dbd |
Fixes most of zulip#1764, namely for Android. Fixes zulip#322. This will cause the app to start receiving and displaying end-to-end encrypted notifications (zulip#1764), on Android, when the server is new enough to support the feature. The new protocol also fixes zulip#322, deduping sending the token to the server, by design. This commit also stops showing any notifications we receive over the legacy plaintext protocol when E2EE notifications are supported. This avoids duplicates. It potentially causes a brief interruption in getting notifications when either client or server is first upgraded to support the new protocol. The remaining required piece before zulip#1764 is complete is to implement it on iOS too. I believe that will be a fairly straightforward extension of this change (and all the changes that led up to it) together with zulip#2156, which gives us control over iOS notifications in the first place (zulip#1265).
gnprice
left a comment
There was a problem hiding this comment.
OK, and here's a review of the main commit:
9265eaf ios: Implement ability to intercept APNs push notifications from Dart
Still up for me to read:
f034f9b ios: Add both Runner and NotificationService target to the same app group
a857dcd store ios: Migrate database file to iOS App Group container path
| // Adapted from: https://github.com/flutter/flutter/blob/65b1ec407/engine/src/flutter/fml/platform/darwin/message_loop_darwin.mm#L44-L62 | ||
| let kDistantFuture = 1.0e10 | ||
| while loopRunning { | ||
| let result = CFRunLoopRunInMode(.defaultMode, kDistantFuture, true) | ||
| if result == .stopped || result == .finished { | ||
| loopRunning = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Why does this loop get revised to a different form in this commit:
9265eaf ios: Implement ability to intercept APNs push notifications from Dart
after being introduced in this commit?
6938dbd notif ios: Initialize headless FlutterEngine in NotificationService extension
It looks like there are a couple of substantive changes in behavior in that revision, too:
- a result of
.timedOutwould stop the loop in the initial version, but continue it in the new version; - the
seconds:argument is 1 in the initial version, but 1e10 in the new version.
Should the loop have the later form from the beginning? How did you determine what the right answer was for those two details? (It looks like the linked code this was adapted from matches the later form; was there a source for the initial form?)
| class IosNotificationService { | ||
| const IosNotificationService._(); | ||
|
|
There was a problem hiding this comment.
nit: since the point is that this never gets instantiated, make that more explicit with abstract class:
| class IosNotificationService { | |
| const IosNotificationService._(); | |
| abstract class IosNotificationService { |
| final body = alertData['body'] as String?; | ||
| return _ApnsPayload._(title: title, body: body); |
There was a problem hiding this comment.
This doesn't end up having any visible effect, right? It just sets the title and body to what they already were.
That's fine for completing #1265, and as an intermediate state toward #1764 and the other things that #1265 enables. But it's kind of confusing when reading this code, because it's a whole lot of apparatus that turns out to (currently) do nothing. So let's make it explicit with a comment.
| factory _ApnsPayload.parseApnsPayload(Map<Object?, Object?> payload) { | ||
| if (payload case { | ||
| "aps": { | ||
| "alert": { | ||
| "title": final String title, | ||
| } && final alertData, | ||
| }, | ||
| }) { | ||
| final body = alertData['body'] as String?; |
There was a problem hiding this comment.
I don't love this way of parsing. The payload here is JSON; our normal way of parsing JSON is with package:json_serializable, and I think that works out more cleanly. (Among other things, it would naturally treat the optional body and required title in a symmetrical way.)
It seems like this whole _ApnsPayload class is basically temporary, though, as a scaffold to demonstrate #1265 which gets torn down when #2230 implements #1764. So I guess we can leave it.
| _debugLog("dart: _IosNotifFlutterApiImpl.didReceivePushNotification"); | ||
| _debugLog("dart: content.payload=${jsonEncode(content.payload)}"); | ||
|
|
||
| final parsed = _ApnsPayload.parseApnsPayload(content.payload); |
There was a problem hiding this comment.
As long as we're doing this parsing in a throwaway fashion, though, I think we can do it more simply:
| final parsed = _ApnsPayload.parseApnsPayload(content.payload); | |
| final apsData = content.payload['aps'] as Map<Object?, Object?>; | |
| final alertData = apsData['alert'] as Map<Object?, Object?>; | |
| final title = alertData['title'] as String; | |
| final body = alertData['body'] as String?; |
I believe that behaves equivalently to the PR's current version. (It'll throw directly from the as operators if the data doesn't match, rather than FormatException. But the current version will already do that if body in particular has the wrong type.)
|
|
||
| Stream<android_intents_pigeon.AndroidIntentEvent> get androidIntentEvents; | ||
|
|
||
| void setupIosNotifFlutterApi(IosNotifFlutterApi api); |
There was a problem hiding this comment.
What does this method do; when and how should it be called?
This should get a brief bit of docs, like the other binding methods.
| void main() { | ||
| TestZulipBinding.ensureInitialized(); | ||
|
|
||
| IosNotifFlutterApi iosNotifFlutterApi() => TestZulipBinding.instance.iosNotifFlutterApi!; |
There was a problem hiding this comment.
nit: use the handy testBinding alias
| IosNotifFlutterApi iosNotifFlutterApi() => TestZulipBinding.instance.iosNotifFlutterApi!; | |
| IosNotifFlutterApi iosNotifFlutterApi() => testBinding.iosNotifFlutterApi!; |
(And then do we need this method at all, or can it just as well be inlined?)
| IosNotificationService.init(); | ||
| addTearDown(IosNotificationService.debugReset); |
There was a problem hiding this comment.
nit: add the reset teardown first
| IosNotificationService.init(); | |
| addTearDown(IosNotificationService.debugReset); | |
| addTearDown(IosNotificationService.debugReset); | |
| IosNotificationService.init(); |
That's what most of our tests do. It's a bit more comprehensive in behaving correctly, in particular if an exception occurs for some reason within the init call.
| // `assert()` statements are not working in the extension even when running in | ||
| // debug mode. But fortunately `kDebugMode` does correctly return true in debug | ||
| // mode and false in release mode. So, use this helper instead of `debugLog` | ||
| // from `log.dart`. | ||
| // TODO(upstream) debug asserts not working | ||
| void _debugLog(String message) { | ||
| if (kDebugMode) { | ||
| print(message); |
There was a problem hiding this comment.
This file's tests are noisy when run:
$ flutter test --no-pub test/notifications/ios_service_test.dart
00:02 +0: smoke
dart: IosNotificationService.init
dart: _IosNotifFlutterApiImpl.didReceivePushNotification
dart: content.payload={"aps":{"alert":{"title":"test title","subtitle":"test","body":"test content"},"sound":"default","badge":0},"zulip":{"server":"zulip.example.cloud","realm_id":4,"realm_uri":"https://chat.example/","realm_url":"https://chat.example/","realm_name":"Test","user_id":1008,"sender_id":12345,"sender_email":"a-person@example","time":1678139636,"message_ids":[1035],"recipient_type":"stream","stream_id":123,"topic":"example topic 703"}}
00:02 +1: All tests passed!
The bulk of our code avoids that problem by using debugLog, whose behavior is controlled by a flag debugLogEnabled which is left false in tests. So this problem can be solved by adding a similar flag which is used in this file.
But I wonder now if debugLogEnabled could explain the behavior you saw that led you to introduce this function. If you have iosNotificationServiceMain set debugLogEnabled in the same way as the app's main function, and then use debugLog the same way as elsewhere, does the issue still reproduce?
gnprice
left a comment
There was a problem hiding this comment.
Partial review of the remaining commits below. (Going afk for now, so going ahead and posting this.)
| FakeNotificationPigeonApi? _notificationPigeonApi; | ||
|
|
||
| IosNotifFlutterApi? get iosNotifFlutterApi => _iosNotifFlutterApi; | ||
| IosNotifFlutterApi? _iosNotifFlutterApi; |
There was a problem hiding this comment.
This field, like all state on the test binding, should get reset by one of the methods called by reset.
| } | ||
| } | ||
|
|
||
| static Future<String> _getIosAppGroupContainerPath() async { |
There was a problem hiding this comment.
nit: define entry points vs helpers in a consistent order; the surrounding methods go caller, then callee, so this should match by going after _iosDbFile rather than before
Another angle on the same point: _iosDbFile is a direct helper of _dbFile, so it's better if they can go next to each other, in order to enable reading them together. This is unrelated to _dbFile, so there's no benefit to having it separate those two from each other; it's a helper for _iosDbFile, but it can go next to that method by going after it instead of before.
| final pathProviderFoundation = PathProviderFoundation(); | ||
| final containerDbDir = await pathProviderFoundation.getContainerPath( |
gnprice
left a comment
There was a problem hiding this comment.
OK, finished reading; just a couple more comments below.
| // This group identifier is used to access the shared file system, | ||
| // across the different targets which are sandboxed from each other. | ||
| // See: https://developer.apple.com/documentation/xcode/configuring-app-groups | ||
| // | ||
| // Generally this identifier should never be changed, because doing so | ||
| // will reset the filesystem, causing the creation of new database from | ||
| // scratch. |
There was a problem hiding this comment.
Hmm, maybe I understand now why you initially didn't expect rename to work and were surprised to find that it did.
The app group's data doesn't live in a distinct filesystem. These comments shouldn't say "filesystem", because that has a specific meaning (see for example man mount), with implications including that rename will not work between one filesystem and another.
The Apple docs on this feature don't ever say it's a distinct filesystem:
https://developer.apple.com/documentation/xcode/configuring-app-groups
https://developer.apple.com/documentation/Foundation/FileManager/containerURL(forSecurityApplicationGroupIdentifier:)
Instead they say things like
the app group’s shared container
and even
the group’s shared directory in the file system
which specifically points toward it being in the same filesystem as other data.
The fact that rename works for moving a file from outside the shared container to inside the shared container is also definitive empirical proof that the two sides are part of the same filesystem, not distinct filesystems. That's enough to show that we shouldn't be saying in our comments that it's a separate filesystem. On that evidence alone, there's always the possibility that it's different on different iOS versions, or could be in the future; but then there's also what the docs say.
| // Move the database file from Runner target's filesystem which was stored | ||
| // at the directory path returned from path_provider's | ||
| // `getApplicationSupportDirectory` function, and it returns the following | ||
| // path: | ||
| // "Library/Application Support" via https://developer.apple.com/documentation/foundation/nssearchpathdirectory/nsapplicationsupportdirectory | ||
| final nonContainerDbDir = await getApplicationSupportDirectory(); |
There was a problem hiding this comment.
This comment recites what the code says, but doesn't say why we'd want to do this. Instead:
| // Move the database file from Runner target's filesystem which was stored | |
| // at the directory path returned from path_provider's | |
| // `getApplicationSupportDirectory` function, and it returns the following | |
| // path: | |
| // "Library/Application Support" via https://developer.apple.com/documentation/foundation/nssearchpathdirectory/nsapplicationsupportdirectory | |
| final nonContainerDbDir = await getApplicationSupportDirectory(); | |
| // Older versions of the app stored the database file at a different path. | |
| // Move it from there, if present. | |
| final nonContainerDbDir = await getApplicationSupportDirectory(); |
(We also don't need the details about what getApplicationSupportDirectory turns out to return. Those were relevant when choosing that function in the first place, and are arguably still relevant in _dbFile because they explain the choice that's still live there. But here it doesn't matter if this directory was a good place to choose for where the file should live; the reason we're interacting with it in these lines is because it's a place we did choose in earlier versions of the app.)
Hmm, does this mean that after this PR, those instructions no longer exercise the code path that notifications normally take? And after #2230, they won't work at all for the normal kind of notification (namely E2EE notifications). In that case, they should get deleted so as not to potentially cause confusion in the future. |
Testing
iOS Notification Service doesn't execute unless the push notification has the
"mutable-content" : 1property under"aps"object.Zulip server doesn't set this property in non-e2ee push notifications, so the dev server will need the patch below to do the testing:
Additionally, the iOS Notification Service doesn't execute with the simulated push notifications as described in "Testing Push Notifications on iOS Simulator" docs. It only executes if the push notification is a real push notification either from the Sandbox/Production APNs server. So, the dev server will need to setup those as documented here.
Fixes: #1265