Skip to content

Separate web and io implementations of network image - #34112

Merged
jonahwilliams merged 14 commits into
flutter:masterfrom
jonahwilliams:add_network_image_impl
Jun 15, 2019
Merged

jonahwilliams merged 14 commits into
flutter:masterfrom
jonahwilliams:add_network_image_impl

Conversation

@jonahwilliams

@jonahwilliams jonahwilliams commented Jun 8, 2019 •

Copy link
Copy Markdown
Contributor

Description

Both NetworkImage and AssetImage use dart:io functionality that cannot be supported on the web. Similar to what we've done with compute, bitfield, and defaultTargetPlatform we can separate them into io and web implementations, with the latter coming directly from flutter_web.

To ensure the interfaces stay consistent, the original class becomes abstract and then redirects to an imported subclass.

There are also other ways these could be split up, by making certain functions public and then importing these.

#34082

Update: The usage of dart:io by AssetImage seems trivial and can be replaced, assuming that we're dealing with file URIs and not file paths.

@jonahwilliams jonahwilliams added framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically labels Jun 8, 2019
@jonahwilliams

Copy link
Copy Markdown
Contributor Author

cc @yjbanov @hterkelsen

@jonahwilliams

Copy link
Copy Markdown
Contributor Author

also cc @goderbauer

Comment thread packages/flutter/lib/src/painting/_image_provider_io.dart
Comment thread packages/flutter/test/painting/image_resolution_test.dart Outdated
Comment thread packages/flutter/test/painting/image_provider_test.dart Outdated
Comment thread packages/flutter/lib/src/painting/_image_resolution_web.dart Outdated
Comment thread packages/flutter/lib/src/painting/_image_provider_web.dart
@jonahwilliams jonahwilliams changed the title [Discuss] Separate web and io implementations of network and asset image [Discuss] Separate web and io implementations of network image Jun 10, 2019
Comment thread packages/flutter/lib/src/painting/image_resolution.dart Outdated
@jonahwilliams jonahwilliams changed the title [Discuss] Separate web and io implementations of network image Separate web and io implementations of network image Jun 10, 2019
Comment thread packages/flutter/lib/src/painting/_network_image_web.dart

@harryterkelsen harryterkelsen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, as long as the use of File and Directory to get the asset directory path were just because that is a convenient way to implement it, and not because they needed platform-specific handling of path separators or something

Comment thread packages/flutter/lib/src/painting/image_provider.dart Outdated
@jonahwilliams
jonahwilliams requested a review from goderbauer June 11, 2019 23:45
@Piinks Piinks added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 12, 2019
Comment thread packages/flutter/lib/src/painting/_network_image_io.dart Outdated
Comment thread packages/flutter/lib/src/painting/image_resolution.dart Outdated
Comment thread packages/flutter/lib/src/painting/image_resolution.dart Outdated
Comment thread packages/flutter/test/painting/image_provider_test.dart Outdated
@jonahwilliams
jonahwilliams requested a review from goderbauer June 13, 2019 06:00

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with some nits

Comment thread packages/flutter/lib/src/painting/_network_image_io.dart Outdated
Comment thread packages/flutter/lib/src/painting/_network_image_web.dart Outdated
Comment thread packages/flutter/lib/src/painting/_network_image_web.dart
Comment thread packages/flutter/lib/src/painting/image_provider.dart
@jonahwilliams
jonahwilliams merged commit dfa39f3 into flutter:master Jun 15, 2019
@jonahwilliams
jonahwilliams deleted the add_network_image_impl branch June 15, 2019 17:21
tango5614 added a commit to tango5614/flutter that referenced this pull request Jun 17, 2019
* master:
  retry on HttpException (flutter#34526)
  308c113 Roll src/third_party/skia 057a32d9a2c8..5fe7429babe2 (1 commits) (flutter-team-archive/engine#9342) (flutter#34583)
  Add back ability to override the local engine in Gradle (flutter#34460)
  Added customizable padding for the segmented controll (flutter#34555)
  466a1d8 Roll src/third_party/skia 3721688f64a5..057a32d9a2c8 (1 commits) (flutter-team-archive/engine#9340) (flutter#34558)
  Handles parsing APK manifests with additional namespaces or attributes (flutter#34535)
  add route information to Flutter.Navigation events (flutter#34508)
  Separate web and io implementations of network image (flutter#34112)
  pass .packages path to snapshot invocation (flutter#34517)
  [Material] Fix TextDirection and selected thumb for RangeSliderThumbShape and RangeSliderValueIndicatorShape (flutter#34501)
  redux of a change to use new engine APIs for Flutter.Frame events (flutter#34521)
  Roll engine b0757e6..f44b7b5 (4 commits) (flutter#34533)
  Roll engine 2589785..b0757e6 (6 commits) (flutter#34522)
  Revert "Revert "redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)" (flutter#34514)" (flutter#34530)
  Make sure fab semantics end up on top (flutter#34512)
  Revert "redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)" (flutter#34514)
  redux of a change to use new engine APIs for Flutter.Frame events (flutter#34365)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants