Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Merged
TikhomirovSergey merged 1 commit intoappium:masterappium/java-client:masterfrom
mykola-mokhnach:screenshot_statemykola-mokhnach/java-client:screenshot_stateCopy head branch name to clipboard
Apr 10, 2017
Merged

Add possibility to calculate screenshots overlap score + several helpers#595
TikhomirovSergey merged 1 commit intoappium:masterappium/java-client:masterfrom
mykola-mokhnach:screenshot_statemykola-mokhnach/java-client:screenshot_stateCopy head branch name to clipboard

Conversation

@mykola-mokhnach
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach commented Mar 9, 2017

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 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Mar 10, 2017

@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

SrinivasanTarget commented Mar 11, 2017

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

We follow Google's standards :)

@SrinivasanTarget
Copy link
Member

SrinivasanTarget commented Mar 11, 2017

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

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Mar 11, 2017

@SrinivasanTarget squashing done. Waiting for review comments.

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Mar 17, 2017

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

@SrinivasanTarget
Copy link
Member

SrinivasanTarget commented Mar 17, 2017 via email

@SrinivasanTarget SrinivasanTarget self-requested a review March 17, 2017 22:45
@SrinivasanTarget SrinivasanTarget added this to the 5.0.0 milestone Mar 17, 2017
@mykola-mokhnach mykola-mokhnach mentioned this pull request Apr 3, 2017
Merged
4 tasks
@TikhomirovSergey
Copy link
Contributor

TikhomirovSergey commented Apr 3, 2017

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

@TikhomirovSergey TikhomirovSergey self-requested a review April 3, 2017 20:49
TikhomirovSergey
TikhomirovSergey suggested changes Apr 8, 2017
View reviewed changes
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

TikhomirovSergey commented Apr 8, 2017

@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

mykola-mokhnach commented Apr 9, 2017

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

mykola-mokhnach commented Apr 9, 2017

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

TikhomirovSergey commented Apr 10, 2017

@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

Reviewers

@SrinivasanTarget SrinivasanTarget Awaiting requested review from SrinivasanTarget SrinivasanTarget is a code owner

1 more reviewer

@TikhomirovSergey TikhomirovSergey TikhomirovSergey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mykola-mokhnach @jsf-clabot @SrinivasanTarget @TikhomirovSergey
Morty Proxy This is a proxified and sanitized view of the page, visit original site.