Skip to content
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

Add typedefs for completion blocks #339

Merged
merged 2 commits into from
Aug 5, 2016

Conversation

timothyekl
Copy link
Contributor

Apple's documentation for declaring and creating blocks advocates for naming block types when they are used in multiple places. The current 1Password extension API "spells out" entire block signatures at each use, despite having only three distinct completion block types.

Including typedefs for these three block types would significantly reduce line noise across seven method declarations in OnePasswordExtension.h, as well as several more in the corresponding implementation file.

This branch declares these three types in OnePasswordExtension.h, each with the name pattern OnePassword<Kind>CompletionBlock. "Kind" is:

  • Login for blocks that pass a login item as an NSDictionary
  • Success for blocks that pass a success BOOL
  • Extension for blocks that pass an NSExtensionItem

All "spelled out" block types in method signatures (in both the header and implementation files) are replaced with equivalent typedef'd names. Since several methods exist in the implementation that aren't exposed in the header, and the assume-nonnull range does not extend throughout the implementation file, explicitly mark blocks in such methods nonnull (matching other arguments).

This change does slightly increase the surface area of symbols needing prefixing for developers intending to redistribute this source as part of a framework. I think this is an acceptable tradeoff, but am happy to hear out concerns in that direction. (The branch also adjusts the comment warning about prefixing the class to also mention the new typedefs.)

Declare three block types in OnePasswordExtension.h, each with the
name pattern "OnePassword<Kind>CompletionBlock". <Kind> is:

* "Login" for blocks that pass a login item as an NSDictionary
* "Success" for blocks that pass a `success` BOOL
* "Extension" for blocks that pass an NSExtensionItem

Adjust the immediately preceding header comment to warn framework
repackagers about namespacing not only the class, but the new typedefs
as well.

Replace all explicit block types in method signatures (in both the
header and implementation files) with equivalent typedef'd names. Since
several methods exist in the implementation that aren't exposed in the
header, and the assume-nonnull range does not extend throughout the
implementation file, explicitly mark blocks in such methods `nonnull`
(matching other arguments).

Signed-off-by: Tim Ekl <lithium3141@gmail.com>
// and associated typedefs. You might to so by adding your own project prefix, e.g.,
// MyLibraryOnePasswordExtension.

typedef void (^OnePasswordLoginCompletionBlock)(NSDictionary * __nullable loginDictionary, NSError * __nullable error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please rename OnePasswordLoginCompletionBlock to something like OnePasswordLoginDictionaryCompletionBlock?

@radazzouz
Copy link
Contributor

Thanks so much for contributing @lithium3141! 👍

At first glance, the proposed changes look great, except for the couple of renaming issue (see the in-line comments).

I just labeled the PR with under consideration and in review, as we need more time to properly test things out and to make sure that this is indeed something we want.

I will keep you posted by commenting on this PR. In the meantime, if you have any other questions, please do not hesitate to ask.

Cheers!

Expand the Login and Extension completion block type names to contain
full return type phrases: LoginDictionary and ExtensionItem.

Signed-off-by: Tim Ekl <lithium3141@gmail.com>
@timothyekl
Copy link
Contributor Author

Done in d5e4de7. Thanks for the quick reply!

@radazzouz
Copy link
Contributor

Thanks for updating your PR so quickly, @lithium3141 👍

After much consideration and testing with my colleague @nathanvf, we concluded that this is an Awesome addition to the App Extension API. I am therefore merging this PR right now!

All the Best,

@radazzouz radazzouz merged commit 346de88 into agilebits:master Aug 5, 2016
radazzouz pushed a commit that referenced this pull request Aug 9, 2016
[NEW] Added typedefs for completion blocks for more flexibility. {#339}

[IMPROVED] Updated the Javascript code for Web View Filling. {#335}
[IMPROVED] All the completion blocks from the API are now `nonnull`. {#341}

[FIXED] Fixed minor typo in the documentation. {#340}
@radazzouz radazzouz mentioned this pull request Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants