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

Use ResizeImage in ColorScheme._imageProviderToScaled and remove timeout #150560

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

gawsfbet
Copy link

@gawsfbet gawsfbet commented Jun 20, 2024

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.

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?

… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
… 5sec timeout (which should be handled by the caller of ColorScheme.fromImageProvider)
@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😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants