[api] Add support for wraparound animation #88

jinatonic · 2018-02-18T06:07:48Z

closes #72

danh32 · 2018-02-18T06:53:02Z

ticker/src/main/java/com/robinhood/ticker/TickerColumn.java

@@ -115,7 +115,7 @@ float getMinimumRequiredWidth() {
     * A helper method for populating {@link #startIndex} and {@link #endIndex} given the
     * current and target characters for the animation.
     */
-    private void setCharacterIndices() {
+    private void setCharacterIndices(boolean wraparound) {


i've just taken a quick glance at this, will do a full review tomorrow. just wanna ask - does the wraparound always force an upward animation? if i'm going from 9 to 8, i'm assuming we still animate downwards?

yep, it will always wrap-around and animate upwards.

danh32 · 2018-02-18T07:01:55Z

ticker/src/main/java/com/robinhood/ticker/TickerColumn.java

+                    numFromStart,
+                    numToEnd
+            );
+            currentCharacterList = modifiedWraparoundCharList;


i wonder if some of the internals here would be made a lot easier by using a circular buffer/ring type instead of a list? i'm not sure if it complicates other places, but we could compute which direction would be a shorter distance to travel and use that to determine whether we should go upwards or downwards. i think it would avoid needing to do these arraycopys, and possibly some other shenanigans.

yeah i started going down that route, but it ended up being quite a bit of change for a somewhat uncommon use-case (a lot of the internal drawing logic had to be re-worked). I decided that it was easier to just use the exact same drawing logic and just properly set up the constraints for it.

i do think some of this logic can be simplified using better data structure, just too lazy to do it now lol..

danh32 · 2018-02-18T07:03:50Z

ticker/src/main/java/com/robinhood/ticker/TickerView.java

+     *
+     * @param characterListWraparound whether or not to enable character list wraparound.
+     */
+    public void setCharacterListWraparound(boolean characterListWraparound) {


the reason i asked about the behavior, is it seems to me like this should be the default behavior (from a ui perspective), so it seems odd to expose a flag for it.

you are saying that wrap-around should be the default behavior? im not sure if having the ticker always animate in one direction is good haha. i feel like wraparound is a rare/specific use-case, basically when you are trying to build a timer or something.

i misunderstood a bit, lemme back up.

i think ideal behavior would be animating up when doing so would cause a wraparound, but the total distance via wraparound is lower than the total distance via not wrapping around. for cases where we go from 9 -> 0, that's a distance of 1 via wraparound, but 8 via not wrapping around. we should just always do the shortest.

having a force flag seems odd, i'm not sure that folks would want an 'always up behavior'. i guess some apps might, but it seems pretty limited.

so, i think we should work towards doing the shortest distance (wraparound or not), and not put it behind a flag (it should just be the behavior of the lib).

i see, yeah i like that.

danh32 · 2018-04-04T20:39:31Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+ * it will know that it has to go from 'd' to 'c' to 'b', and these are the characters
+ * that show up during the animation scroll.
+ *
+ * @author Jin Cao, Robinhood


danh32 · 2018-04-04T20:41:02Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+        }
+    }
+
+    public class AnimationCharacterIndices {


this could just be a Pair<Integer, Integer>

this is assuming we could also keep this class internal, which would be great

i exposed this class to allow for overriding for custom behavior, but you are right using a pair is easier until other needs arise.

actually im reverting back to use a custom class because Pair is an android class and it breaks my unit tests

danh32 · 2018-04-04T20:43:58Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+    }
+
+    private int getIndexOfChar(char c) {
+        if (c == TickerUtils.EMPTY_CHAR) {


i'm not loving the special case handling of the empty_char. is there any way to get this for free? like enforcing the empty_char is always at 0? or why do we even need it at 0? can we eliminate that restriction?

looping back here - maybe user passes in "abcdef...", and when we wrap to the TickerCharacterList, we add the empty_char at index 0, then don't have to special case here?

so i went back and forth on this one for a while. the reason why the empty char is needed in the saved character list is that we use the character list for animation purposes (so we don't have to construct a new char array for each animation). I originally played around with saving the empty char index (which is 0) into the map. however, this actually made wrap-around animation a little more complicated than i would like.

e.g., given the characters "abcd", the persisted character list for animation is "_abcdabcd". the reason why we save two copies is so that we can easily animate from 'd' -> 'a' this way. Now, if we persist the actual indices, _ would be 0, 'a' would be 1, 'b' would be 2, and so on. However, computing the target distance wraparound would then become something like numOriginalCharacters - startIndex - 1 + endIndex, and I didn't like the arbitrary 1 there.

i liked this way slightly more in the end because from the perspective of character indices there is no concept of added characters, and let getIndexOfChar deal with manipulating the saved indices.

eh, i can go either way at this point. im gonna see if i can think of a better way to handle empty chars in general. the main pain point as pointed out earlier is that i want to have it included in the main char array for animation purposes which i dont think we can avoid.

danh32 · 2018-04-04T20:46:28Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+    public TickerCharacterList(String characterList) {
+        if (characterList.contains(Character.toString(TickerUtils.EMPTY_CHAR))) {
+            throw new IllegalArgumentException(
+                    "You don't need to include TickerUtils.EMPTY_CHAR in the character list.");


message is a bit misleading - "You cannot include TickerUtils.EMPTY_CHAR in the character list"

danh32 · 2018-04-04T20:49:52Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+    // The saved character list will always be of the format: EMPTY, list, list
+    private final char[] characterList;
+    // A minor optimization so that we can cache the indices of each character.
+    private final Map<Character, Integer> characterIndicesMap;


does this still work now that the same char maps to multiple indices?

yep see explanation above. it will use the original index in the list.

danh32 · 2018-04-04T20:51:25Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+ */
+public class TickerCharacterList {
+    private final int numOriginalCharacters;
+    // The saved character list will always be of the format: EMPTY, list, list


clever use of a second list to get the wraparound. i haven't gotten that far yet, but curious how we handle wraparound if you're at the end of the second list? seems like whatever the solution is there should be applicable to the first list

yeah the second list is entirely for wraparound, the persisted indices for the characters always refer to the indices in the first list so shouldn't have any problems there.

danh32 · 2018-04-04T20:56:21Z

ticker/src/main/java/com/robinhood/ticker/TickerColumnManager.java

-    void setCharacterLists(String... characterLists) {
-        this.characterLists = new ArrayList<>(characterLists.length);
-        this.characterIndicesMaps = new ArrayList<>(characterLists.length);
+    void setCharacterLists(TickerCharacterList... characterLists) {


since this used to take in a String... and each TickerCharacterList takes in a String, can we hide TickerCharacterList from the api, and just wrap them internally here?

yeah i dont see a huge use-case for exposing this for allowing extensibility so ill change the public API to use string

danh32 · 2018-04-04T21:03:33Z

ticker/src/main/java/com/robinhood/ticker/TickerCharacterList.java

+            final int nonWrapDistance = startIndex - endIndex;
+            final int wrapDistance = numOriginalCharacters - startIndex + endIndex;
+            if (wrapDistance < nonWrapDistance) {
+                endIndex = numOriginalCharacters + endIndex;


yeah, i'm still not 100% sure about the second list thing -

say we wraparound from 9 -> 3, then we go from 3 -> 7, 7 -> 9

at this point, are we displaying the 9 from the second list? if so, i think if we try to go from 9 -> 3 again, won't it always go down instead of wrapping around?

see explanation above, the indices for animation will always use the first list, the second list is entirely for drawing the animation.

[api] Add support for wraparound animation

f396b95

jinatonic self-assigned this Feb 18, 2018

jinatonic requested a review from danh32 February 18, 2018 06:07

danh32 reviewed Feb 18, 2018

View reviewed changes

jinatonic added 2 commits March 4, 2018 15:59

Extracted out TickerCharacterList class

b725312

copyright

506de6a

danh32 reviewed Apr 4, 2018

View reviewed changes

feedback

ae8f4e9

danh32 merged commit 65fcb60 into master Apr 19, 2018

jinatonic deleted the wrap_around branch April 19, 2018 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Add support for wraparound animation #88

[api] Add support for wraparound animation #88

jinatonic commented Feb 18, 2018

danh32 Feb 18, 2018

jinatonic Feb 18, 2018

danh32 Feb 18, 2018

jinatonic Feb 18, 2018

danh32 Feb 18, 2018

jinatonic Feb 18, 2018

danh32 Feb 21, 2018

jinatonic Feb 22, 2018

danh32 Apr 4, 2018

danh32 Apr 4, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

jinatonic Apr 5, 2018

danh32 Apr 4, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

danh32 Apr 4, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

danh32 Apr 4, 2018

jinatonic Apr 5, 2018

[api] Add support for wraparound animation #88

[api] Add support for wraparound animation #88

Conversation

jinatonic commented Feb 18, 2018

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

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