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.

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.

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.

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

Adopting pigeon would solve this.

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?

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.

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

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.

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.

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

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

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.

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.

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?

Sounds good! Thanks for iterating on this with me!

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

Adopting pigeon would solve this.

Why does this need AnyObject?

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
  }
}

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.