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.

Sign up for GitHub

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

Jump to bottom

Fix bug in Rect::contains, clarify docs, and add doctests for it. #635

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Fix bug in Rect::contains, clarify docs, and add doctests for it. #635

merged 1 commit into from
Apr 21, 2017

Conversation

mdsteele
Copy link
Contributor

This is to fix issue #569.

@mdsteele
Copy link
Contributor Author

Hmm, looks like the doctest for a 0-by-0 rect is failing, because apparently, clamp_size doesn't allow a rect to have a width or height of zero, which is somewhat surprising behavior (and doesn't seem to be documented?). I'll fix the doctest for now.

@Cobrand
Copy link
Member

Cobrand commented Apr 17, 2017

Before merging this I'd like to know what will be affected by this, and if this effectively counts as a bugfix or if this is only a sneaky breaking change. Fixing this bug is fine, but if we end up with other bugs because they thought this version of contains was the intended behavior but isn't anymore, without their knowledge as well, well it's definitely a problem. Unless I'm 90% sure that this change won't break but only fix existing code, I won't merge this and will only accept a rename.

Makes me think about the famous "YOU DO NOT BREAK USERSPACE" from Torvalds. There was the same kind of unintended behavior with memcpy and copy to overlapped memory. After they fixed that, flash users experienced problems. They had two choices: let the developers fix their usage of memcpy, or not make such a breaking change sneakily.

We're in the exact same case, and just because we know that we won't be affected by this change because we know about it, doesn't mean everyone's like that. I'd prefer if this function was deprecated, and we used a rename as the "correct" way of doing things.

@mdsteele
Copy link
Contributor Author

Well...it seems like a bug to me (given that the current behavior differs from SDL_PointInRect in the underlying library, and moreover leads to nonsensical results like having two rects for which has_intersection returns false even though there exists a point that both rects "contain"). But of course, I can't be sure that there aren't projects relying on this behavior, so I'm up for going the deprecate-and-rename route.

So I propose marking contains deprecated, and adding a new method contains_point that matches the behavior of SDL_PointInRect. As a bonus, this rename gives us an easy opportunity to also add a contains_rect method (which I've found myself wanting before) without ambiguity.

Update on the way.

Cobrand
Cobrand reviewed Apr 18, 2017
View reviewed changes
src/sdl2/rect.rs
/// assert!(rect.contains(Point::new(4, 6))); // N.B.
/// assert!(!rect.contains(Point::new(5, 7)));
/// ```
#[deprecated(since = "0.30.0", note = "use `contains_point` instead")]
Copy link
Member

Choose a reason for hiding this comment

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

Wait ... does this work ? I thought they could only be used internally by the compiler ? Which release of Rust added the deprecated attribute ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know which commit or release added this but it definitely works with non-compiler internals.

I've used it in quite a few projects.

Cobrand
Cobrand requested changes Apr 18, 2017
View reviewed changes
Copy link
Member

@Cobrand Cobrand left a comment

Choose a reason for hiding this comment

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

Great changes ! hanks for the doc tests, it definitely helps ! Just a little change about linking the issue, squash all of that in a single commit and we'll be good to go !

src/sdl2/rect.rs Outdated
/// [`SDL_PointInRect`](https://wiki.libsdl.org/SDL_PointInRect) by
/// including points along the bottom and right edges of the rectangle, so
/// that a 1-by-1 rectangle actually covers an area of four points, not
/// one.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please link to issue #569 here ? The notice here is great but it would be even better you there was a link to the original issue / discussion so curious users can see what it is about exactly 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Cobrand
Copy link
Member

Cobrand commented Apr 21, 2017

Great, thanks ! Merged.

@Cobrand Cobrand merged commit c0d6e0e into Rust-SDL2:master Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phrohdoh phrohdoh phrohdoh left review comments

@Cobrand Cobrand Cobrand requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdsteele @Cobrand @phrohdoh