Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to calculate screenshots overlap score + several helpers #595

Merged
merged 1 commit into from
Apr 10, 2017

Conversation

mykola-mokhnach
Copy link
Contributor

Change list

This feature has been asked by several users, so I've just nicely decorated the code we use in our own automation framework and added it to the client.

Types of changes

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • [ x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Screenshots comparison algorithm itself is based on standard opencv calls. That is why the library has been added to Gradle config as a dependency.

@jsf-clabot
Copy link

jsf-clabot commented Mar 10, 2017

CLA assistant check
All committers have signed the CLA.

@mykola-mokhnach
Copy link
Contributor Author

@TikhomirovSergey is checkstyle configured properly here?

The tool does not like how one decorates loops with post-condition:

        do {
....
        } while (System.currentTimeMillis() - started <= timeoutMs);

The output is

[ant:checkstyle] [ERROR] /home/travis/build/appium/java-client/src/main/java/io/appium/java_client/ScreenshotState.java:165:9: '}' at column 9 should be alone on a line. [RightCurly]

Somebody should be wrong here...

@SrinivasanTarget
Copy link
Member

The tool does not like how one decorates loops with post-condition:

We follow Google's standards :)

@SrinivasanTarget
Copy link
Member

@mykola-mokhnach Can you rebase this branch as well with master to make commits clean?

@mykola-mokhnach
Copy link
Contributor Author

@SrinivasanTarget squashing done. Waiting for review comments.

@mykola-mokhnach
Copy link
Contributor Author

@SrinivasanTarget @saikrishna321 could you please find some time to finish the review?

@SrinivasanTarget
Copy link
Member

SrinivasanTarget commented Mar 17, 2017 via email

@TikhomirovSergey
Copy link
Contributor

@mykola-mokhnach I remember about this PR. It is the next for the revieving.

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey left a comment

Choose a reason for hiding this comment

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

@mykola-mokhnach
Hi. Sorry for the delaying.

There are some issues.
I can't run it from IDE (IntellijIDEA 2017.1) nor via command line.

IDEA

Command line

What is wrong?

@TikhomirovSergey
Copy link
Contributor

@mykola-mokhnach
Also there is the question. I think the scope of this feature is more general than mobile automation. Why is it necessary to include it to appium java client?

@mykola-mokhnach
Copy link
Contributor Author

Hi @TikhomirovSergey
It was failing because of inconsistent opencv lib import. This was one of cases when it "works for me". Now the problem should be fixed and all the unit tests were double checked as well.

@mykola-mokhnach
Copy link
Contributor Author

Regarding why it is necessary for java lib - several users were asking for it. And we also have similar implementation in our own automation framework. So I just wanted to share verified solution and give people possibility to solve more problems with Appium (since the java client is the most popular one).

@TikhomirovSergey
Copy link
Contributor

@mykola-mokhnach
ok
Yeah. All tests are green now.

@TikhomirovSergey TikhomirovSergey merged commit 96305fe into appium:master Apr 10, 2017
@mykola-mokhnach mykola-mokhnach deleted the screenshot_state branch June 26, 2017 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants