[quick_actions]Migrates all remaining components to Swift, and deprecate OCMock

[hellohuanlin]

This PR completes the Swift migration for quick_actions plugin. The main part is to migrate FLTShortcutStateManager using POP. We defined the following new types:

• MethodChannel protocol (to which FlutterMethodChannel conforms)
• AppShortcutControlling protocol (to which UIApplication conforms)
• ShortcutItemParser protocol

The above naming follows the proposal "Swift POP naming convention".

Note that we have to migrate FLTQuickActionsPluginTests too in this PR, because this test depends on ShortcutStateManaging, which is non-objc type that OCMock does not support.

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

flutter/flutter#108750

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[hellohuanlin]

Here I put this extension in the same file as the protocol. Alternatively we can also use separate files and I am ok either way.

More details discussed in Rule 3(b) of the Swift POP naming convention proposal. Feel free to leave any comments.

[hellohuanlin]

Same here: i put this class in the same file as the protocol, but also ok to use separate files.

More details discussed in Rule 3(b) of the Swift POP naming convention proposal. Feel free to leave any comments.

[cyanglaz]

Maybe we should not let this plugin crash the production app? Maybe use Assert to crash debug app and return nil if either serialized["type"] or serialized["localizedTitle"] is nil in release.

Does the original Objective-C API crashes the App if type or localizedTitle is nil? If so, I'm ok with keep the same behavior and follow up with a fix.

[hellohuanlin]

Does the original Objective-C API crashes the App if type or localizedTitle is nil?

I think so, the API is non-null (and that's why it bridges to non-null String in Swift): https://developer.apple.com/documentation/uikit/uiapplicationshortcutitem/1623372-initwithtype?language=objc

[jmagman]

Adopting pigeon would solve this.

[jmagman]

Does the Objective-C API actually crash if you pass in nil though? A lot of Objective-C APIs that predate nullability do handle nils under the covers, especially since most implementation files aren't wrapped in NS_ASSUME_NULL to catch the error in analysis. Ideally passing in the wrong types would give an actionable error, but at the least this should not introduce a new crash?

[hellohuanlin]

Good point! Let me try it out.

The dart counterpart also guarantees that this is never null though. So it likely doesn't matter anyways.

[hellohuanlin]

I just tried it out - indeed the objc API did not crash - instead it just ignores it.

[hellohuanlin]

friendly ping

[jmagman]

Does it make sense for there to be a var shortcutItems: [UIApplicationShortcutItem]? { get set } protocol as well as this one? It looks like DefaultShortcutStateManager could easily implement ShortcutItemServicing instead by exposing service.shortcutItems.

[hellohuanlin]

The var shortcutItems: [UIApplicationShortcutItem]? { get set } protocol is just an abstraction to stub UIApplication. The ShortcutStateManager has extra parsing logic. So it is better not to combine these 2.

An alternative way is to define a ShortcutItemParser, like this:

protocol ShortcutItemParser {
  func parseItems(_ items: [[String, Any]]) -> [UIApplicationShortcutItem]
}

This will make the dependency DAG more "flat". Let me try and see how it goes.

[hellohuanlin]

I updated ShortcutStateManager to ShortcutItemParser, and renamed ShortcutItemServicing to AppShortcutControlling.

The benefit is simpler and more flat dependency tree: Now the QuickActionsPlugin depends on both ShortcutItemParser and AppShortcutControlling, and these 2 subcomponents do not depend on each other anymore.

PTAL thx

[hellohuanlin]

Note that we still want to have a separate AppShortcutControlling protocol, so that we can stub out the UIApplication.default

[jmagman]

ShortcutItemParser SGTM.

But AppShortcutControlling isn't a capability in the sense the docs mean, in that setting this shortcutItems collection is not operating on the "implementing" UIApplication, it's more mutating it. Like the example ProgressReporting would be reporting on the progress of the thing implementing the protocol.

In addition, "AppShortcutControlling" doesn't exactly make sense in English. How about ShortcutItemProvider? At the end of the day Swift's naming guidance is purposely very loosey, we should name it in a way that describes what it does, and also makes sense in English.

[hellohuanlin]

Hmmm, Provide sounds like it only provides something, with only getter and no setters (e.g. AgeProviding in the style guide).

How about AppShortcutManaging? it's setting a collection, but also making iOS to update the springboard shortcuts. So it feels like performing an action, rather than just a collection holder.

[hellohuanlin]

i'm circling back on this - i think i am fine with the Provider name (it's not ideal, as explained above), but i can't think of a better name.

Though we want Providing for protocol name, since it's a capability to provide something (similar to NameProviding and AgeProviding in our style guide. What do you think?

[jmagman]

Sounds good! Thanks for iterating on this with me!

[hellohuanlin]

Renamed to ShortcutItemProviding (unless @cyanglaz or @stuartmorgan can think of a better name?)

[jmagman]

Adopting pigeon would solve this.

[jmagman]

Why does this need AnyObject?

[hellohuanlin]

AnyObject scopes this protocol to reference types (classes). It prevents any value types (enums & structs) to conform to it.

Since this is injected dependency with no value semantics, we want to scope it to reference types. See the NOTE below:

class QuickActionsPlugin {
  // NOTE: If we remove `AnyObject`, we have to use `var` here. 
  private let controller: AppShortcutControlling 

  func foo() {
    controller.shortcutItems = [] // mutate here
  }
}

[jmagman]

ShortcutItemParser SGTM.

But AppShortcutControlling isn't a capability in the sense the docs mean, in that setting this shortcutItems collection is not operating on the "implementing" UIApplication, it's more mutating it. Like the example ProgressReporting would be reporting on the progress of the thing implementing the protocol.

In addition, "AppShortcutControlling" doesn't exactly make sense in English. How about ShortcutItemProvider? At the end of the day Swift's naming guidance is purposely very loosey, we should name it in a way that describes what it does, and also makes sense in English.

[hellohuanlin]

@jmagman @cyanglaz friendly ping

[cyanglaz]

LGTM

[jmagman]

LGTM