-
Notifications
You must be signed in to change notification settings - Fork 39
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
Implement diff. percentage #480
Conversation
Also, I would love to include this contribution as part of my bachelor's thesis about screenshot testing. |
@@ -84,17 +85,23 @@ fun processOutputImageAndReport( | |||
bufferedImageType = recordOptions.pixelBitConfig.toBufferedImageType() | |||
) | |||
} | |||
|
|||
// Only used by CaptureResult.Changed | |||
var diffPercentage by Delegates.notNull<Float>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to know this: the reason you use Delegates.notNull<Float>()
instead of var diffPercentage = NaN
is that you want to trigger an error when the value is not set, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it is a little more robust to force the developer to set the diffPercentage
in each case separately than to set a default and only change it in the cases where it should be different. But it is mostly a matter of taste, in my opinion. I can change it if you wish.
Also, I did not log anything, because I wanted to know from you if you want the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for being late. Thank you for your contribution.
No worries. Thank you as well! |
This PR is my proposed solution for #460.
Please take a look at the way I implemented the property
diffPercentage
in theCaptureResult.Changed
data class. Another possibility would be to declarediffPercentage
directly as a property in theCaptureResult
interface, and implement them as null-returning-getters in the other data classes, as you have done for other properties. (This would also remove the need for my if-statement inCaptureResultTest.kt
.)Also, as far as I can tell, you plan to use the Dropbox Differ in the future for iOS as well (see this comment). That's why I have implemented the
diffPercentage
to always equalFloat.NaN
(undefined) for iOS (the reason being that your diffing algorithm for iOS would require a lot of changes to be able to deliver the needed data for calculating a diff. percentage).I hope you like this contribution :)
Don't hesitate to tell me if I need to change something.