Add useLogicalPixels to ResizeImage for logical pixel cache sizing - #184549

I think the docs should recognize the bug. When/why should useLogicalPixels be set?

This is an option addition (default off), not a bug fix — wouldn't bug framing be excessive?

These (and other constructors) will need tests as well.

I think this is covered through image_provider_resize_image_test.dart.

The Image.* constructors forward useLogicalCacheSize to ResizeImage.resizeIfNeeded via a named parameter; image_test.dart doesn't carry similar forwarding tests for cacheWidth/cacheHeight either. Happy to add explicit forwarding tests if you'd still prefer.

I think we should still test, even if it is only the forwarding that is tested (i.e the underlying ResizeImage has the expected value). For every public API we are adding here.

But why even set this to true? The documentation should explain this.

Added a use-case hint to the field/param doc. 33d73e2

same here

and here

Instead of storing only the devicepixelratio in the key, can we store the effective size? That way if the devicepixelratio is different but the effective size (which is the downstream quantity we need) is the same we can just cache that. What do you think?

Adopted, thanks.

ResizeImageKey now stores effective decode dimensions; two ResizeImages whose post-DPR size matches share a cache entry.

Nit:

Thanks — no longer applies after the shift to opt-in default.

I don't think this is enough discoverability-wise. We need to explain what the default should be, what it is, why this might cause issues, and how to correct it. This line only explains the "how", and introduces no new information that isn't immediately obvious from the name of the flag.

I want to make sure I'm reading your comment correctly. Are you suggesting the doc should state what the default should be? For example, "logical pixels would be the appropriate default; physical is the current default."

More than that - a developer doing their due diligence before adopting this widget for use in their project should be able to know from the documentation that specifying cacheWidth/Height may result in lower quality images, why that happens, and how this flag helps resolve this mismatch.

While we don't conventionally link to issues in public-facing API docs, the docs should contain enough information for new adopters to foresee and solve the problem, and the flag should specify the correct default is logical, but is set to physical for backwards compatibility.

Would one of these match what you have in mind?

A — full inline explanation

/// Whether [width] and [height] are interpreted as logical pixels.
///
/// When `false` (the default), [width] and [height] are treated as physical
/// pixels. On high-density displays, this can result in images being decoded
/// at fewer pixels than the screen can render, producing lower-quality output.
/// When `true`, [width] and [height] are multiplied by the device pixel ratio
/// from the current [ImageConfiguration], so the image is decoded at the
/// resolution that matches the rendered display.
///
/// The correct default is logical, but it is set to physical for backwards
/// compatibility.

B — link to the originating issue

/// Whether [width] and [height] are interpreted as logical pixels.
///
/// When `false` (the default), [width] and [height] are treated as physical
/// pixels. When `true`, they are multiplied by the device pixel ratio from
/// the current [ImageConfiguration] before decoding.
///
/// See https://github.com/flutter/flutter/issues/56239 for background on why
/// physical pixels can produce lower-quality output on high-density displays.
///
/// The correct default is logical, but it is set to physical for backwards
/// compatibility.

Personally, when I encounter a new flag in the Flutter SDK, I search GitHub for the PR that introduced it — so for me, issue or PR links inside the field doc haven't been necessary.

Could you let me know which of these is appropriate as Flutter documentation? If neither matches your intent, please share the concrete wording you have in mind.

A is better, we try not to link to issues etc in widget API docs.

Slight tweaks:

• Replace the last paragraph with "Typically in the framework, pixel dimensions are interpreted as logical rather than physical. However, this defaults to physical for backwards compatibility". A behavior being correct or incorrect is a matter of perspective.

Thanks! Applied in 735b911.

Here as well.

Done here as well in 735b911.

What happens if width is unset but cacheWidth is set and useLogicalCacheSize is true?

Can we add a test for this as well?

width (layout) and cacheWidth / useLogicalCacheSize (decode) are independent paths — an unset width doesn't feed into the resize path, so cacheWidth is still DPR-multiplied. That multiplication is already asserted in image_provider_resize_image_test.dart ("useLogicalSize accounts for devicePixelRatio when decoding"), and the constructors just forward the flag. width being unset doesn't reach a distinct path, so there's no separate case to add here.