[go: nahoru, domu]
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

Use ResizeImage in ColorScheme._imageProviderToScaled and remove timeout #150560

Closed
wants to merge 15 commits into from
Closed

Use ResizeImage in ColorScheme._imageProviderToScaled and remove timeout #150560

wants to merge 15 commits into from

Conversation

gawsfbet
Copy link
@gawsfbet gawsfbet commented Jun 20, 2024 •
edited
Loading

The timeout case should be handled by the caller of ColorScheme.fromImageProvider.

Note: the current impl where the listener is removed multiple times might result in the error Bad state: Stream has been disposed., as the stream attempts to remove the listener twice (once during image resolution and once when the timer expires)

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
#124806

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla Google CLA
Copy link
google-cla bot commented Jun 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jun 20, 2024
@Piinks
Copy link
Contributor
Piinks commented Jun 20, 2024

Hey @gawsfbet can you sign the cla? This will likely need a test as well. Thank you for contributing!

@Piinks Piinks requested a review from QuncCccccc June 20, 2024 20:07
@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch 2 times, most recently from cb16672 to 1e2db4f Compare June 26, 2024 05:55
@gawsfbet gawsfbet changed the title Dispose resources at end of ColorScheme._imageProviderToScaled Use ResizeImage in ColorScheme._imageProviderToScaled and remove timeout Jun 26, 2024
@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch from 31b840f to 38d784b Compare June 26, 2024 07:03
@gawsfbet
Copy link
Author

Hey @gawsfbet can you sign the cla? This will likely need a test as well. Thank you for contributing!

Already done it, thanks!

This PR mainly consists of refactoring a private method, and I have modified existing tests according to the new behavior. Do I need to write additional tests as well?

@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch from 1bc6cfe to cdfe054 Compare June 26, 2024 13:52
@QuncCccccc
Copy link
Contributor

This PR mainly consists of refactoring a private method, and I have modified existing tests according to the new behavior. Do I need to write additional tests as well?

Thanks a lot for the contribution! Yes, if there are any new behaviors, please add unit tests to cover that:) Please let me know once this PR is ready to review! I saw it's still a draft, so not sure whether I should start reviewing😄

@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch from b4368e6 to 5fff70d Compare July 2, 2024 03:41
@gawsfbet gawsfbet marked this pull request as ready for review July 2, 2024 05:41
@gawsfbet
Copy link
Author
gawsfbet commented Jul 2, 2024

Hi @QuncCccccc , the PR is ready for review now, thanks! With regards to the failing Google test, that particular test case that is failing is owned by my team so I will raise a fix for that before this PR is merged.

QuncCccccc
QuncCccccc reviewed Jul 8, 2024
View reviewed changes
Copy link
Contributor
@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! If the google testing failures are fixed, can you rebase master to trigger the test again?

packages/flutter/lib/src/material/color_scheme.dart Show resolved Hide resolved
@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch from d15eeb6 to d65bef9 Compare July 19, 2024 07:30
QuncCccccc
QuncCccccc approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor
@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution:)

packages/flutter/lib/src/material/color_scheme.dart Show resolved Hide resolved
Renzo-Olivares
Renzo-Olivares approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor
@Renzo-Olivares Renzo-Olivares 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 small nit. Thanks for the contribution!

packages/flutter/lib/src/material/color_scheme.dart Outdated Show resolved Hide resolved
@gawsfbet
Call stream.removeListener and timer.cancel at the end of _imageProvi…
8e99283 
…derToScaled method
@gawsfbet
Use ResizeImage to resize image in _imageProviderToScaled, and remove…
4341dee 
… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
@gawsfbet
Use ResizeImage to resize image in _imageProviderToScaled, and remove…
d1e1da4 
… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
@gawsfbet
Use ResizeImage to resize image in _imageProviderToScaled, and remove…
e1ff803 
… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
@gawsfbet
Fix lint warnings
0e8d599
@gawsfbet
Add type annotation to ImageStreamListener parameter
38ff297
@gawsfbet
Fix failing test
dab7b82
@gawsfbet
Attempt to fix failing test
4d3ac48
@gawsfbet
Attempt to fix broken test
0a87879
@gawsfbet
Attempt to fix broken test
c5ec88d
@gawsfbet
Attempt to fix broken test
4b19023
@gawsfbet gawsfbet force-pushed the remove-lister-at-imageprovidertoscaled-end branch from 1179772 to 626ad94 Compare July 22, 2024 03:04
@Piinks
Copy link
Contributor
Piinks commented Aug 7, 2024

Hey @gawsfbet checking in from PR triage! Is this still waiting on resolving some Google failures?

@Piinks
Copy link
Contributor
Piinks commented Sep 11, 2024

I am going to close this PR, since we have not heard back in a while. Please do not hesitate to reopen it so we can land it if you'd like to come back! Thanks!

@Piinks Piinks closed this Sep 11, 2024
@Piinks Piinks mentioned this pull request Sep 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lauweijie lauweijie lauweijie left review comments

@QuncCccccc QuncCccccc QuncCccccc approved these changes

@Renzo-Olivares Renzo-Olivares Renzo-Olivares approved these changes

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gawsfbet @Piinks @QuncCccccc @Renzo-Olivares @lauweijie