Cleaning and extract image comparison #55
Conversation
There was a problem hiding this comment.
Hi @BielinskiLukasz ,
Thank you for your contribution! Great work, never got around refactoring myself:)
Please find some comments on src/main/java/com/assertthat/selenium_shutterbug/core/PageSnapshot.java
and
src/main/java/com/assertthat/selenium_shutterbug/core/Snapshot.java
The rest looks great.
Regards,
Glib
| @@ -51,7 +51,7 @@ public PageSnapshot highlight(WebElement element) { | |||
| */ | |||
| public PageSnapshot highlight(WebElement element, Color color, int lineWidth) { | |||
| try { | |||
| image = ImageProcessor.highlight(image, new Coordinates(element, devicePixelRatio), color, lineWidth); | |||
| ImageProcessor.highlight(image, new Coordinates(element, devicePixelRatio), color, lineWidth); | |||
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
| @@ -93,7 +93,7 @@ public PageSnapshot highlightWithText(WebElement element, Color elementColor, in | |||
| try { | |||
| highlight(element, elementColor, 0); | |||
| Coordinates coords = new Coordinates(element, devicePixelRatio); | |||
| image = ImageProcessor.addText(image, coords.getX(), coords.getY() - textFont.getSize() / 2, text, textColor, textFont); | |||
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
| @@ -176,7 +185,7 @@ public T withThumbnail(double scale) { | |||
| * @return instance of type Snapshot | |||
| */ | |||
| public T monochrome() { | |||
| this.image = ImageProcessor.convertToGrayAndWhite(this.image); | |||
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
|
Thanks for quick response and code review. What about @6de649e commit? I didn't test other combination but there is assignment scrollBarMaxWidth to _viewportHeight and next again scrollBarMaxWidth to _viewportWidth. I am wondering about the multiple uses of width. If it's correct, there is TODO what I created, should be removed :) |
|
Hi @BielinskiLukasz , Regarding @6de649e, I think we should leave this as is. scrollBarMaxWidth is standard so it is used for both - removing vertical and horizontal scrollbars if present. |
Hello :)
I work with your code this week, great job. I made some cleaning there (removing doubles etc.). I extracted image comparison methods from ImageProcessor to a new class.
commit 6de649e
I'm not sure that the line is correct:
_viewportHeight -= scrollBarMaxWidth;