Migrate composition APIs to emitter objects.
ClosedPublic

Authored by featherless on Aug 16 2016, 11:09 AM.

Details

Summary

The old block-based API was confusing to use and did not work with Xcode autocompletion.

This new object-based API removes a layer of scope and plays more nicely with Xcode autocompletion.

Closes https://github.com/material-motion/material-motion-runtime-objc/issues/76

Diff Detail

Repository
rMDMRUNTIMEOBJC material-motion/runtime-objc
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
featherless retitled this revision from to Migrate composition APIs to emitter objects..Aug 16 2016, 11:09 AM
featherless updated this object.
featherless edited the test plan for this revision. (Show Details)
Restricted Application added a project: Material Motion. · View Herald Transcript
willlarche added inline comments.
src/MDMPerforming.h
102

I'm concerned about 'emitter.' When I saw the title of this CL I assumed it had to do with particle emitters.

src/private/MDMPerformerGroup.m
112–113

Nice

markwei added inline comments.
tests/unit/ComposedPerformanceTests.swift
60

not sure I like this. Before, a Transaction was always created and provided to you.

That provided the nice benefit that you cannot misuse Transactions - a transaction is always in a clean state (because the scheduler creates it). With this change, a Performer could conceivably save a reference to an old Transaction and reuse that reference, causing old Plans to be mistakenly re-committed.

I also believe that the "Xcode autocompletion" issue is solvable in other ways.

appsforartists added inline comments.
src/MDMPerforming.h
102

Emit, publish, and dispatch are all common names for things that broadcast events on a stream.

tests/unit/ComposedPerformanceTests.swift
60

I'm still experimenting with using reactive techniques to implement transactions. In that case, you could just have a stream that represents a transaction and add plans to it whenever you like. The Scheduler would subscribe to the stream and react every time a new plan is published.

featherless added inline comments.Aug 18 2016, 10:43 AM
tests/unit/ComposedPerformanceTests.swift
60

The concern of Transactions being reused is valid and likely to happen. I expect that if someone does reuse a Transaction for some reason that they will quickly learn that it causes more problems than it's worth and choose to create a new Transaction each time they need to instead.

If there is demand for it we could also provide an emitter.transact API that is similar to the transact function API. I feel that the emit API is less verbose though and still am leaning toward it.

markwei added inline comments.Aug 18 2016, 3:49 PM
tests/unit/ComposedPerformanceTests.swift
60

I do think that reusing Transactions is an issue that won't be easily caught. Adding the same Plan twice (different Performer instances are always created) usually results in issues that are difficult to debug. For instance, 2 performers may attach 2 gesture listeners of the same type to the target. Whether that will result in bad behavior is unclear and really hard to debug.

But I do like the conciseness of the emitter api given http://codereview.cc/D1482#inline-5716

Would we want to set a bit on the Transaction when it's passed to Scheduler.commitTransaction() so that adding additional Plans to that Transaction throws an error?

featherless added inline comments.Aug 18 2016, 3:55 PM
tests/unit/ComposedPerformanceTests.swift
60

different Performer instances are always created

When would the same plan create two different performers? As per the spec, there should only be one instance of a given performer class per target.

https://material-motion.gitbooks.io/material-motion-starmap/content/specifications/runtime/scheduler.html
Search "One instance of a performer type per target".


For instance, 2 performers may attach 2 gesture listeners of the same type to the target. Whether that will result in bad behavior is unclear and really hard to debug.

This will be a general problem with this system and funnels into the importance of having good tooling for inspecting the state of a given runtime.


The difference between this:

let plan = SomePlan()

transact({ transaction in 
  transaction.add(plan, target)
})

// Sometime later...
transact({ transaction in 
  transaction.add(plan, target)
})

and

let plan = SomePlan()

transaction.add(plan, target)
emitter.emit(transaction)

// Sometime later...
emitter.emit(transaction)

Are no different in my eyes, so I'm not certain that we should be sacrificing API readability/usability for a situation that we can't strictly avoid (storing objects when you likely shouldn't be).

markwei added inline comments.Aug 18 2016, 4:12 PM
tests/unit/ComposedPerformanceTests.swift
60

When would the same plan create two different performers?

Oh man totally misspoke. Yea you're right only one performer is created, but Performer.addPlan() is called twice. Each addPlan() call presumably adds a gesture detector to the target, so two gesture detectors are added total.

The example you provided is not exactly what I had in mind. Here's where I think we'll run into trouble:

With the old API:

transact({ transaction in 
  let plan = FooPlan()
  transaction.add(plan, target)
})

// Sometime later...
transact({ transaction in 
  let plan = BarPlan()
  transaction.add(plan, target)
})

This adds 2 Plans total to the target, FooPlan and BarPlan.

With the new API:

self.transaction = new Transaction() // cached

// Sometime later...
let plan = FooPlan()
transaction.add(plan, target)
emitter.emit(transaction)

// Sometime later...
let plan = BarPlan()
transaction.add(plan, target)
emitter.emit(transaction)

Now we end up with 2 FooPlan and 1 BarPlan.
And because the // Sometime later... is usually in a different context, it's likely that the client will not "remember" that they've previously added something to the Transaction.

markwei added inline comments.Aug 18 2016, 4:17 PM
tests/unit/ComposedPerformanceTests.swift
60

We can totally keep talking about the merits of the 2 apis, but I want to clarify that my stance is leaning towards the emitter api (especially due to the verboseness of java).

featherless added inline comments.Aug 18 2016, 4:22 PM
tests/unit/ComposedPerformanceTests.swift
60

If we're both leaning that way then it may be worth moving in that direction. I worry that we're playing an arms race of "break the API" here and may end up in some cold war-era API feud :P API abusers gonna abuse APIs.

What do you think about having both worlds with the emitter.emit and emitter.transact APIs?

markwei added inline comments.Aug 18 2016, 4:25 PM
tests/unit/ComposedPerformanceTests.swift
60

I always prefer taking a stance on an API decision :)

Let's go with the emitter api. My concern is not with abusers, but rather well-meaning clients that misuse the api and get hard to debug bugs (not always a problem with the client, sometimes a problem with the api).

featherless added inline comments.Aug 18 2016, 4:50 PM
tests/unit/ComposedPerformanceTests.swift
60

Tangential thought: should transactions be cleared when they're committed to a runtime? If we're concerned about transaction reuse then this would be a safe defense.

featherless added inline comments.Aug 19 2016, 11:44 AM
markwei accepted this revision.Aug 21 2016, 5:40 AM
markwei added a reviewer: markwei.
This revision is now accepted and ready to land.Aug 21 2016, 5:40 AM
This revision was automatically updated to reflect the committed changes.