Skip to content

remove single-view assumption from paintImage #117495

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
Closed
goderbauer wants to merge 1 commit into from
Closed

remove single-view assumption from paintImage #117495

goderbauer wants to merge 1 commit into from

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Dec 21, 2022

Fixes #117477.
Part of #116929.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Dec 21, 2022
@goderbauer goderbauer changed the title remove single-view assumption from paintImage remove single-view assumption from paintImage Dec 21, 2022
@goderbauer
Copy link
Member Author

goderbauer commented Dec 21, 2022

@dnfield A while ago you added this code to paintImage that checks for oversized images. This debug check is the only reason, why the paintImage method needs to know the device pixel ratio, which it is currently reading from the global window singleton. That will not work in a multi window/view world. The method needs to read the dpr from the view the image is going to be drawn into. Unfortunately, it doesn't have access to that information. In this PR I changed it so that the dpr can (optionally) be passed into the method to enable the debug check and I made sure that everywhere in the framework we do pass it in. However, if somebody now calls paintImage directly without passing in the DPR, the oversized images check is disabled.

Another option would be to make passing in the dpr required. This would be a breaking change since there are two callers of this method in google3 (those could be easily updated, though). It does feel a little odd to require it just for the debug check, though.

Yet another option could be to move the check out of paintImage. However, I didn't really find a better place for it...

What are your thoughts?

@goderbauer goderbauer requested a review from dnfield December 21, 2022 23:02
goderbauer
goderbauer commented Dec 21, 2022
View reviewed changes
packages/flutter/lib/src/painting/decoration_image.dart
///
/// * `devicePixelRatio`: The device pixel ratio of the [FlutterView] into
/// which the image will be drawn. Can be obtained via
/// `View.of(context).devicePixelRation`.
Copy link
Member Author

@goderbauer goderbauer Dec 21, 2022

Choose a reason for hiding this comment

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

note to self: mention MediaQuery.devicePixelRatioOf(context) as well

Copy link
Contributor

@dnfield dnfield Jan 18, 2023

Choose a reason for hiding this comment

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

Suggested change
/// `View.of(context).devicePixelRation`.
/// `View.of(context).devicePixelRatio`.

Copy link
Contributor

@dnfield dnfield Jan 18, 2023

Choose a reason for hiding this comment

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

Can this get defaulted somehow?

For example, just default it to the first view in the collection of views?

Copy link
Member Author

@goderbauer goderbauer Jan 18, 2023

Choose a reason for hiding this comment

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

If we default this, the assert check in paintImage becomes more or less meaningless, no? It needs to know the exact dpr in order to figure out whether the current image dimensions are overkill or not.

Copy link
Member Author

@goderbauer goderbauer Jan 18, 2023

Choose a reason for hiding this comment

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

We could just use the max device pixel ratio of all views as an upper bound, though. That will greatly simplify things.

goderbauer
goderbauer commented Dec 21, 2022
View reviewed changes
packages/flutter/lib/src/widgets/basic.dart
invertColors: invertColors,
isAntiAlias: isAntiAlias,
filterQuality: filterQuality,
devicePixelRatio: View.maybeOf(context)?.devicePixelRatio,
Copy link
Member Author

@goderbauer goderbauer Dec 21, 2022

Choose a reason for hiding this comment

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

note to self: here and below use MediaQuery.maybeDevicePixelRatioOf(context) instead.

@goderbauer
Copy link
Member Author

goderbauer commented Dec 22, 2022

"Google testing" failed because tests were run without including prerequisite #117244.

This was referenced Jan 18, 2023
Closed
Merged
@goderbauer
Copy link
Member Author

goderbauer commented Jan 18, 2023

Superseded by #118721.

@goderbauer goderbauer closed this Jan 18, 2023
@goderbauer goderbauer deleted the paintImage branch January 18, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnfield dnfield Awaiting requested review from dnfield

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

Multi Window
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove hard-coded devicePixelRatio from paintImage

2 participants

@goderbauer @dnfield