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

Fix Fill options for non Fixed width options #2018

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 46 additions & 20 deletions druid/src/widget/image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,20 +197,44 @@ impl<T: Data> Widget<T> for Image {
) -> Size {
bc.debug_check("Image");

// If either the width or height is constrained calculate a value so that the image fits
// in the size exactly. If it is unconstrained by both width and height take the size of
// the image.
// Let the fill strat determine the size of the widget as a function of the bc.max()
// and the original image size. But catch cases were they would return INFINITY
// and try to make a sensible area as a function of the fill strat.
let max = bc.max();
let image_size = self.image_size();
let size = if bc.is_width_bounded() && !bc.is_height_bounded() {
let ratio = max.width / image_size.width;
Size::new(max.width, ratio * image_size.height)
} else if bc.is_height_bounded() && !bc.is_width_bounded() {
let ratio = max.height / image_size.height;
Size::new(ratio * image_size.width, max.height)
} else {
bc.constrain(image_size)
};
let mut max_width = bc.max().width;
let mut max_height = bc.max().height;
let mut fill_strat = self.fill;
if max_width == f64::INFINITY && max_height == f64::INFINITY {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use bc.is_xxx_bounded.

Copy link
Collaborator Author

@fishrockz fishrockz Nov 3, 2021

Choose a reason for hiding this comment

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

These detect times when it would note be safe to use the max so they work quite differently. ah yes

// If we are in a scroll box for width and height then fall back to
// original image size
max_height = image_size.height;
max_width = image_size.width;
} else if max_width == f64::INFINITY || max_height == f64::INFINITY {
// If we are in a scroll box for width or heigh but not both
// then give self.fill a box the size of the scroll box and let it decide how to
// fill it. Unless that would result in requesting INFINITY in which case fall back
// to contain.
fill_strat = match fill_strat {
FillStrat::FitWidth if max_width == f64::INFINITY => FillStrat::Contain,
FillStrat::FitHeight if max_height == f64::INFINITY => FillStrat::Contain,
FillStrat::Cover | FillStrat::Fill => FillStrat::Contain,
_ => fill_strat,
}
Comment on lines +218 to +223
Copy link
Contributor

Choose a reason for hiding this comment

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

This effectively the same code as I had, but much more convoluted. It is more clear to use the already existing code in this case (and the if statement on line 200-205 as well). Maybe it would be nice to have a match on (bool,bool) for the boundness of each axis and have clean fast code for each case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't believe this does behave very much like your code out side of the options that fall back to Contain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes I see I missed 2 cases.

}
let affine = fill_strat
.affine_to_fill(Size::new(max_width, max_height), image_size)
.as_coeffs();
// The first and forth elements of the affine are the x and y scale factor.
// So just multiply them by the original size to get the ideal area based on `self.fill`.
let mut width = affine[0] * image_size.width;
let mut height = affine[3] * image_size.height;
// We are using the image scale properties so if one dimension
// would be over scaled to keep AR fixed then we just clip back to the `bc.max()`
width = width.min(max.width);
height = height.min(max.height);

let size = Size::new(width, height);
trace!("Computed size: {}", size);
size
}
Expand Down Expand Up @@ -367,20 +391,18 @@ mod tests {

// A middle row of 600 pixels is 100 padding 200 black, 200 white and then 100 padding.
let expecting: Vec<u8> = [
vec![41, 41, 41, 255].repeat(100),
vec![255, 255, 255, 255].repeat(200),
vec![0, 0, 0, 255].repeat(200),
vec![41, 41, 41, 255].repeat(100),
vec![41, 41, 41, 255].repeat(200),
]
.concat();
assert_eq!(raw_pixels[199 * 600 * 4..200 * 600 * 4], expecting[..]);

// The final row of 600 pixels is 100 padding 200 black, 200 white and then 100 padding.
let expecting: Vec<u8> = [
vec![41, 41, 41, 255].repeat(100),
vec![0, 0, 0, 255].repeat(200),
vec![255, 255, 255, 255].repeat(200),
vec![41, 41, 41, 255].repeat(100),
vec![41, 41, 41, 255].repeat(200),
]
.concat();
assert_eq!(raw_pixels[399 * 600 * 4..400 * 600 * 4], expecting[..]);
Expand Down Expand Up @@ -438,8 +460,10 @@ mod tests {
2,
);

let image_widget =
Scroll::new(Container::new(Image::new(image_data)).with_id(id_1)).vertical();
let image_widget = Scroll::new(
Container::new(Image::new(image_data).fill_mode(FillStrat::Fill)).with_id(id_1),
)
.vertical();

Harness::create_simple(true, image_widget, |harness| {
harness.send_initial_events();
Expand All @@ -466,8 +490,10 @@ mod tests {
2,
);

let image_widget =
Scroll::new(Container::new(Image::new(image_data)).with_id(id_1)).horizontal();
let image_widget = Scroll::new(
Container::new(Image::new(image_data).fill_mode(FillStrat::Fill)).with_id(id_1),
)
.horizontal();

Harness::create_simple(true, image_widget, |harness| {
harness.send_initial_events();
Expand Down