Implement draggable plan and rename library to direct manipulation
ClosedPublic

Authored by rcameron on Aug 25 2016, 9:13 PM.

Diff Detail

Repository
rMDMDIRECTMANIPULATIONSWIFT material-motion/direct-manipulation-swift
Branch
cleanup (branched from develop)
Lint
Lint OK
Unit
Unit Tests OK
rcameron retitled this revision from to Internal cleanup.Aug 25 2016, 9:13 PM
rcameron updated this object.
rcameron edited the test plan for this revision. (Show Details)
Restricted Application added a project: Material Motion. · View Herald Transcript
featherless requested changes to this revision.Aug 25 2016, 9:26 PM
featherless added a reviewer: featherless.
featherless added a subscriber: featherless.

When referencing github issues in commit messages, use the full github url. Reasoning being that it allows people to jump to the issue from here on codereview.cc as well.

src/DirectManipulationMotionFamily.swift
75

It needs to be tested, but I think setting the gesture's translation might affect the gesture's velocity calculations. It also means if other objects are watching this gesture recognizer that they might get unexpected results.

This revision now requires changes to proceed.Aug 25 2016, 9:26 PM
rcameron updated this revision to Diff 6466.Aug 26 2016, 12:05 PM

update DraggablePerformer to use a transform instead of modifying frame directly

rcameron updated this object.Aug 26 2016, 12:07 PM
rcameron marked an inline comment as done.
featherless added inline comments.Sep 8 2016, 7:41 PM
src/DirectManipulationMotionFamily.swift
22

I'd like to encourage us to treat "open" similarly to the "!" character, meaning we use it sparingly and with strong justification.

I'm not convinced yet that we want to allow clients to subclass and extend performers, so in this case I think we should keep the performer private.

featherless requested changes to this revision.Sep 9 2016, 1:33 PM
This revision now requires changes to proceed.Sep 9 2016, 1:33 PM
rcameron updated this revision to Diff 6637.Sep 12 2016, 1:32 PM
  • modify ACLs
rcameron marked an inline comment as done.Sep 12 2016, 1:35 PM

I made GesturePerformer internal to handle the eventual case of subclasses being created in separate files.

featherless requested changes to this revision.Sep 12 2016, 1:51 PM
featherless added inline comments.
src/DirectManipulationMotionFamily.swift
22

internal is the default ACL so you can leave it out here and throughout.

This revision now requires changes to proceed.Sep 12 2016, 1:51 PM
featherless added inline comments.Sep 12 2016, 1:54 PM
src/DirectManipulationMotionFamily.swift
40

If we made configure an internal standalone helper method could we remove the need to subclass a base type?

Or, flipping the question: why do we need a common base class?

rcameron updated this revision to Diff 6638.Sep 12 2016, 1:55 PM
  • removing explicit uses of internal
rcameron updated this revision to Diff 6651.Sep 12 2016, 5:43 PM
rcameron marked an inline comment as done.
  • remove use of internal
  • remove GesturePerformer base class
rcameron marked an inline comment as done.Sep 12 2016, 5:59 PM
rcameron added inline comments.
src/DirectManipulationMotionFamily.swift
40

Removed common base class

featherless accepted this revision.Sep 12 2016, 7:24 PM

Looks good! Just a few small comments.

I think that we can get 100% coverage here if we write a test with a faked UIPanGestureRecognizer subclass that can simulate events. Can file an issue and follow-up in another diff though.

src/DirectManipulationMotionFamily.swift
30

Do methods of a final class have to be marked final?

47

let plan = plan as! Draggable so that we assert here if an invalid plan gets passed.

57

We can define this method signature as we choose, so you can make it UIPanGestureRecognizer and remove the cast on the next line.

This revision is now accepted and ready to land.Sep 12 2016, 7:24 PM

Perhaps change the title of the diff to something along the lines of "Implement draggable plan and rename library to direct manipulation."

rcameron retitled this revision from Internal cleanup to Implement draggable plan and rename library to direct manipulation.Sep 12 2016, 8:38 PM
rcameron updated this revision to Diff 6664.Sep 13 2016, 1:33 PM
rcameron marked an inline comment as done.
  • crash if DraggablePerformer doesnt receive Draggable plan
rcameron marked 4 inline comments as done.Sep 13 2016, 1:45 PM
rcameron added inline comments.
src/DirectManipulationMotionFamily.swift
30

Good catch. There's no way to subclass the class, so this is unnecessary.

rcameron updated this revision to Diff 6682.Sep 13 2016, 1:57 PM
rcameron marked an inline comment as done.
  • merge
rcameron closed this revision.Sep 13 2016, 1:58 PM