scap icon indicating copy to clipboard operation
scap copied to clipboard

Move is_supported and has_permission into Capturer::build

Open valenotary opened this issue 1 year ago • 2 comments

This is just a matter of idiomatic preference. I think that (from my minimal usage of this lovely library so far) creating a Capturer fundamentally relies on is_supported and has_permission succeeding, and there should be no point in using a Capturer if either of them are false. Therefore to reduce client boilerplaiting I think you should change the Capturer::new method to a Capturer::build method and internally call those two booleans in there instead. In that way the build method can now return a Result<Capturer, Err>, and the API/the user can handle the errors as they please (example, when has_permission fails, the build method can now request permission, otherwise we expose the error to see if the user wants to do so as well).

valenotary avatar Jul 29 '24 22:07 valenotary

nice idea!

I think the reason why is_supported / has_permission is a thing is so that they can be used in other areas of the codebase to determine whether or not screen recording is supported

so maybe keeping these still public, but also changing to Capturer::build, probably makes sense

richiemcilroy avatar Aug 05 '24 15:08 richiemcilroy

Yes @richiemcilroy . The reason they're publicly exposed is so that they can be used in other areas of the codebase. It also provides the library consumers a simple way to check if screen capture would succeed and handle errors the way they want to.

Thanks for the feedback @valenotary ! It does make sense to call the is_supported and has_permission internally inside Capturer::build.

Let's do it the way @richiemcilroy suggests. I'll work on this whenever I get time sometime this week. In the meantime, this is up for grabs if anyone wants to give it a shot :)

Pranav2612000 avatar Aug 06 '24 16:08 Pranav2612000