remove getChild and getChildCount in UiElement

wait for sentinel in the corner cases when parent updates slowly

Change-Id: I9bbe36c6566dca8453ec12030d17703790ae9a25

10 files changed, 89 insertions, 78 deletions

diff --git a/src/com/google/android/droiddriver/UiElement.java b/src/com/google/android/droiddriver/UiElement.java
index e881475..a0d2ae3 100644
--- a/src/com/google/android/droiddriver/UiElement.java
+++ b/src/com/google/android/droiddriver/UiElement.java
@@ -185,9 +185,19 @@ public interface UiElement {
 
   /**
    * Gets an immutable {@link List} of immediate children that satisfy
-   * {@code predicate}. It always filters children that are null.
+   * {@code predicate}. It always filters children that are null. This gives a
+   * low level access to the underlying data. Do not use it unless you are sure
+   * about the subtle details. Note the count may not be what you expect. For
+   * instance, a dynamic list may show more items when scrolling beyond the end,
+   * varying the count. The count also depends on the driver implementation:
+   * <ul>
+   * <li>{@link InstrumentationDriver} includes all.</li>
+   * <li>the Accessibility API (which {@link UiAutomationDriver} depends on)
+   * does not include off-screen children, but may include invisible on-screen
+   * children.</li>
+   * </ul>
    */
-  List<UiElement> getChildren(Predicate<? super UiElement> predicate);
+  List<? extends UiElement> getChildren(Predicate<? super UiElement> predicate);
 
   /**
    * Filters out invisible children.
@@ -204,27 +214,6 @@ public interface UiElement {
     }
   };
 
-  // TODO: remove getChildCount and getChild.
-  /**
-   * Gets the child at given index.
-   */
-  UiElement getChild(int index);
-
-  /**
-   * Gets the child count. This gives a low level access to the underlying data.
-   * Do not use it unless you are sure about the subtle details. Note the count
-   * may not be what you expect. For instance, a dynamic list may show more
-   * items when scrolling beyond the end, varying the count. The count also
-   * depends on the driver implementation:
-   * <ul>
-   * <li>{@link InstrumentationDriver} includes all.</li>
-   * <li>the Accessibility API (which {@link UiAutomationDriver} depends on)
-   * does not include off-screen children, but may include invisible on-screen
-   * children.</li>
-   * </ul>
-   */
-  int getChildCount();
-
   /**
    * Gets the parent.
    */
diff --git a/src/com/google/android/droiddriver/base/BaseUiElement.java b/src/com/google/android/droiddriver/base/BaseUiElement.java
index 2e038f1..9289e36 100644
--- a/src/com/google/android/droiddriver/base/BaseUiElement.java
+++ b/src/com/google/android/droiddriver/base/BaseUiElement.java
@@ -122,8 +122,9 @@ public abstract class BaseUiElement implements UiElement {
     perform(SwipeAction.toScroll(direction));
   }
 
-  @Override
-  public abstract BaseUiElement getChild(int index);
+  protected abstract int getChildCount();
+
+  protected abstract BaseUiElement getChild(int index);
 
   protected abstract InputInjector getInjector();
 
@@ -134,16 +135,16 @@ public abstract class BaseUiElement implements UiElement {
   }
 
   @Override
-  public List<UiElement> getChildren(Predicate<? super UiElement> predicate) {
+  public List<BaseUiElement> getChildren(Predicate<? super UiElement> predicate) {
     if (predicate == null) {
       predicate = Predicates.notNull();
     } else {
       predicate = Predicates.and(Predicates.notNull(), predicate);
     }
 
-    List<UiElement> list = Lists.newArrayList();
+    List<BaseUiElement> list = Lists.newArrayList();
     for (int i = 0; i < getChildCount(); i++) {
-      UiElement child = getChild(i);
+      BaseUiElement child = getChild(i);
       if (predicate.apply(child)) {
         list.add(child);
       }
diff --git a/src/com/google/android/droiddriver/finders/By.java b/src/com/google/android/droiddriver/finders/By.java
index 69e7c73..152369a 100644
--- a/src/com/google/android/droiddriver/finders/By.java
+++ b/src/com/google/android/droiddriver/finders/By.java
@@ -337,8 +337,9 @@ public class By {
         if (parent == null) {
           return false;
         }
-        for (int i = 0; i < parent.getChildCount(); i++) {
-          if (siblingFinder.matches(parent.getChild(i))) {
+        // Do not care if the sibling is visible
+        for (UiElement child : parent.getChildren(null)) {
+          if (siblingFinder.matches(child)) {
             return true;
           }
         }
diff --git a/src/com/google/android/droiddriver/finders/ByXPath.java b/src/com/google/android/droiddriver/finders/ByXPath.java
index f85335b..50b1d3e 100644
--- a/src/com/google/android/droiddriver/finders/ByXPath.java
+++ b/src/com/google/android/droiddriver/finders/ByXPath.java
@@ -137,18 +137,7 @@ public class ByXPath implements Finder {
     element.setAttribute(Attribute.BOUNDS.getName(), uiElement.getBounds().toShortString());
 
     // TODO: visitor pattern
-    int childCount = uiElement.getChildCount();
-    for (int i = 0; i < childCount; i++) {
-      BaseUiElement child = uiElement.getChild(i);
-      if (child == null) {
-        Logs.log(Log.INFO, "Skip null child for " + uiElement);
-        continue;
-      }
-      if (!child.isVisible()) {
-        Logs.log(Log.VERBOSE, "Skip invisible child: " + child);
-        continue;
-      }
-
+    for (BaseUiElement child : uiElement.getChildren(UiElement.VISIBLE)) {
       element.appendChild(child.getDomNode());
     }
     return element;
diff --git a/src/com/google/android/droiddriver/finders/MatchFinder.java b/src/com/google/android/droiddriver/finders/MatchFinder.java
index 22f3fc2..2818fde 100644
--- a/src/com/google/android/droiddriver/finders/MatchFinder.java
+++ b/src/com/google/android/droiddriver/finders/MatchFinder.java
@@ -52,17 +52,7 @@ public abstract class MatchFinder implements Finder {
       Logs.log(Log.INFO, "Found match: " + context);
       return context;
     }
-    int childCount = context.getChildCount();
-    for (int i = 0; i < childCount; i++) {
-      UiElement child = context.getChild(i);
-      if (child == null) {
-        Logs.log(Log.INFO, "Skip null child for " + context);
-        continue;
-      }
-      if (!child.isVisible()) {
-        Logs.log(Log.VERBOSE, "Skip invisible child: " + child);
-        continue;
-      }
+    for (UiElement child : context.getChildren(UiElement.VISIBLE)) {
       try {
         return find(child);
       } catch (ElementNotFoundException enfe) {
diff --git a/src/com/google/android/droiddriver/instrumentation/ViewElement.java b/src/com/google/android/droiddriver/instrumentation/ViewElement.java
index 587b65f..5106975 100644
--- a/src/com/google/android/droiddriver/instrumentation/ViewElement.java
+++ b/src/com/google/android/droiddriver/instrumentation/ViewElement.java
@@ -199,7 +199,7 @@ public class ViewElement extends BaseUiElement {
   }
 
   @Override
-  public int getChildCount() {
+  protected int getChildCount() {
     if (!(view instanceof ViewGroup)) {
       return 0;
     }
@@ -207,7 +207,7 @@ public class ViewElement extends BaseUiElement {
   }
 
   @Override
-  public ViewElement getChild(int index) {
+  protected ViewElement getChild(int index) {
     if (!(view instanceof ViewGroup)) {
       return null;
     }
diff --git a/src/com/google/android/droiddriver/scroll/AbstractSentinelStrategy.java b/src/com/google/android/droiddriver/scroll/AbstractSentinelStrategy.java
index 9fbf339..345725f 100644
--- a/src/com/google/android/droiddriver/scroll/AbstractSentinelStrategy.java
+++ b/src/com/google/android/droiddriver/scroll/AbstractSentinelStrategy.java
@@ -15,7 +15,11 @@
  */
 package com.google.android.droiddriver.scroll;
 
+import com.google.android.droiddriver.DroidDriver;
 import com.google.android.droiddriver.UiElement;
+import com.google.android.droiddriver.exceptions.ElementNotFoundException;
+import com.google.android.droiddriver.finders.By;
+import com.google.android.droiddriver.finders.Finder;
 import com.google.android.droiddriver.scroll.Direction.LogicalDirection;
 import com.google.android.droiddriver.scroll.Direction.PhysicalToLogicalConverter;
 import com.google.android.droiddriver.scroll.Direction.PhysicalDirection;
@@ -41,11 +45,16 @@ public abstract class AbstractSentinelStrategy implements SentinelStrategy {
       this.description = description;
     }
 
+    /**
+     * Gets the sentinel, which must be an immediate child of {@code parent} --
+     * not a descendant. Note this could be null if {@code parent} has not
+     * finished updating.
+     */
     public UiElement getSentinel(UiElement parent) {
       return getSentinel(parent.getChildren(predicate));
     }
 
-    protected abstract UiElement getSentinel(List<UiElement> children);
+    protected abstract UiElement getSentinel(List<? extends UiElement> children);
 
     @Override
     public String toString() {
@@ -68,7 +77,7 @@ public abstract class AbstractSentinelStrategy implements SentinelStrategy {
     }
 
     @Override
-    protected UiElement getSentinel(List<UiElement> children) {
+    protected UiElement getSentinel(List<? extends UiElement> children) {
       return original.getSentinel(children);
     }
   }
@@ -79,8 +88,8 @@ public abstract class AbstractSentinelStrategy implements SentinelStrategy {
   public static final GetStrategy FIRST_CHILD_GETTER = new GetStrategy(Predicates.alwaysTrue(),
       "FIRST_CHILD") {
     @Override
-    protected UiElement getSentinel(List<UiElement> children) {
-      return children.get(0);
+    protected UiElement getSentinel(List<? extends UiElement> children) {
+      return children.isEmpty() ? null : children.get(0);
     }
   };
 
@@ -90,8 +99,8 @@ public abstract class AbstractSentinelStrategy implements SentinelStrategy {
   public static final GetStrategy LAST_CHILD_GETTER = new GetStrategy(Predicates.alwaysTrue(),
       "LAST_CHILD") {
     @Override
-    protected UiElement getSentinel(List<UiElement> children) {
-      return children.get(children.size() - 1);
+    protected UiElement getSentinel(List<? extends UiElement> children) {
+      return children.isEmpty() ? null : children.get(children.size() - 1);
     }
   };
 
@@ -102,29 +111,59 @@ public abstract class AbstractSentinelStrategy implements SentinelStrategy {
   public static final GetStrategy SECOND_CHILD_GETTER = new GetStrategy(Predicates.alwaysTrue(),
       "SECOND_CHILD") {
     @Override
-    protected UiElement getSentinel(List<UiElement> children) {
-      return children.get(1);
+    protected UiElement getSentinel(List<? extends UiElement> children) {
+      return children.size() <= 1 ? null : children.get(1);
     }
   };
 
+  private static class SentinelFinder implements Finder {
+    private final GetStrategy getStrategy;
+
+    public SentinelFinder(GetStrategy getStrategy) {
+      this.getStrategy = getStrategy;
+    }
+
+    @Override
+    public UiElement find(UiElement parent) {
+      UiElement sentinel = getStrategy.getSentinel(parent);
+      if (sentinel == null) {
+        throw new ElementNotFoundException(this);
+      }
+      return sentinel;
+    }
+
+    @Override
+    public String toString() {
+      return String.format("SentinelFinder{%s}", getStrategy);
+    }
+  }
+
   protected final GetStrategy backwardGetStrategy;
   protected final GetStrategy forwardGetStrategy;
   protected final PhysicalToLogicalConverter physicalToLogicalConverter;
+  private final SentinelFinder backwardSentinelFinder;
+  private final SentinelFinder forwardSentinelFinder;
 
   public AbstractSentinelStrategy(GetStrategy backwardGetStrategy, GetStrategy forwardGetStrategy,
       PhysicalToLogicalConverter physicalToLogicalConverter) {
     this.backwardGetStrategy = backwardGetStrategy;
     this.forwardGetStrategy = forwardGetStrategy;
     this.physicalToLogicalConverter = physicalToLogicalConverter;
+    this.backwardSentinelFinder = new SentinelFinder(backwardGetStrategy);
+    this.forwardSentinelFinder = new SentinelFinder(forwardGetStrategy);
   }
 
-  protected UiElement getSentinel(UiElement parent, PhysicalDirection direction) {
+  protected UiElement getSentinel(DroidDriver driver, Finder parentFinder,
+      PhysicalDirection direction) {
+    // Make sure sentinel exists in parent
+    Finder chainFinder;
     LogicalDirection logicalDirection = physicalToLogicalConverter.toLogicalDirection(direction);
     if (logicalDirection == LogicalDirection.BACKWARD) {
-      return backwardGetStrategy.getSentinel(parent);
+      chainFinder = By.chain(parentFinder, backwardSentinelFinder);
     } else {
-      return forwardGetStrategy.getSentinel(parent);
+      chainFinder = By.chain(parentFinder, forwardSentinelFinder);
     }
+    return driver.on(chainFinder);
   }
 
   @Override
diff --git a/src/com/google/android/droiddriver/scroll/DynamicSentinelStrategy.java b/src/com/google/android/droiddriver/scroll/DynamicSentinelStrategy.java
index 866c71b..3dca121 100644
--- a/src/com/google/android/droiddriver/scroll/DynamicSentinelStrategy.java
+++ b/src/com/google/android/droiddriver/scroll/DynamicSentinelStrategy.java
@@ -187,10 +187,9 @@ public class DynamicSentinelStrategy extends AbstractSentinelStrategy {
 
   @Override
   public boolean scroll(DroidDriver driver, Finder parentFinder, PhysicalDirection direction) {
-    UiElement parent = driver.on(parentFinder);
-    UiElement oldSentinel = getSentinel(parent, direction);
-    parent.scroll(direction);
-    UiElement newSentinel = getSentinel(driver.on(parentFinder), direction);
+    UiElement oldSentinel = getSentinel(driver, parentFinder, direction);
+    oldSentinel.getParent().scroll(direction);
+    UiElement newSentinel = getSentinel(driver, parentFinder, direction);
     return isUpdatedStrategy.isSentinelUpdated(newSentinel, oldSentinel);
   }
 
diff --git a/src/com/google/android/droiddriver/scroll/StaticSentinelStrategy.java b/src/com/google/android/droiddriver/scroll/StaticSentinelStrategy.java
index f83ce2a..e9225aa 100644
--- a/src/com/google/android/droiddriver/scroll/StaticSentinelStrategy.java
+++ b/src/com/google/android/droiddriver/scroll/StaticSentinelStrategy.java
@@ -26,10 +26,12 @@ import com.google.android.droiddriver.scroll.Direction.PhysicalDirection;
 
 /**
  * Determines whether scrolling is possible by checking whether the last child
- * in the logical scroll direction is fully visible. Use this when the count of
- * children is static, and {@link UiElement#getChildCount()} includes all
- * children no matter if it is visible. Currently {@link InstrumentationDriver}
- * behaves this way.
+ * in the logical scroll direction is fully visible. This assumes the count of
+ * children is static, and {@link UiElement#getChildren} includes all children
+ * no matter if it is visible. Currently {@link InstrumentationDriver} behaves
+ * this way.
+ * <p>
+ * This does not work if a child is larger than the physical size of the parent.
  */
 public class StaticSentinelStrategy extends AbstractSentinelStrategy {
   /**
@@ -47,11 +49,12 @@ public class StaticSentinelStrategy extends AbstractSentinelStrategy {
 
   @Override
   public boolean scroll(DroidDriver driver, Finder parentFinder, PhysicalDirection direction) {
-    UiElement parent = driver.on(parentFinder);
+    UiElement sentinel = getSentinel(driver, parentFinder, direction);
+    UiElement parent = sentinel.getParent();
     // If the last child in the logical scroll direction is fully visible, no
     // more scrolling is possible
     Rect visibleBounds = parent.getVisibleBounds();
-    if (visibleBounds.contains(getSentinel(parent, direction).getBounds())) {
+    if (visibleBounds.contains(sentinel.getBounds())) {
       return false;
     }
 
diff --git a/src/com/google/android/droiddriver/uiautomation/UiAutomationElement.java b/src/com/google/android/droiddriver/uiautomation/UiAutomationElement.java
index f8ffab6..e570938 100644
--- a/src/com/google/android/droiddriver/uiautomation/UiAutomationElement.java
+++ b/src/com/google/android/droiddriver/uiautomation/UiAutomationElement.java
@@ -161,12 +161,12 @@ public class UiAutomationElement extends BaseUiElement {
   }
 
   @Override
-  public int getChildCount() {
+  protected int getChildCount() {
     return node.getChildCount();
   }
 
   @Override
-  public UiAutomationElement getChild(int index) {
+  protected UiAutomationElement getChild(int index) {
     AccessibilityNodeInfo child = node.getChild(index);
     return child == null ? null : context.getUiElement(child);
   }