#565 FIX by TikhomirovSergey · Pull Request #646 · appium/java-client

TikhomirovSergey · May 21, 2017

Change list

#565 FIX. The new feature implementation.

Types of changes

No changes in production code.
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

There annotations were re-designed (some annotations were added)

AndroidFindAll
AndroidFindBys
iOSFindAll
iOSFindBys
iOSXCUITFindAll
iOSXCUITFindBys
WindowsFindAll
WindowsFindBys

All these annotations are repeateble. The use case is described here.

but it needs to be covered with tests

- bug fixes - test coverage

TikhomirovSergey · May 21, 2017

@SrinivasanTarget @amedvedjev could you take a look at the PR?

amedvedjev · May 21, 2017

@HowToUseLocators(androidAutomation = CHAIN, iOSAutomation = ALL_POSSIBLE)
@FINDALL{@findby(someStrategy1), @findby(someStrategy2)})

last ")" ?

same:
@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
@FINDALL{@findby(someStrategy1), @findby(someStrategy2)})

otherwise nice

TikhomirovSergey · May 22, 2017

@amedvedjev Are you talking about the WIKI page?

amedvedjev · May 22, 2017

@TikhomirovSergey yes. on wiki.

Also documentation was improved

TikhomirovSergey · May 26, 2017

@amedvedjev @SrinivasanTarget
This PR was updated. Could you take a look at this one more time?

@SrinivasanTarget could you please approve it if everything is ok?

SrinivasanTarget

Few Comments in readme. Overall cool stuff. 👍

SrinivasanTarget · May 29, 2017

docs/Page-objects.md

+@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
+@iOSFindBy(someStrategy1)
+@iOSFindAll(value = {@iOSBy(subloctor1), @iOSBy(subloctor1)}, priority = 1) //this possible variant is 
+// the chain


Can we update this section as below please?

@HowToUseLocators(iOSAutomation = ALL_POSSIBLE) @iOSFindBy(someStrategy1) @iOSFindBys({ @iOSBy(parentLocator), @iOSBy(childLocator)}, priority = 1) //this possible variant is the chain @iOSFindBy(someStrategy2, priority = 2) @iOSFindBy(someStrategy3, priority = 3) RemoteWebElement someElement;

ALL_POSSIBLE contains CHAIN

SrinivasanTarget · May 29, 2017

docs/Page-objects.md

+@iOSFindBy(someStrategy2, priority = 2) 
+@iOSFindBy(someStrategy3, priority = 3) 
+RemoteWebElement someElement;
+


CHAINING Contains ALL_POSSIBLE. Awesome👍

SrinivasanTarget · May 29, 2017

docs/Page-objects.md

+@HowToUseLocators(iOSAutomation = ALL_POSSIBLE)
+@iOSFindBy(someStrategy1)
+@iOSFindAll(value = {@iOSBy(subloctor1), @iOSBy(subloctor1)}, priority = 1) //this possible variant is 
+// the chain


Same here too.

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            if (selendroidFindByArray != null && selendroidFindByArray.length == 1) {
-                return createBy(new Annotation[] {selendroidFindByArray[0]}, HowToUseSelectors.USE_ONE);
-            }
+        List<Annotation> annotations =  new ArrayList<>(asList(annotatedElement.getAnnotationsByType(singleLocator)));


nit: extra space here

Ok. I am working on the improving.

:) The code is very well written. Although things like this bother my internal perfectionist.

@mykola-mokhnach I have noticed that space. Ok. Thak you.

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            if (selendroidFindBys != null) {
-                return createBy(selendroidFindBys.value(), HowToUseSelectors.BUILD_CHAINED);
-            }
+        Annotation[] annotationsArray = annotations.toArray(new Annotation[]{});


is there any particular reason to transform it back to array? lists can also be sorted without any problems

@mykola-mokhnach Good catch. This is something like "legacy". I supposed to use array at first revisions.

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            }
+        Annotation[] annotationsArray = annotations.toArray(new Annotation[]{});
+        sort(annotationsArray, comparator);
+        By[] result = new By[] {};


same here - why not to use list and only convert it to array on return?

@mykola-mokhnach This is not the same. Array is returned by java by design. Comporator can work with collection and array so why we have to covert it to a list?

