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

feat(#574): reimplementation of Image widget layout function #605

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(#574): reimplementation of Image widget layout function #605

wants to merge 4 commits into from

Conversation

failingprovince
Copy link
Contributor

@failingprovince failingprovince commented Sep 22, 2024 •
edited by DJMcNab
Loading

Reimplement calculation of image size in Image widget layout function.
Makes it match against self.fill before setting size.

Tests still missing.

Fixes #574

@failingprovince
feat(linebender#574): Image widget layout reimplementation
23855b2 
Reimplement calculation of image size in `Image`
widget `layout` function.
Makes it match against `self.fill` before setting
size.
@failingprovince failingprovince mentioned this pull request Sep 22, 2024
Open
@DJMcNab DJMcNab linked an issue Sep 23, 2024 that may be closed by this pull request
Masonry: Reason about Image widget layout implementation #574
Open
@DJMcNab
Copy link
Member

DJMcNab commented Sep 23, 2024 •
edited
Loading

I've edited your comment to include the text "fixes #574", as the version in the title doesn't count.

This looks like a good start, although it'll be easier to judge once there are tests.

@failingprovince
feat(linebender#574): add tests
172b810 
Create testing case for `layout` function.
@failingprovince
Copy link
Contributor Author

Ready for review.
Test looks good to me, but still I'm not sure if they are written correctly.

@failingprovince failingprovince marked this pull request as ready for review September 24, 2024 18:09
@failingprovince
Copy link
Contributor Author

There is still one point open: should FillStrat be renamed?
If yes, to what?

jaredoconnell
jaredoconnell approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

I can confirm that this PR does not introduce a regression in the default Contain case in the examples. And the tests look good.
Regarding renaming FillStrat, I'll defer to the maintainers. I don't love the current name, but it works. So I'm open to either renaming or keeping it.

@jaredoconnell
Copy link
Contributor

One comment I have is that there is code in the implementation of FillStrat that is not used with this code. What do we want to do about that?

@failingprovince
Copy link
Contributor Author

I think the code of FillStrat::affine_to_fill isn't of any use anymore.

Positioning of an image is a work for the Image::layout function, plus the current implementation is much simplier than calculating an affine matrix for then defining positioning/layout inside a BoxConstraint.

But maybe I'm missing something.

@DJMcNab
Copy link
Member

DJMcNab commented Sep 30, 2024

Next steps here are removing any unused code, if there is some. I think renaming FillStrat to ObjectFit is uncontroversial, and I'd recommend doing so in this PR. We can always rename it again if needed.

@waywardmonkeys
Copy link
Contributor

If you rename it, we can remove it from the typos config too!

@failingprovince
Copy link
Contributor Author

Next steps here are removing any unused code, if there is some. I think renaming FillStrat to ObjectFit is uncontroversial, and I'd recommend doing so in this PR. We can always rename it again if needed.

Renamed the struct but couldn't find any unused code to remove.

If you rename it, we can remove it from the typos config too!

Done.

DJMcNab
DJMcNab approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Let me know when you're happy for this to land once you've addressed/decided against these comments.

masonry/src/widget/image.rs
@@ -28,7 +28,7 @@ use crate::{
/// than the image size).
pub struct Image {
image_data: ImageBuf,
fill: FillStrat,
fill: ObjectFit,
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this field? I'd suggest either fit or object_fit?

masonry/src/widget/mod.rs
@@ -61,7 +61,7 @@ use crate::{Affine, Size};
// These are based on https://api.flutter.dev/flutter/painting/BoxFit-class.html
/// Strategies for inscribing a rectangle inside another rectangle.
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit in these docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DJMcNab DJMcNab DJMcNab approved these changes

@jaredoconnell jaredoconnell jaredoconnell approved these changes

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

Successfully merging this pull request may close these issues.

Masonry: Reason about Image widget layout implementation
4 participants
@failingprovince @DJMcNab @jaredoconnell @waywardmonkeys