Skip to content

Fixed: 'canvas difference area' is not included in mask.#6

Merged
subiron merged 7 commits intowttech:masterfrom
subiron:master
Oct 20, 2016
Merged

Fixed: 'canvas difference area' is not included in mask.#6
subiron merged 7 commits intowttech:masterfrom
subiron:master

Conversation

@subiron
Copy link
Copy Markdown
Contributor

@subiron subiron commented Sep 27, 2016

Fixed: 'canvas difference area' is not included in mask.
Improve mask feature by marking canvas-only difference as yellow.
Include canvas diff in differenceCounter.
Updated unit test for fix and new mask feature.

Improve mask feature by marking canvas-only difference as yellow.
Include canvas diff in differenceCounter.
Updated unit test for fix and new mask feature.
@@ -48,6 +46,8 @@ public final class ImageComparison {

private static final int INVALID_PIXEL_COLOR = 125 << ALPHA_SHIFT | 255 << RED_SHIFT | 0 << GREEN_SHIFT | 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we have somethiing like this here?:

int rgba = new java.awt.Color(r,g,b,a).getRGB();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

+1
done

Copy link
Copy Markdown
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Very good idea! I've added some cosmetics comments.


// Sets not covered areas as error (red)
fastMarkOuterAreaAsError(minWidth, minHeight, widthDifference, heightDifference,
differenceCounter += fastMarkOuterAreaAsError(minWidth, minHeight, widthDifference, heightDifference,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering - but it is probably a good idea to count outer area as errors.
Please update comment above in line 82.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i was thinking about saving "canvas-diff" value in separate field but
what's the reason ?
As far as I know we even don't show differenceCounter in report ;]

}

@Test
public void testCompare_different_canvas() throws Exception {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please rename this test to meet the convention e.g. to:
compare_differentHeightScreenshots_expectHeightDifferenceMarkedWithYellow .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

renamed

writableRaster.setDataElements(minWidth, 0, widthDifference, resultHeight, emptyAreaA);

// fil area [0, minHeight, minWidth, resultHeight]
// fil bottom area
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fil bottom area
->
fill bottom area

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed... but,are you familiar with "Allow edits from maintainers." feture in github?
It's not enabled by default for no reason ;)

Copy link
Copy Markdown
Contributor

@wiiitek wiiitek Sep 29, 2016

Choose a reason for hiding this comment

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

Thanks @subiron! I didn't know about this!

@malaskowski
Copy link
Copy Markdown
Contributor

malaskowski commented Oct 3, 2016

Please update documentation.

@subiron subiron merged commit 89d0251 into wttech:master Oct 20, 2016
@subiron subiron added the bug label Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants