Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Fix bug where transitions would not complete if the presentation controller didn't implement the startWithContext method #45

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Fix bug where transitions would not complete if the presentation controller didn't implement the startWithContext method #45

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/private/MDMViewControllerTransitionCoordinator.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ - (instancetype)initWithTransitions:(NSArray<NSObject<MDMTransition> *> *)origin

_completionBlocks = [NSMutableArray array];

if (_presentationController) {
if (_presentationController
&& [_presentationController respondsToSelector:@selector(startWithContext:)]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to call out that we are checking for compliance to MDMTransition rather than the specific method?
[_presentationController conformsToProtocol:@protocol(MDMTransition)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking for protocol conformity requires that the client explicitly conform to the protocol, while this check is a bit more lenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is more lenient, I'm not sure that is preferable. However, as this is a private component, we can save that spirited debate for a later date. 😁

Copy link
Contributor Author

@jverkoey jverkoey Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Checking whether the instance responds to the selector is the only safe guarantee we have that the method is implemented and won't crash if invoked.

For example, if a given API becomes optional in the future then a conformsToProtocol check would not only be unhelpful, but if we did not then also check respondsToSelector then it would also lead us to a crash.

Copy link
Contributor Author

@jverkoey jverkoey Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If your argument is against duck typing, however, then that is a separate discussion.

My personal belief is that a shared component library should be lenient on how Objective-C clients are expected to implement protocols and I've followed this practice through Nimbus, GoogleKit, MDC, and Material Motion. In other words, I believe that duck-type conformity is preferable over strict conformity for Objective-C code because it's more future-proof and lenient on casual conformers.

Aside: Swift brings its own opinions into this discussion because it is not possible to have optional methods in pure Swift protocols, so in Swift it's much easier to do strict conformity (obj as? MyType). As a result, duck typing is not often seen (and that's ok!).

Copy link
Contributor

Choose a reason for hiding this comment

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

Different philosophies I think.
My feelings lean more toward, if you want to use Q you must conform to W.
I don't know that I've experienced to many protocols being made optional, usually just additions or complete deprecations.
Which leads me to my concern that MDMTransition would add a method, endWithContext: perhaps. We would then be checking for two methods. Additionally, by checking only for respondsToSelector, we aren't 'verifying' the parameter types of the selectors which conforms would do.
I think it comes down to the balance of ease of use for casual conformers -vs- strict(!) APIs.

So,...... pistols at dawn? 🔫 🌅 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pistols at dawn!

_presentationContext =
[[MDMViewControllerTransitionContext alloc] initWithTransition:nil
direction:direction
Expand Down