Add support for interactive transitioning #14

randcode-generator · 2017-06-06T05:38:43Z

codecov · 2017-06-06T05:45:37Z

Codecov Report

Merging #14 into develop will decrease coverage by 15.66%.
The diff coverage is 21.15%.

@@             Coverage Diff              @@
##           develop      #14       +/-   ##
============================================
- Coverage    80.13%   64.46%   -15.67%     
============================================
  Files            3        4        +1     
  Lines          146      197       +51     
============================================
+ Hits           117      127       +10     
- Misses          29       70       +41

Impacted Files	Coverage Δ
...te/MDMViewControllerInteractiveTransitionContext.m	`0% <0%> (ø)`
src/UIViewController+TransitionController.m	`76% <0%> (-24%)`	⬇️
src/private/MDMPresentationTransitionController.m	`61.33% <24%> (-18.67%)`	⬇️
src/private/MDMViewControllerTransitionContext.m	`74.69% <71.42%> (-0.63%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10daa68...3c1d297. Read the comment docs.

jverkoey

First pass at feedback.

jverkoey · 2017-06-06T18:17:45Z

examples/CustomPresentationExample.swift

+    }
+    timer.resume()
+  }
+   */


Create a new example and include this timer code there so that users can play with the demo more easily.

jverkoey · 2017-06-06T18:18:39Z

examples/CustomPresentationExample.swift

+  var i = 0.01
+  func start(withInteractiveContext context: InteractiveTransitionContext) {
+    let timer = DispatchSource.makeTimerSource(queue: DispatchQueue.main)
+    timer.scheduleRepeating(deadline: .now(), interval: .milliseconds(10), leeway: .milliseconds(10))


Remember: transition instances are reused for both the present and dismiss transitions, so any instance variables need to be re-initialized before the transition begins. In this case, we need to remember to reset i to 0.01 when we start the interactive transition.

If we don't reset the i value then the transition will immediately complete the dismissal transition.

jverkoey · 2017-06-06T18:19:47Z

examples/FadeExample.m

+  });
+  dispatch_resume(timer);
+}
+ */


Similar to above: let's create a new example that specifically demonstrates interactive transitioning.

jverkoey · 2017-06-06T18:21:16Z

src/MDMTransition.h

@@ -34,6 +35,8 @@ NS_SWIFT_NAME(Transition)
 */
 - (void)startWithContext:(nonnull id<MDMTransitionContext>)context;

+@optional
+- (void)startWithInteractiveContext:(nonnull id<MDMInteractiveTransitionContext>)context;


I generally prefer avoiding @optional these days because it means the compiler won't enforce method signatures.

Instead, I suggest we add an MDMTransitionWithInteraction protocol similar to how we've been building the other features.

jverkoey · 2017-06-06T18:21:30Z

src/MDMTransitionContext.h

@@ -80,3 +80,9 @@ NS_SWIFT_NAME(TransitionContext)
 @property(nonatomic, strong, readonly, nonnull) UIView *containerView;

 @end
+
+NS_SWIFT_NAME(InteractiveTransitionContext)
+@protocol MDMInteractiveTransitionContext


Should this conform to MDMTransitionContext?

I made MDMInteractiveTransitionContext because this controls how much of the animation should be presented. The MDMTransitionContext controls what animation should be presented.

This means the interactive start API won't have access to things like the container view, the transition's direction, or the fore/back view controller - is this intentional? I can imagine that someone might want to have access to those properties in order to be able to configure the interactive behavior.

You mean someone changing the animation midway?
For example,
1 second (total) animation of view fading in. Around 0.5 seconds into the animation, user can also manipulate the size of view.

I might want to parse gestural movement differently depending on whether the transition is moving forward or backward, for example.

Checking in here again: were you able to try making this conform to MDMTransitionContext?

Haven't tried. I'll try it now.

Ok, I have to admit this doesn't make any sense to me.
So right now, I have

@property(nonatomic, readonly, nonnull) id<MDMTransitionContext> transitionContext;

to save the instance of MDMTransitionContext. If I inherit it, all I am doing is adding wrappers around MDMTransitionContext and I still have to save the instance so I can implement transitionDidEnd.

I left a checklist below, but in short you shouldn't have to save the transition context in any sense because we already have a transition context - you'll provide this same instance to both the transition and the interactive transition (i.e. the transition interaction controller).

Sounds like you want interactive transition and transition merged. So, I'll check if isInteractive is implemented. If it's not implemented, I assume it's not interactive, if it is implemented, I'll call isInteractive to determine if the direction (forward/backward) should be interactive.

jverkoey · 2017-06-06T18:22:32Z

src/private/MDMPresentationTransitionController.m

+
+  if ([_transition respondsToSelector:@selector(startWithInteractiveContext:)]) {
+      _interactiveContext = [[MDMViewControllerInteractiveTransitionContext alloc] initWithTransition: _transition];
+    [_transition startWithInteractiveContext:_interactiveContext];


Ideally we'd put the animated and interactive start invocations alongside one another in the transition context instance. Is it possible to move this call there?

jverkoey · 2017-06-16T17:19:53Z

src/MDMTransitionContext.h

@@ -80,3 +80,9 @@ NS_SWIFT_NAME(TransitionContext)
 @property(nonatomic, strong, readonly, nonnull) UIView *containerView;

 @end
+
+NS_SWIFT_NAME(InteractiveTransitionContext)
+@protocol MDMInteractiveTransitionContext


Checking in here again: were you able to try making this conform to MDMTransitionContext?

jverkoey · 2017-06-16T21:28:59Z

Am proposing the following changes to consolidate the code paths here (these affect public APIs which is why I'm suggesting we make these changes now before the code lands):

Make MDMInteractiveTransitionContext conform to TransitionContext.
Delete transitionContext from InteractiveTransitionContext.
Rename InteractiveTransition to TransitionInteractionController.
Delete ViewControllerInteractiveTransitionContext and implement InteractiveTransitionContext in ViewControllerInteractiveTransitionContext.
Provide the transition context instance to both the Transition and TransitionInteractionController. Transition will receive a TransitionContext. TransitionInteractionController will receive an InteractiveTransitionContext.

jverkoey · 2017-06-16T21:22:58Z

src/MDMTransition.h

+
+NS_SWIFT_NAME(InteractiveTransition)
+@protocol MDMInteractiveTransition <NSObject>
+- (Boolean)isInteractive:(nonnull id<MDMTransitionContext>)context;


jverkoey · 2017-06-16T21:23:52Z

src/MDMTransitionController.h

@@ -44,5 +45,5 @@ NS_SWIFT_NAME(TransitionController)
 This may be non-nil while a transition is active.
 */
 @property(nonatomic, strong, nullable, readonly) id<MDMTransition> activeTransition;
-
+@property(nonatomic, strong, nullable) id<MDMInteractiveTransition> interactiveTransition;


This technically isn't a "transition" as much as an interaction controller for a transition. Can we consider renaming this to transitionInteractionController?

jverkoey · 2017-06-16T21:24:38Z

src/UIViewController+TransitionController.h

@@ -31,4 +32,5 @@
 @property(nonatomic, strong, readonly, nonnull) id<MDMTransitionController> mdm_transitionController
    NS_SWIFT_NAME(transitionController);

+@property(nonatomic, strong, nullable) id<MDMInteractiveTransitionContext> interactiveTransitionContext;


We don't need to expose this if we store the variable on the transition controller.

transitionController.transition = CrossFade() transitionController.transitionInteractionController = SwipeToDismiss()

What's the type for transitionInteractionController?

jverkoey · 2017-06-16T21:27:47Z

src/private/MDMPresentationTransitionController.m

@@ -129,4 +144,35 @@ - (void)prepareForTransitionWithSourceViewController:(nullable UIViewController
  }
 }

+- (nullable id<UIViewControllerInteractiveTransitioning>) prepareForInteractiveTransition {


Ideally this method would happen as a result of prepareForTransition. We know that the animation callback is invoked first and therefor our transition context will be initialized, so we can return the percent-driven transition controller from our transition context.

jverkoey suggested changes Jun 6, 2017

View reviewed changes

randcode-generator force-pushed the issue3 branch 8 times, most recently from 8cce24b to b9ea0c3 Compare June 13, 2017 03:56

jverkoey suggested changes Jun 16, 2017

View reviewed changes

randcode-generator closed this Jun 17, 2017

randcode-generator force-pushed the issue3 branch from 3c1d297 to f643cd5 Compare June 17, 2017 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for interactive transitioning #14

Add support for interactive transitioning #14

randcode-generator commented Jun 6, 2017

codecov bot commented Jun 6, 2017 •

edited

Loading

jverkoey left a comment

jverkoey Jun 6, 2017

jverkoey Jun 6, 2017

jverkoey Jun 6, 2017

jverkoey Jun 6, 2017

jverkoey Jun 6, 2017

randcode-generator Jun 6, 2017

jverkoey Jun 6, 2017

randcode-generator Jun 6, 2017

jverkoey Jun 6, 2017

jverkoey Jun 16, 2017

randcode-generator Jun 16, 2017

randcode-generator Jun 16, 2017

jverkoey Jun 16, 2017 •

edited

Loading

randcode-generator Jun 16, 2017

jverkoey Jun 6, 2017

jverkoey Jun 16, 2017

jverkoey commented Jun 16, 2017 •

edited

Loading

jverkoey Jun 16, 2017

jverkoey Jun 16, 2017

jverkoey Jun 16, 2017

randcode-generator Jun 16, 2017

jverkoey Jun 16, 2017

Add support for interactive transitioning #14

Add support for interactive transitioning #14

Conversation

randcode-generator commented Jun 6, 2017

codecov bot commented Jun 6, 2017 • edited Loading

Codecov Report

jverkoey left a 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

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

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

jverkoey Jun 16, 2017 • 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

jverkoey commented Jun 16, 2017 • 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

codecov bot commented Jun 6, 2017 •

edited

Loading

jverkoey Jun 16, 2017 •

edited

Loading

jverkoey commented Jun 16, 2017 •

edited

Loading