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

Add ResizeImage.policy #121154

Merged
merged 7 commits into from Feb 23, 2023
Merged

Add ResizeImage.policy #121154

merged 7 commits into from Feb 23, 2023

Conversation

tvolkert
Copy link
Contributor

This adds a new ResizeImage.policy property that controls how ResizeImage will interpret its width and height properties. The existing behavior is preserved via ResizeImagePolicy.exact (default), but there is now the option to use ResizeImagePolicy.fit, which satisfies the use case outlined in #118543.

The API doc assets were added in flutter/assets-for-api-docs#209

Fixes #118543

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 21, 2023
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM with some doc nits

/// [ResizeImage.width] and [ResizeImage.height].
///
/// If [ResizeImage.width] and [ResizeImage.height] are both non-null, the
/// output image will have the specified width and height, stretching the
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to be more specific about how the new aspect ratio is applied.

Maybe

output image will have the spedified width and height, with the aspect ratio of the new width and height regardless of whether it matches the intrinsic aspect ratio of the image's width and height.

///
/// If [ResizeImage.allowUpscaling] is false, the width and the height of the
/// output image will each be clamped to the intrinsic width and height of the
/// image.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

This may result in a different aspect ratio than the aspect ratio specified by the target width and height, if for example the height gets clamped downwards but the width does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, consider mentioning that this is similar to [BoxFit.fill] with consideration for null values on width and height, so setting one of those to null would end up corresponding to [BoxFit.fitWidth] or [BoxFit.fitHeight].

Consider mentioning that the fit member is similar to [BoxFit.contain].

I'm not sure if that would confuse more people or just lead to asking why we don't use BoxFit here, but to me having that linkage helps.

This adds a new `ResizeImage.policy` property that controls how `ResizeImage`
will interpret its `width` and `height` properties. The existing behavior is
preserved via `ResizeImagePolicy.exact` (default), but there is now the option
to use `ResizeImagePolicy.fit`, which satisfies the use case outlined in
flutter#118543.

The API doc assets were added in flutter/assets-for-api-docs#209

Fixes flutter#118543
@tvolkert tvolkert added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 23, 2023
@auto-submit auto-submit bot merged commit 71c4570 into flutter:master Feb 23, 2023
@tvolkert tvolkert deleted the resize_image branch February 23, 2023 03:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use ResizeImage effectively when you don't know the aspect ratio of the image ahead of time
3 participants