result = add(result, createBy(new Annotation[] {a}, HowToUseSelectors.USE_ONE));

I suppose this will create a new copy of the array on each call and this is not the case for a list where we can add elements to the same instance.

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            if (androidFindByArray != null && androidFindByArray.length == 1) {
-                return createBy(new Annotation[] {androidFindByArray[0]}, HowToUseSelectors.USE_ONE);
+            Method value;
+            Annotation[] set;


can this variable have some more descriptive name?

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            }
+        By result = null;
+        if (isSelendroidAutomation()) {
+            result =  buildMobileBy(howToUseLocators != null ? howToUseLocators.selendroidAutomation() : null,


I'm always a bit suspicious about using method calls with ternary operator, since Java will invoke the method even if the precondition equals to false

same question to all the further ternary operator usages in such context (with method invocation instead of constant values)

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

-            if (iOSFindBys != null) {
-                return createBy(iOSFindBys.value(), HowToUseSelectors.BUILD_CHAINED);
-            }
+        if (isAndroid()  && result == null) {


exchanging operands

result == null && iisAndroid()

could make it faster

in general one can simplify it by adding

if (result != null) { return result; }

before all these conditions. Then it won't be necessary to check whether it is equal to null in all the further conditions

@mykola-mokhnach There are some difficulties.
It is by design. When selendroind-mode-specific annotations are not defined there when it is possible to use '@AndroidFindBy-s' at some cases. The same is true for iOSXCUITFindBy-s/iOSFindBy-s

...however I know how to make to look it better.

Do you want to say that isAndroid() method (and others) may implicitly change the value of result?

mykola-mokhnach · May 29, 2017

src/main/java/io/appium/java_client/pagefactory/DefaultElementByBuilder.java

+            int p1 = getPriorityValue(priority1, o1, c1);
+            int p2 = getPriorityValue(priority2, o2, c2);
+
+            if (p2 > p1) {


the whole condition can be replaced with

return Integer.signum(p1 - p2);

TikhomirovSergey · May 29, 2017

@mykola-mokhnach @SrinivasanTarget
Required changes have been done. Could you rewiew the PR one more time?

TikhomirovSergey added 7 commits May 7, 2017 00:21

#565: initial commit

02a1852

#565: Repeatable complex locators

d447014

#565: Annotation comparator.

6c85323

#565: The feature was implemented

29336b8

but it needs to be covered with tests

#565: The feature was completed

69ab315

- bug fixes - test coverage

#565: Code style issues were fixed.

8472b4f

#565: the WIKI was updated

8fc9a82

TikhomirovSergey requested a review from SrinivasanTarget May 21, 2017 20:43

TikhomirovSergey self-assigned this May 21, 2017

TikhomirovSergey added documentation enhancement refactor labels May 21, 2017

TikhomirovSergey added this to the 5.0.0 milestone May 22, 2017

#565: Issues which were found by codecy were fixed

5a51da0

Also documentation was improved

SrinivasanTarget requested changes May 29, 2017

View reviewed changes

mykola-mokhnach reviewed May 29, 2017

View reviewed changes

#565: The work on remarks

a736259

mykola-mokhnach approved these changes May 30, 2017

View reviewed changes

SrinivasanTarget approved these changes May 30, 2017

View reviewed changes

TikhomirovSergey merged commit 5505387 into appium:master May 30, 2017

TikhomirovSergey mentioned this pull request May 30, 2017

Feature: To support mix of search strategy #565

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

TikhomirovSergey commented May 21, 2017

Change list

Types of changes

Details

Uh oh!

TikhomirovSergey commented May 21, 2017

Uh oh!

amedvedjev commented May 21, 2017

Uh oh!

TikhomirovSergey commented May 22, 2017

Uh oh!

amedvedjev commented May 22, 2017

Uh oh!

TikhomirovSergey commented May 26, 2017

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

mykola-mokhnach May 29, 2017 •

edited

Loading

TikhomirovSergey May 29, 2017 •

edited

Loading

mykola-mokhnach May 29, 2017 •

edited

Loading

mykola-mokhnach May 29, 2017 •

edited

Loading

TikhomirovSergey commented May 29, 2017 •

edited

Loading