Skip to content

Add GenericImageView::try_view #2485

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

imlazyeye
Copy link

This adds GenericImageView::try_view as a non-panicking alternative to GenericImageView::view. It additionally updates the documentation of GenericImageView::view to mention its panics.

I am writing a non-panicking binary that captures screenshots of a window. If the user resizes their window unexpectedly, the coordinates/dimensions I pass in to GenericImageView::view may become invalid. I would like to be able to get an Err so I can handle this behavior myself.

I am unsure if the error I utilized here is correct within this context -- please let me know if there is another I should be using/if I should create a new one!

@imlazyeye imlazyeye changed the title Try view Adds GenericImageView::try_view Jun 12, 2025
@imlazyeye imlazyeye changed the title Adds GenericImageView::try_view Add GenericImageView::try_view Jun 12, 2025
Copy link
Member

@197g 197g left a comment

Choose a reason for hiding this comment

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

The error type is fine with me, another variant would be returning Option but imo try_ is a better pattern as we have an error here. The error itself is consistent with other choices.

@fintelia
Copy link
Contributor

I think Option is generally preferable for try_ methods that have only one failure mode.

We should certainly add the "panics" section to the docs on the view method, but I'm not convinced on adding try_view. It feels very specialized and reasonably straightforward to work around in user code.

@197g
Copy link
Member

197g commented Jun 16, 2025

The responsibility of the implementation of the check seems to me like it should belong in our code. Similar to the standard library having added checked variants of split even though for a long time it had the argument that its precondition would be simple to check. And mind you, in comparison this precondition is harder to get right due to overflows.

@imlazyeye
Copy link
Author

Agreed, I think there's precedent for crates guiding users through what failure states could occur through the return of their types, not just the documentation. It's still the user's responsibility to handle this error case, but I think the crate is responsible for guiding the user through what errors they will potentially run into.

I think Option is generally preferable for try_ methods that have only one failure mode.

Typically I follow the same philosophy, however in cases where your error case is the result of a mistake rather than "there is not something there", Result feels more appropriate. I think there's an argument both ways for where this particular example lies, but personally Result seems most natural.

@imlazyeye
Copy link
Author

redid above work after rebasing to main's refactor

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.

3 participants