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

Sample code for ImageProvider #131952

Merged
merged 1 commit into from Aug 9, 2023
Merged

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented Aug 4, 2023

Also:

  • minor improvements to documentation
  • wrap one of our test error messages in a manner more consistent with other messages

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Aug 4, 2023
@Hixie Hixie force-pushed the imageprovider branch 4 times, most recently from 9cdd279 to b45c51d Compare August 8, 2023 19:01
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 24 to 27
if (configuration.platform == null)
'platform': null.toString(),
if (configuration.platform != null)
'platform': configuration.platform!.toString().substring('$TargetPlatform'.length + 1),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't configuration.platform an enum so this could be simplified to:

Suggested change
if (configuration.platform == null)
'platform': null.toString(),
if (configuration.platform != null)
'platform': configuration.platform!.toString().substring('$TargetPlatform'.length + 1),
'platform': configuration.platform?.name ?? null.toString(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I always forget about EnumName because it's not in the API docs for Enum

Copy link
Member

Choose a reason for hiding this comment

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

Right! Send a PR to add it? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it simplified even more than you suggested, heh

Copy link
Member

Choose a reason for hiding this comment

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

Beautiful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted dart-lang/sdk#53164 for the Enum doc issue.

Comment on lines 30 to 35
if (configuration.textDirection == null)
'bidi': null.toString(),
if (configuration.textDirection == TextDirection.ltr)
'bidi': 'ltr',
if (configuration.textDirection == TextDirection.rtl)
'bidi': 'rtl',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (configuration.textDirection == null)
'bidi': null.toString(),
if (configuration.textDirection == TextDirection.ltr)
'bidi': 'ltr',
if (configuration.textDirection == TextDirection.rtl)
'bidi': 'rtl',
'bidi': configuration.textDirection?.name ?? null.toString(),

debugPrint('Fetching "$key"...');
return MultiFrameImageStreamCompleter(
codec:
_httpClient.getUrl(key)
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting here seems off. Maybe move this to the previous line (or indent the .then lines further)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 9, 2023

auto label is removed for flutter/flutter/131952, due to - The status or check suite Linux web_canvaskit_tests_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023
Also:
- minor improvements to documentation
- wrap one of our test error messages in a manner more consistent with other messages
@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023
@auto-submit auto-submit bot merged commit 9c8f395 into flutter:master Aug 9, 2023
135 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants