Skip to content

#342 Fix#366

Closed
SrinivasanTarget wants to merge 3 commits into
appium:masterfrom
SrinivasanTarget:master
Closed

#342 Fix#366
SrinivasanTarget wants to merge 3 commits into
appium:masterfrom
SrinivasanTarget:master

Conversation

@SrinivasanTarget

Copy link
Copy Markdown
Member

Change list

Added ScrollToUsingPredicates method for android and iOS

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

ScrollTo - Existing method doesn't work for UIScrollableView.It works fine only for UITableView since it is hardcoded. This will again be re-fractored in upcoming commits in same PR.

Now introduced a new method "ScrollToUsingPredicates" for both android and iOS that enables user to define their own predicates that works fine for any scrollable views.

For Predicates in iOS refer:
1)https://github.com/appium/appium/blob/master/docs/en/writing-running-appium/ios_predicate.md
2)https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSPredicate_Class/

For Android Scroll refer:
1)http://developer.android.com/reference/android/support/test/uiautomator/UiScrollable.html#scrollTextIntoView(java.lang.String)

@TikhomirovSergey Your thoughts on this?

As we discussed,this PR will continue to fix existing ScrollTo method.Will work on this.

@SrinivasanTarget SrinivasanTarget changed the title https://github.com/appium/java-client/issues/342 Fix #https://github.com/appium/java-client/issues/342 Fix Apr 24, 2016
@SrinivasanTarget SrinivasanTarget changed the title #https://github.com/appium/java-client/issues/342 Fix #342 Fix Apr 24, 2016
@SrinivasanTarget

Copy link
Copy Markdown
Member Author

#342

@TikhomirovSergey

TikhomirovSergey commented Apr 24, 2016

Copy link
Copy Markdown
Contributor

@SrinivasanTarget
I'm against the proposed PR is it is now. It is much easier to make scrolling methods deprecated and remove it later and make users to use findBySomeAutomation instead. May be it is more correct solution but it is not convenient I think.

I'm looking forward for more complex solution:

  • There is no need to modify io.appium.java_client.ScrollsTo. Let it stay as it is.
  • MobileBy needs for some refacoring I think. It would be cool if it looked like the sample below after the refactoring:
...
public abstract class MobileBy extends By implements Serializable {
    private final String automatorText;

    public MobileBy(String automatorText) {
        this.automatorText = automatorText;
    }

    protected String getAutomatorText() {
        return automatorText;
    }

    ...

    public static class ByIosUIAutomation extends MobileBy  {
        public ByIosUIAutomation(String uiautomationText) {
            super(uiautomationText);
        }

        @SuppressWarnings("unchecked")
        @Override
        public List<WebElement> findElements(SearchContext context) {
            return (List<WebElement>) ((FindsByIosUIAutomation<?>) context)
                    .findElementsByIosUIAutomation(getAutomatorText());
        }

        @Override public WebElement findElement(SearchContext context) {
            return ((FindsByIosUIAutomation<?>) context)
                .findElementByIosUIAutomation(getAutomatorText());
        } 

        @Override public String toString() {
            return "By.IosUIAutomation: " + getAutomatorText();
        } 
    }

    public static class ByAndroidUIAutomator extends MobileBy {
        public ByAndroidUIAutomator(String uiautomatorText) {
            super(uiautomationText);
        }

        @SuppressWarnings("unchecked")
        @Override
        public List<WebElement> findElements(SearchContext context) {
            return (List<WebElement>) ((FindsByAndroidUIAutomator<?>) context)
                    .findElementsByAndroidUIAutomator(getAutomatorText());
        }

        @Override public WebElement findElement(SearchContext context) {
            return ((FindsByAndroidUIAutomator<?>) context)
                .findElementByAndroidUIAutomator(getAutomatorText());
        }

        @Override public String toString() {
            return "By.AndroidUIAutomator: " + automatorText;
        }

        @Override public String getAutomatorText() {
            return automatorText;
        }
    }

    public static class ByAccessibilityId extends By implements Serializable {

        private final String id;

        public ByAccessibilityId(String id) {
            this.id = id;
        }

        @SuppressWarnings("unchecked")
        @Override
        public List<WebElement> findElements(SearchContext context) {
            return (List<WebElement>) ((FindsByAccessibilityId<?>) context)
                .findElementsByAccessibilityId(id);
        }

        @Override public WebElement findElement(SearchContext context) {
            return ((FindsByAccessibilityId<?>) context).findElementByAccessibilityId(id);
        }

        @Override public String toString() {
            return "By.AccessibilityId: " + id;
        }
    }
}
  • Two additional methods are supposed to be added to AndroidDriver/IOSDriver:
 public class AndroidDriver<T extends WebElement>
    extends AppiumDriver<T>
    implements AndroidDeviceActionShortcuts, HasNetworkConnection, PushesFiles, StartsActivity,
    FindsByAndroidUIAutomator<T> {

    ....
    @Override public T scrollTo(String text) {
        String uiScrollables =
            uiScrollable("new UiSelector().descriptionContains(\"" + text + "\")") 
                + uiScrollable("new UiSelector().textContains(\"" + text + "\")");
        return findElementByAndroidUIAutomator(uiScrollables);
    }

    @Override public T scrollToExact(String text) {
        String uiScrollables =
            uiScrollable("new UiSelector().description(\"" + text + "\")") + uiScrollable(
                "new UiSelector().text(\"" + text + "\")");
        return findElementByAndroidUIAutomator(uiScrollables);
    }

    public T scrollToUsingAutomamator(ByAndroidUIAutomator by) {
        return findElementByAndroidUIAutomator(uiScrollable(by.getAutomatorText()));
    }   
    ....
}
public class IOSDriver<T extends WebElement>
    extends AppiumDriver<T>
    implements IOSDeviceActionShortcuts, GetsNamedTextField<T>,
    FindsByIosUIAutomation<T> {
    ....
    @SuppressWarnings("unchecked")
    @Override
    public T scrollTo(String text) {
        return (T) ((ScrollsTo<?>) findElementByClassName("UIATableView"))
            .scrollTo(text);
    }

    @SuppressWarnings("unchecked")
    @Override
    public T scrollToExact(String text) {
        return (T) ((ScrollsTo<?>) findElementByClassName("UIATableView"))
            .scrollToExact(text);
    }

   public T scrollToUsingPredicate(By scrollableElementBy, String iOSpredicate) {
        return (T) findElementBy(scrollableElementBy)
            .scrollToUsingPredicate(iOSpredicate);
   }

   public T scrollToUsingPredicate(String iOSpredicate) {
        return scrollToUsingPredicate(By.className("UIATableView"), iOSpredicate);
   }
    ....
}
  • An additional method is supposed to be added to IOSElement:
public class IOSElement extends MobileElement
    implements FindsByIosUIAutomation<MobileElement>, ScrollsTo<MobileElement> {
    ....
    public MobileElement scrollToUsingPredicate(String iOSpredicate) {
        return (IOSElement) findElementByIosUIAutomation(
            ".scrollToElementWithPredicate(\"" + iOSpredicate + ""\")");
    }

    @Override public MobileElement scrollTo(String text) {
        return scrollToUsingPredicate("name CONTAINS '" + text + "'");
    }

    @Override public MobileElement scrollToExact(String text) {
        return scrollToUsingPredicate("name == '" + text + "'");
    }
    ....
}

@TikhomirovSergey

Copy link
Copy Markdown
Contributor

@SrinivasanTarget what do you think about this solution?

@bootstraponline @Jonahss you can join to this discussion if you want.
@autoaim800 @nuggit32 you are invited too.

#341

@SrinivasanTarget

SrinivasanTarget commented Apr 26, 2016

Copy link
Copy Markdown
Member Author

@TikhomirovSergey Apologies for the delay.

Above suggested Android changes looks fine for me,

Have few queries w.r.t iOS,

  1. Why are we not gonna refractor existing ScrollTo method? It works fine only for "UITableView" but not for other scrollable views e.g "UIScrollView"
  2. I see above suggested iOS changes also uses "UITableView" hard coded, this serves the same way as existing ScrollTo i.e may work only for UITableView.
public T scrollToUsingPredicate(By scrollableElementBy, String iOSpredicate) {
        return (T) findElementBy(scrollableElementBy)
            .scrollToUsingPredicate(iOSpredicate);
   }

   public T scrollToUsingPredicate(String iOSpredicate) {
        return scrollToUsingPredicate(By.className("UIATableView"), iOSpredicate);
   }

I think we need to use ByIosUIAutomation instead of By. Still not clear with this portion.Please clarify.

@autoaim800

autoaim800 commented Apr 27, 2016

Copy link
Copy Markdown

Hi SrinivasanTarget and TikhomirovSergey (in alpha order),
Thanks for inviting me.

I'm just thinking aloud:

#341 supposed to be fixed by #342, and #342 is supposed to be fixed by #366

For #341 the issue is scrollTo() and/or scrollToExact() can not be stopped at first occurrence at first glance, what's the root cause?
I thought it's not fix-able, as on Android, ListView has to scroll top->bottom to build a complete list of cells - as cells are not loaded at first/once (not like iOS), that's why only on its way back bottom->top, it stops at the first occurrence; correct me if I'm mistaken - I only have observation, no evidence at all to back up myself.

For #342, the solution seems reasonable from my perspective.

For #366, it's more like a new feature - just the methods have different names on two platforms, not ideal.
scrollToUsingPredicate()
scrollToUsingAutomamator()

-Bill

@TikhomirovSergey

Copy link
Copy Markdown
Contributor

#197

@TikhomirovSergey TikhomirovSergey added this to the 4.0.0 milestone Apr 28, 2016
@SrinivasanTarget

Copy link
Copy Markdown
Member Author

#226

@SrinivasanTarget

Copy link
Copy Markdown
Member Author

@autoaim800 Thanks for your comments. Your observations are correct and the above tagged/discussed issues will be addressed.

@TikhomirovSergey Have updated another possible iOS solution that works for all scrollable views now.Please have a look.Waiting for your comments:)

@SrinivasanTarget

Copy link
Copy Markdown
Member Author

@TikhomirovSergey

Second Solution:
Have altered your solution here below, please check which one would be good to have,

*MobileBy will be re-fractored as you said above
*AndroidDriver will have below changes,

    static String uiScrollablenew(String uiSelector) {
        return "new UiScrollable(new UiSelector().scrollable(true).instance(0))"
            + ".scrollTextIntoView(\"" + uiSelector + "\");";
    }

    public T scrollToUsingAutomator(ByAndroidUIAutomator by) {
        return findElementByAndroidUIAutomator(uiScrollablenew(by.getAutomatorText()));
    }

*IOSDriver will have below changes,

    static String scrollToVisible(ByIosUIAutomation scrollableElementBy) {
        return  scrollableElementBy.getAutomatorText() + ".scrollToVisible();";
    }

    public T scrollToUsingAutomator(ByIosUIAutomation scrollableElementBy) {
        return (T) findElementByIosUIAutomation(scrollToVisible(scrollableElementBy));
    }

Your thoughts on solution comitted and the second one?

@SrinivasanTarget

Copy link
Copy Markdown
Member Author

appium/appium#2942

@SrinivasanTarget

Copy link
Copy Markdown
Member Author

appium/appium#4633

TikhomirovSergey and others added 3 commits May 2, 2016 21:15
# The first commit's message is:
Merge pull request #378 from TikhomirovSergey/rafael-chavez-xcuitesting

addition to the PR #352

Alternative solution for scrollTo

# This is the 2nd commit message:

App path correction in Tests

# This is the 3rd commit message:

removed scrollToUsingAutomator

# This is the 4th commit message:

cleanup
@TikhomirovSergey

Copy link
Copy Markdown
Contributor

I'm closing it according to #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants