Deprecate History#iterator() and History#reverseIterator(), use more …

…explicit methods returning Iterables instead.

The raw `iterator` methods on `History` do not make it clear you're iterating from the top or bottom of the
stack. Introduced new methods `framesFromTop` and `framesFromBottom` to make it very clear. This also lets
you use `for each` loops for both forward and backward iteration.

Fix for issue #261.

flow/src/main/java/flow/Flow.java

@@ -51,8 +51,8 @@ public final class Flow {
  * Activity's {@link Activity#onResume()} method in the current Android task. In practice
  * this boils down to two rules:
  * <ol>
- *  <li>In views, do not access Flow before {@link View#onAttachedToWindow()} is called.
- *  <li>In activities, do not access flow before {@link Activity#onResume()} is called.
+ * <li>In views, do not access Flow before {@link View#onAttachedToWindow()} is called.
+ * <li>In activities, do not access flow before {@link Activity#onResume()} is called.
  * </ol>
  */
  @NonNull public static Flow get(@NonNull Context context) {
@@ -243,9 +243,7 @@ public void set(@NonNull final Object newTopKey) {
  int count = 0;
  // Search backward to see if we already have newTop on the stack
  Object preservedInstance = null;
- for (Iterator<Object> it = history.reverseIterator(); it.hasNext(); ) {
- Object entry = it.next();
-
+ for (Object entry : history.framesFromTop()) {
  // If we find newTop on the stack, pop back to it.
  if (entry.equals(newTopKey)) {
  for (int i = 0; i < history.size() - count; i++) {
@@ -321,8 +319,8 @@ private void move(PendingTraversal pendingTraversal) {
  }
 
  private static History preserveEquivalentPrefix(History current, History proposed) {
- Iterator<Object> oldIt = current.reverseIterator();
- Iterator<Object> newIt = proposed.reverseIterator();
+ Iterator<Object> oldIt = current.framesFromTop().iterator();
+ Iterator<Object> newIt = proposed.framesFromTop().iterator();
 
  History.Builder preserving = current.buildUpon().clear();
 

flow/src/main/java/flow/History.java

@@ -32,6 +32,10 @@
 
 /**
  * Describes the history of a {@link Flow} at a specific point in time.
+ *
+ * <p><em>Note: use of this class as an {@link Iterable} is deprecated. Use {@link
+ * #framesFromBottom()}
+ * and {@link #framesFromTop()} instead.</em>
  */
 public final class History implements Iterable<Object> {
 
@@ -46,17 +50,35 @@ public final class History implements Iterable<Object> {
  return emptyBuilder().push(key).build();
  }
 
+ private static <T> Iterator<T> iterateFromTop(List<Object> history) {
+ return new ReadStateIterator<>(history.iterator());
+ }
+
+ private static <T> Iterator<T> iterateFromBottom(List<Object> history) {
+ return new ReadStateIterator<>(new ReverseIterator<>(history));
+ }
+
  private History(List<Object> history) {
  checkArgument(history != null && !history.isEmpty(), "History may not be empty");
  this.history = history;
  }
 
- @NonNull public <T> Iterator<T> reverseIterator() {
- return new ReadStateIterator<>(history.iterator());
+ @NonNull public <T> Iterable<T> framesFromTop() {
+ return new HistoryIterable<>(history, true);
  }
 
- @NonNull @Override public Iterator<Object> iterator() {
- return new ReadStateIterator<>(new ReverseIterator<>(history));
+ @NonNull public <T> Iterable<T> framesFromBottom() {
+ return new HistoryIterable<>(history, false);
+ }
+
+ /** @deprecated Use {@link #framesFromTop()} instead. */
+ @Deprecated @NonNull public <T> Iterator<T> reverseIterator() {
+ return iterateFromTop(history);
+ }
+
+ /** @deprecated Use {@link #framesFromBottom()} instead. */
+ @Deprecated @NonNull @Override public Iterator<Object> iterator() {
+ return iterateFromBottom(history);
  }
 
  public int size() {
@@ -194,6 +216,24 @@ public Object pop() {
  }
  }
 
+ private static class HistoryIterable<T> implements Iterable<T> {
+ private final List<Object> history;
+ private final boolean fromTop;
+
+ HistoryIterable(List<Object> history, boolean fromTop) {
+ this.history = history;
+ this.fromTop = fromTop;
+ }
+
+ @NonNull @Override public Iterator<T> iterator() {
+ if (fromTop) {
+ return iterateFromTop(history);
+ } else {
+ return iterateFromBottom(history);
+ }
+ }
+ }
+
  private static class ReverseIterator<T> implements Iterator<T> {
  private final ListIterator<T> wrapped;
 
@@ -214,6 +254,7 @@ private static class ReverseIterator<T> implements Iterator<T> {
  }
  }
 
+ /** Wraps an {@link Iterator} and delegates to it, but throws on {@link #remove()}. */
  private static class ReadStateIterator<T> implements Iterator<T> {
  private final Iterator<Object> iterator;
 

flow/src/main/java/flow/InternalLifecycleIntegration.java

@@ -25,7 +25,6 @@
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import java.util.ArrayList;
-import java.util.Iterator;
 
 import static flow.Preconditions.checkArgument;
 import static flow.Preconditions.checkNotNull;
@@ -117,9 +116,7 @@ public InternalLifecycleIntegration() {
  static void addHistoryToIntent(Intent intent, History history, KeyParceler parceler) {
  Bundle bundle = new Bundle();
  ArrayList<Parcelable> parcelables = new ArrayList<>(history.size());
- final Iterator<Object> keys = history.reverseIterator();
- while (keys.hasNext()) {
- Object key = keys.next();
+ for (Object key : history.framesFromTop()) {
  parcelables.add(State.empty(key).toBundle(parceler));
  }
  bundle.putParcelableArrayList(PERSISTENCE_KEY, parcelables);
@@ -207,9 +204,7 @@ private static History selectHistory(Intent intent, History saved, History defau
  private static void save(Bundle bundle, KeyParceler parceler, History history,
  KeyManager keyManager) {
  ArrayList<Parcelable> parcelables = new ArrayList<>(history.size());
- final Iterator<Object> keys = history.reverseIterator();
- while (keys.hasNext()) {
- Object key = keys.next();
+ for (Object key : history.framesFromTop()) {
  if (!key.getClass().isAnnotationPresent(NotPersistent.class)) {
  parcelables.add(keyManager.getState(key).toBundle(parceler));
  }

flow/src/main/java/flow/NotPersistentHistoryFilter.java

@@ -1,7 +1,6 @@
 package flow;
 
 import android.support.annotation.NonNull;
-import java.util.Iterator;
 
 /**
  * Default implementation of {@link HistoryFilter}, enforces the contract
@@ -11,9 +10,7 @@ class NotPersistentHistoryFilter implements HistoryFilter {
  @NonNull @Override public History scrubHistory(@NonNull History history) {
  History.Builder builder = History.emptyBuilder();
 
- final Iterator<Object> keys = history.reverseIterator();
- while (keys.hasNext()) {
- Object key = keys.next();
+ for (Object key : history.framesFromTop()) {
  if (!key.getClass().isAnnotationPresent(NotPersistent.class)) {
  builder.push(key);
  }

flow/src/test/java/flow/FlowTest.java

@@ -135,8 +135,8 @@ class Ourrobouros implements Dispatcher {
  @Override
  public void dispatch(@NonNull Traversal traversal, @NonNull TraversalCallback onComplete) {
  assertThat(firstHistory).hasSameSizeAs(flow.getHistory());
- Iterator<Object> original = firstHistory.iterator();
- for (Object o : flow.getHistory()) {
+ Iterator<Object> original = firstHistory.framesFromBottom().iterator();
+ for (Object o : flow.getHistory().framesFromBottom()) {
  assertThat(o).isEqualTo(original.next());
  }
  onComplete.onTraversalCompleted();
@@ -460,9 +460,7 @@ static class Picky {
  @NonNull @Override public History scrubHistory(@NonNull History history) {
  History.Builder builder = History.emptyBuilder();
 
- final Iterator<Object> keys = history.reverseIterator();
- while (keys.hasNext()) {
- Object key = keys.next();
+ for (Object key : history.framesFromTop()) {
  if (!key.equals(able)) {
  builder.push(key);
  }

flow/src/test/java/flow/HistoryTest.java

@@ -105,6 +105,14 @@ public class HistoryTest {
  }
  }
 
+ @Test public void framesFromBottom() {
+ List<Object> paths = new ArrayList<>(Arrays.<Object>asList(ABLE, BAKER, CHARLIE));
+ History history = History.emptyBuilder().pushAll(paths).build();
+ for (Object o : history.framesFromBottom()) {
+ assertThat(o).isSameAs(paths.remove(paths.size() - 1));
+ }
+ }
+
  @Test public void reverseIterator() {
  List<Object> paths = new ArrayList<>(Arrays.<Object>asList(ABLE, BAKER, CHARLIE));
  History history = History.emptyBuilder().pushAll(paths).build();
@@ -114,6 +122,14 @@ public class HistoryTest {
  }
  }
 
+ @Test public void framesFromTop() {
+ List<Object> paths = new ArrayList<>(Arrays.<Object>asList(ABLE, BAKER, CHARLIE));
+ History history = History.emptyBuilder().pushAll(paths).build();
+ for (Object o : history.framesFromTop()) {
+ assertThat(o).isSameAs(paths.remove(0));
+ }
+ }
+
  @Test public void emptyBuilderPeekIsNullable() {
  assertThat(History.emptyBuilder().peek()).isNull();
  }

flow/src/test/java/flow/ReentranceTest.java

@@ -390,7 +390,7 @@ static class Error extends TestKey {
 
  private void verifyHistory(History history, Object... keys) {
  List<Object> actualKeys = new ArrayList<>(history.size());
- for (Object entry : history) {
+ for (Object entry : history.framesFromBottom()) {
  actualKeys.add(entry);
  }
  assertThat(actualKeys).containsExactly(keys);