[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap #6369

hellohuanlin · 2022-09-07T01:07:44Z

As suggested in #6229, this PR contains 2 changes together: (1) migration of FLTQuickActionsPlugin class, and (2) removing custom modulemap

About migration of `FLTQuickActionsPlugin`:

Why migrating this class first

This is following the "top down approach" discussed in the proposal. The migration is pretty smooth - Swift has much better support when importing ObjC.

Why test is not migrated in this PR

Migrating tests in separate PRs will give us better confidence, because the code will be verified by both ObjC and Swift tests, according to the proposal:

"For ObjC code that is already tested, make sure to also write tests in Swift. It is even better if the existing ObjC test just works for the migrated Swift code, because it provides us with good confidence that the new Swift code behaves exactly the same as the original ObjC code. Though due to interoperability and OCMock limitations discussed above, it is not always possible to migrate code and its test in separate steps. "

Swift format

For now I run swift-format on my local machine. But we should add the check to CI in the future, before wider community starts contributing Swift code.

About removing custom modulemap:

I do not like this solution since it makes private headers public. However, I wasn't able to find a better solution after lots of research on this topic.

Below is a summary of what I found so far, in case it's useful searching for a better solution in the future:

Cocoapods does not support a custom modulemap file for static libraries that contains Swift:

[!] Using Swift static libraries with custom module maps is currently not supported. Please build `quick_actions_ios` as a framework or remove the custom module map.

This limitation is the culprit of everything. If we could use a custom modulemap, we could easily define a submodule (aka we are currently doing) or use a private modulemap to map the private headers, like this.

Adding use_frameworks! to the Podfile also works, but we can't force developers to choose frameworks and not static libraries.
There seems to be some effort fixing this problem in Cocoapods (this and this), but there isn't much update since then.
What about removing clang modules at all? In pure ObjC it's fine, but Swift cannot directly import headers - it can only interact with modules.
The auto-generated default modulemap simply exports everything from the umbrella header. It defines a submodule for each of the header import, and re-export them to the main module. Only public headers are allowed in this process. Something like this:

module ModuleName {
  umbrella header "ModuleName-umbrella.h"
  export * 
  module * { export * }
}

This is only a temporary workaround during migration, but the bad state could last a long time for larger plugins. Though on the bright side, consider that private headers aren't that "private" at all - they can be imported by clients just like public headers. it's only that they are put under a separate directory named "PrivateHeaders" to warn the clients that it's risky to import them, but nothing can technically stop them from doing so.

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#108750

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…support custom modumemaps anymore

hellohuanlin · 2022-09-07T01:48:04Z

packages/quick_actions/quick_actions_ios/ios/Classes/FLTQuickActionsPlugin.m

-
- (void)dealloc {
-  [_channel setMethodCallHandler:nil];
-  _channel = nil;


Did not migrate this function since I don't think it's necessary. _channel should be gone automatically. Also double checked other plugins and they don't call setMethodCallHandler:nil

Also double checked other plugins and they don't call setMethodCallHandler:nil

It's been a while since I dug into plugin lifetime, but IIRC it's the channel handler registration that keeps the plugin instance alive in the first place; if that's true, then this will never do anything in dealloc since we couldn't be here if there's still an active channel. I may be misremembering though.

Gotcha. This would be a serious memory issue for our plugins. I created flutter/flutter#111206 to track it.

hellohuanlin · 2022-09-07T03:55:38Z

packages/quick_actions/quick_actions_ios/ios/quick_actions_ios.podspec

+     'LIBRARY_SEARCH_PATHS' => '$(TOOLCHAIN_DIR)/usr/lib/swift/$(PLATFORM_NAME)/ $(SDKROOT)/usr/lib/swift',
+     'LD_RUNPATH_SEARCH_PATHS' => '/usr/lib/swift',
+  }
+  s.public_header_files = 'Classes/**/*.h'


unfortunately we have to expose private headers until migration is complete (refer to the PR description)

fix unused variable warnings nit

hellohuanlin · 2022-09-07T18:54:28Z

packages/quick_actions/quick_actions_ios/ios/Classes/QuickActionsPlugin.swift

+    switch call.method {
+    case "setShortcutItems":
+      // `arguments` must be an array of dictionaries
+      let items = call.arguments as! [[String: Any]]


Force unwrap it to keep the same behavior as ObjC. Whether to crash on platform code or to surface an error is discussed here in this proposal.

If items is nil, it doesn't crash on Objective-C(here). It actually sets the [UIApplication sharedApplication].shortcutItems to an empty array, which results the same behavior as "clearShortcutItems". On the other hand, it would crash in this swift version due to force unwrapping.

I think the current Objective-C behavior is more like?

let items = call.arguments as [[String: Any]] ?? [] shortcutStateManager.setShortcutItems(items) result(nil)

I'm not sure the original design of an implicit "clearShortcutItems" is intended but for migration I feel like we should keep the behavior the same if possible and have a follow up PR to handle the error.

If wrong type it should crash:

NSNumber *arr = @1; for (NSDictionary *item in (NSArray *)arr) { // crash here NSLog(@"%@", item); }

But I see your point about nil value. The equivalent swift should be this instead:

if let arguments = call.arguments { // crash of wrong type items = arguments as! [[String:Any]] } else { // no crash if nil items = [] }

Not sure if it's worth to do tho. The corresponding dart side guarantees that the argument is non-null. So the else block actually never run.

hellohuanlin · 2022-09-07T18:55:51Z

packages/quick_actions/quick_actions_ios/ios/Classes/QuickActionsPlugin.swift

+      // Return false to indicate we handled the quick action to ensure
+      // the `application:performActionFor:` method is not called (as
+      // per Apple's documentation:
+      // https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622935-application).


these comments are copied over from objc.

stuartmorgan · 2022-09-08T15:33:13Z

For now I run swift-format on my local machine. But we should add the check to CI in the future, before wider community starts contributing Swift code.

I don't think that's viable yet for the reasons in flutter/flutter#41129 (comment). We already have macOS plugins that are using Swift, and we accept changes to them. Auto-formatting is definitely convenient, but it's not catastrophic if we don't have it.

We should add support for it in the tool sooner rather than later, and provide instructions in the tooling docs for using it; then if we have PRs that have really problematic formatting we can point people to those to do locally. (Minor drift relative to the autoformatter is something we can just automatically fix as people who do run the autoformatter change files.)

stuartmorgan · 2022-09-08T15:52:33Z

packages/quick_actions/quick_actions_ios/ios/Classes/FLTQuickActionsPlugin.m

-
- (void)dealloc {
-  [_channel setMethodCallHandler:nil];
-  _channel = nil;


Also double checked other plugins and they don't call setMethodCallHandler:nil

It's been a while since I dug into plugin lifetime, but IIRC it's the channel handler registration that keeps the plugin instance alive in the first place; if that's true, then this will never do anything in dealloc since we couldn't be here if there's still an active channel. I may be misremembering though.

stuartmorgan · 2022-09-08T15:54:36Z

packages/quick_actions/quick_actions_ios/pubspec.yaml

@@ -13,7 +13,7 @@ flutter:
    implements: quick_actions
    platforms:
      ios:
-        pluginClass: FLTQuickActionsPlugin
+        pluginClass: QuickActionsPlugin


Don't we still require the thin ObjC wrapper around Swift plugins? That's still what we are generating in the template; I thought it was required for the Swift-plugin-in-ObjC host to work in iOS apps. @jmagman

The generated file is this:

#import "GeneratedPluginRegistrant.h" #if __has_include(<integration_test/IntegrationTestPlugin.h>) #import <integration_test/IntegrationTestPlugin.h> #else @import integration_test; #endif #if __has_include(<quick_actions_ios/QuickActionsPlugin.h>) #import <quick_actions_ios/QuickActionsPlugin.h> #else @import quick_actions_ios; #endif @implementation GeneratedPluginRegistrant + (void)registerWithRegistry:(NSObject<FlutterPluginRegistry>*)registry { [IntegrationTestPlugin registerWithRegistrar:[registry registrarForPlugin:@"IntegrationTestPlugin"]]; [QuickActionsPlugin registerWithRegistrar:[registry registrarForPlugin:@"QuickActionsPlugin"]]; } @end

It is directly using [QuickActionsPlugin registerWithRegistrar:]. So we should be fine here?

@jmagman Did __has_include resolve all the issues with direct Swift-into-Obj-C import? If so, we should fix the plugin template to stop generating the Obj-C wrapper.

Are you suggesting to remove GeneratedPluginRegistrant.h/m and move the logic to AppDelegate?

No, I'm talking about the ObjC files here.

It seems like either there is still at least one combination of app language and library build type that requires those wrappers, in which case this plugin needs them too, or that's no longer the case, in which case the template shouldn't need them.

Objc-plugin-in-Swift-app will work without the wrapper if the Objective-C plugins define modules correctly, which they should out of the box as of plugin podspec template change in flutter/flutter#40302 (Flutter 1.10.3). Plugins created before that point may not work when used in a Swift app since it's missing DEFINES_MODULE. I think all other combinations should work without the wrapper.

We could add a warning to the tool for the plugin author that they need to add { 'DEFINES_MODULE' => 'YES' } to their podspec (although I don't think we have a good tool hook to know they are building a plugin example app and not just a regular app). Or we could add some documentation about how to fix up the podspec when their plugin customers report it doesn't work in a Swift app.

@jmagman thanks for the explanation. Sounds like we should remove those templates. Created an issue here flutter/flutter#111928

Objc-plugin-in-Swift-app will work without the wrapper if the Objective-C plugins define modules correctly

Are all the languages swapped in this comment by accident, or we talking about different cases? I was talking about the ObjC wrapper that we generate in the Swift plugin template, to make them look like older (ObjC) plugins, which IIRC was necessary for the Swift-plugin-in-ObjC-app case (for at least one of the library build modes). But I guess modules could fix mismatches in both directions?

Are all the languages swapped in this comment by accident, or we talking about different cases? I was talking about the ObjC wrapper that we generate in the Swift plugin template, to make them look like older (ObjC) plugins, which IIRC was necessary for the Swift-plugin-in-ObjC-app case (for at least one of the library build modes).

You're right I was mixing this up with the Objective-C files that get generated in a Swift iOS app, which I think was originally so the Swift app can import Objective-C plugins pre-modules.
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/templates/app_shared/ios-swift.tmpl/Runner/Runner-Bridging-Header.h
https://github.com/flutter/flutter/blob/5aad12cab1e9c0863ea5fbc9a770b2734d7772db/packages/flutter_tools/lib/src/flutter_plugins.dart#L456

But you're right, I think the Swift plugins also don't need the Objective-C code now. Fortunately this area is pretty well tested, so if someone tries flutter/flutter#111928 the tests should let us know if there are any corner cases I'm not remembering.

Reading it again (@jmagman you gave me hints about clang module), i think it's possible that the original plugin template wanted to expose both header and module, so that its objc client can freely choose from importing headers vs importing modules. This benefit seems so trivial that we should probably remove the template.

packages/quick_actions/quick_actions_ios/ios/Classes/QuickActionsPlugin.swift

cyanglaz

Swift code LGTM

jmagman

LGTM

jmagman · 2022-09-21T20:01:27Z

@hellohuanlin is this ready to land?

hellohuanlin · 2022-09-21T21:23:29Z

@jmagman sorry i was busy with the video player ios 16 bug. will rebase it to fix the submit-queue and land later.

…e custom modulemap (flutter/plugins#6369)

…odulemap (flutter#6369)

hellohuanlin added 5 commits September 6, 2022 16:34

[quick_actions]remove custom module map and private header files

619709f

migrated quick actions plugin class

ff69557

update podspec to include swift

e6955ae

fix unit test class name and add back umbrella header since we don't …

6d63647

…support custom modumemaps anymore

format

569eb63

github-actions bot added p: quick_actions platform-ios labels Sep 7, 2022

hellohuanlin force-pushed the quick_actions_remove_custom_module_map_and_migration_plugin_class branch from 31335dd to 40875c9 Compare September 7, 2022 01:19

nit

a548386

hellohuanlin commented Sep 7, 2022

View reviewed changes

hellohuanlin force-pushed the quick_actions_remove_custom_module_map_and_migration_plugin_class branch from 40875c9 to d54408e Compare September 7, 2022 01:59

swift format

f5b80bc

hellohuanlin force-pushed the quick_actions_remove_custom_module_map_and_migration_plugin_class branch from d54408e to f5b80bc Compare September 7, 2022 03:45

remove umbrella header since it's automatically generated

e1e5835

hellohuanlin commented Sep 7, 2022

View reviewed changes

bump patch version

ef032a4

fix unused variable warnings nit

hellohuanlin force-pushed the quick_actions_remove_custom_module_map_and_migration_plugin_class branch from 479b98a to ef032a4 Compare September 7, 2022 18:51

hellohuanlin commented Sep 7, 2022

View reviewed changes

hellohuanlin requested review from jmagman, stuartmorgan and cyanglaz September 7, 2022 18:57

hellohuanlin marked this pull request as ready for review September 7, 2022 18:57

stuartmorgan reviewed Sep 8, 2022

View reviewed changes

fix typo

8cf6109

jmagman mentioned this pull request Sep 8, 2022

Skip platform-specific tests for federated plugins for different platforms when testing PRs in presubmit flutter/flutter#111211

Closed

hellohuanlin mentioned this pull request Sep 13, 2022

[quick_actions]Migrate the XCUITests to Swift #6425

Merged

11 tasks

cyanglaz approved these changes Sep 19, 2022

View reviewed changes

jmagman approved these changes Sep 19, 2022

View reviewed changes

hellohuanlin mentioned this pull request Sep 19, 2022

[plugins] remove ObjC wrapper in swift plugin templates flutter/flutter#111928

Closed

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 22, 2022

auto-submit bot merged commit ff5b28a into flutter:main Sep 22, 2022

engine-flutter-autoroll mentioned this pull request Sep 23, 2022

Roll Plugins from 7d5ff597e35b to 3ba81503e7f7 (7 revisions) flutter/flutter#112273

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 23, 2022

ff5b28a8a [quick_actions]Migrate the plugin class to Swift, and remov…

54d5847

…e custom modulemap (flutter/plugins#6369)

jmagman mentioned this pull request Oct 18, 2022

Remove Swift plugin Objective-C files flutter/flutter#113585

Merged

8 tasks

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[quick_actions]Migrate the plugin class to Swift, and remove custom m…

7508ada

…odulemap (flutter#6369)

This was referenced Jan 13, 2023

in_app_purchase_storekit iOS platform tests failing on master, crashing during native tests flutter/flutter#118418

Closed

[shared_preferences_foundation] Add Swift runtime search paths for Objective-C apps #6952

Merged

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[quick_actions]Migrate the plugin class to Swift, and remove custom m…

546e3f8

…odulemap (flutter#6369)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap #6369

[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap #6369

hellohuanlin commented Sep 7, 2022 •

edited

Loading

hellohuanlin Sep 7, 2022 •

edited

Loading

stuartmorgan Sep 8, 2022

hellohuanlin Sep 8, 2022

hellohuanlin Sep 7, 2022

hellohuanlin Sep 7, 2022

cyanglaz Sep 16, 2022 •

edited

Loading

hellohuanlin Sep 16, 2022

cyanglaz Sep 19, 2022

hellohuanlin Sep 7, 2022

stuartmorgan commented Sep 8, 2022

stuartmorgan Sep 8, 2022

stuartmorgan Sep 8, 2022

hellohuanlin Sep 8, 2022

stuartmorgan Sep 8, 2022

hellohuanlin Sep 8, 2022 •

edited

Loading

stuartmorgan Sep 9, 2022

jmagman Sep 19, 2022

hellohuanlin Sep 19, 2022 •

edited

Loading

stuartmorgan Sep 19, 2022

jmagman Sep 19, 2022

hellohuanlin Sep 22, 2022

cyanglaz left a comment

jmagman left a comment

jmagman commented Sep 21, 2022

hellohuanlin commented Sep 21, 2022

[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap #6369

[quick_actions]Migrate the plugin class to Swift, and remove custom modulemap #6369

Conversation

hellohuanlin commented Sep 7, 2022 • edited Loading

About migration of FLTQuickActionsPlugin:

Why migrating this class first

Why test is not migrated in this PR

Swift format

About removing custom modulemap:

Pre-launch Checklist

hellohuanlin Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

jmagman commented Sep 21, 2022

hellohuanlin commented Sep 21, 2022

hellohuanlin commented Sep 7, 2022 •

edited

Loading

About migration of `FLTQuickActionsPlugin`:

hellohuanlin Sep 7, 2022 •

edited

Loading

cyanglaz Sep 16, 2022 •

edited

Loading

hellohuanlin Sep 8, 2022 •

edited

Loading

hellohuanlin Sep 19, 2022 •

edited

Loading