-
Notifications
You must be signed in to change notification settings - Fork 29.2k
fix: ColorScheme will removeListener on imageStream twice if there is error loading the image. #174465
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly fixes a bug where an image stream listener could be removed twice if an image fails to load, and adds a regression test. My review focuses on improving error handling semantics, test code quality, and robustness. I've identified a duplicated test case that should be removed, suggested more accurate error handling, and pointed out areas for code cleanup and improved robustness in the new test widget.
@@ -1256,6 +1259,24 @@ void main() { | |||
expect(colorSchemeOfTheme, colorScheme); | |||
expect(colorSchemeFromContext, colorScheme); | |||
}); | |||
|
|||
testWidgets('ColorScheme from an invalid network image should only throw one error', ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this test is failing on the web shards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
…f there is error loading the image. (flutter/flutter#174465)
This PR is based on #171261 and finishes it with a test.
fix #170413
cc @ahyangnb
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.