Move operators into their own files.
ClosedPublic

Authored by markwei on Thu, Apr 6, 3:58 PM.

Diff Detail

Repository
rMDMSTREAMSANDROID reactive-motion-android
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
markwei created this revision.Thu, Apr 6, 3:58 PM
Restricted Application completed building Restricted Buildable.Thu, Apr 6, 3:58 PM
Restricted Application added a reviewer: O2: Material Motion. · View Herald TranscriptThu, Apr 6, 3:58 PM
Restricted Application added a project: Material Motion. · View Herald Transcript
Restricted Application added a reviewer: O6: Material Android platform reviewers. · View Herald Transcript
Restricted Application added a reviewer: Material Motion. · View Herald Transcript
Restricted Application failed to build Restricted Buildable!Thu, Apr 6, 3:59 PM
appsforartists retitled this revision from Move operators into its own file. to Move operators into their own files..Fri, Apr 7, 10:22 AM
appsforartists added inline comments.
library/src/main/java/com/google/android/material/motion/Source.java
43

In some of the other files in this diff, you're importing a static function directly and then composing over it. Can you do that here?

getStream().compose(dedupe())

Rather than

getStream().compose(Dedupe.dedupe())

?

library/src/main/java/com/google/android/material/motion/operators/Anchored.java
15

This comment could be more clear:

// Allocate local variables once and recycle the same memory across all invocations

static variables sound scary because they share state across invocations. This comment should allay that fear. :)

32

Can you please add a comment?

// Saves transformation matrices from view into matrix and inverse

The side effects here are implicit, which makes the code harder to read.

library/src/main/java/com/google/android/material/motion/operators/Centroid.java
31

Isn't it the x value from the centroid (and similarly for y)? I know you're not paying attn to docs yet, but this is worth remembering to fix when you get there.

library/src/main/java/com/google/android/material/motion/operators/Rewrite.java
50

Should rewrite and rewriteRange be in separate files?

library/src/main/java/com/google/android/material/motion/operators/Threshold.java
33

Similarly, should threshold and thresholdRange be in separate files?

library/src/test/java/com/google/android/material/motion/operators/GestureOperatorsTests.java
91

Another place you could consider importing and using the function directly (rather than the classes).

markwei marked an inline comment as done.Fri, Apr 7, 11:29 AM
markwei added inline comments.
library/src/main/java/com/google/android/material/motion/Source.java
43

Unfortunately can't in this case. The <Boolean> type inference hint is necessary for type safety, and requires a Class.<Type>method() syntax.

library/src/main/java/com/google/android/material/motion/operators/Anchored.java
32

Standard practice in java is to document the method definition so you don't have to document every method invocation. The IDE for android devs makes that documentation very accessible, which is not very clear if you are just reading this sans-IDE.

library/src/main/java/com/google/android/material/motion/operators/Centroid.java
31

can you clarify what you want me to change here? Using the term fooX and fooY is a reasonable shorthand for x from foo and y from foo in android-land, as the practice comes from the framework code.

library/src/main/java/com/google/android/material/motion/operators/Rewrite.java
50

Hm, part of the motivation for creating these separate operator classes is so I can group related operators. Some foo and fooRange operators may share common logic as well.

library/src/test/java/com/google/android/material/motion/operators/GestureOperatorsTests.java
91

Generally I avoid doing extra cosmetic work in tests (3 extra keystrokes per operator you want to beautify), but I'll do it here since you pointed it out :)

markwei updated this revision to Diff 10673.Fri, Apr 7, 11:30 AM

Comments

Restricted Application failed to build Restricted Buildable!Fri, Apr 7, 11:32 AM
appsforartists accepted this revision.Fri, Apr 7, 11:57 AM

Another thing you could consider, inspired by the JS mixin pattern:

Have one of your foundation operators extend IndefiniteObservable and each other operator extend the one before it, with MotionObservable extending the final operator. I realize it feels a bit weird architecturally, but it would allow you to skip compose for most operations and chain like we do in JS and Swift:

number$().normalize().rubberBand() vs number$.compose(Normalize.normalize(), RubberBand.rubberBand())

library/src/main/java/com/google/android/material/motion/operators/Centroid.java
31

Just the docstrings.

This revision is now accepted and ready to land.Fri, Apr 7, 11:57 AM

Another thing you could consider, inspired by the JS mixin pattern:

Have one of your foundation operators extend IndefiniteObservable and each other operator extend the one before it, with MotionObservable extending the final operator. I realize it feels a bit weird architecturally, but it would allow you to skip compose for most operations and chain like we do in JS and Swift:

number$().normalize().rubberBand() vs number$.compose(Normalize.normalize(), RubberBand.rubberBand())

Thanks for review. The "rabbit hole" mixin approach is interesting but a non-starter for reasons I'm sure you already know. Luckily, the new default interface methods in Java 8 are mixin-like and extension-like. So I'll have a nicer solution soon.

This revision was automatically updated to reflect the committed changes.