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

Implement diff. percentage #480

Merged
merged 2 commits into from
Sep 21, 2024
Merged

Conversation

vladcudoidem
Copy link
Contributor

This PR is my proposed solution for #460.

Please take a look at the way I implemented the property diffPercentage in the CaptureResult.Changed data class. Another possibility would be to declare diffPercentage directly as a property in the CaptureResult 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 in CaptureResultTest.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 equal Float.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.

@vladcudoidem
Copy link
Contributor Author

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>()
Copy link
Owner

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?

Copy link
Contributor Author

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.

@vladcudoidem
Copy link
Contributor Author

Also, I did not log anything, because I wanted to know from you if you want the diffPercentage logged anywhere or not. I can add it if you want.

Copy link
Owner

@takahirom takahirom left a 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.

@takahirom takahirom merged commit fe9d08a into takahirom:main Sep 21, 2024
6 checks passed
@vladcudoidem
Copy link
Contributor Author

No worries. Thank you as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants