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

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

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

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

last ")" ?

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

otherwise nice

Also documentation was improved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few Comments in readme. Overall cool stuff. 👍

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHAINING Contains ALL_POSSIBLE. Awesome👍

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here too.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I am working on the improving.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this variable have some more descriptive name?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exchanging operands

result == null && iisAndroid()

could make it faster

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		int p1 = getPriorityValue(priority1, o1, c1);
		int p2 = getPriorityValue(priority2, o2, c2);

		if (p2 > p1) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole condition can be replaced with

return Integer.signum(p1 - p2);