mchome / flutter_advanced_networkimage

flutter advanced network image provider
https://pub.dev/packages/flutter_advanced_networkimage
MIT License
285 stars 180 forks source link

Crashes without network connection without fallbackImage #131

Closed kentcb closed 4 years ago

kentcb commented 4 years ago

To reproduce, point AdvancedNetworkImage at a valid URL, then run your app with the device in airplane mode. There is an unhandled crash originating from AdvancedNetworkImage's HTTP calls.

Here is a repro:

import 'package:flutter/material.dart';
import 'package:flutter_advanced_networkimage/provider.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: ReproView(),
    );
  }
}

class ReproView extends StatefulWidget {
  @override
  _ReproViewState createState() => _ReproViewState();
}

class _ReproViewState extends State<ReproView> {
  bool showImage = false;

  @override
  Widget build(BuildContext context) => Scaffold(
      appBar: AppBar(title: Text('Crash Repro')),
      body: Column(
        children: <Widget>[
          RaisedButton(
            child: Text('Show Image (turn airplane mode on first)'),
            onPressed: () => setState(() => showImage = true),
          ),
          if (showImage)
            Image(
              image: AdvancedNetworkImage(
                'https://4.img-dpreview.com/files/p/E~TS590x0~articles/3925134721/0266554465.jpeg',
                useDiskCache: false,
                loadFailedCallback: () => debugPrint('load failed'),
              ),
            ),
        ],
      ));
}

When running in release, the app crashes. When running in debug, the following output appears in the console:


════════ Exception caught by image resource service ════════════════════════════
The following StateError was thrown resolving an image codec:
Bad state: Failed to load https://4.img-dpreview.com/files/p/E~TS590x0~articles/3925134721/0266554465.jpeg.

Image provider: AdvancedNetworkImage("https://4.img-dpreview.com/files/p/E~TS590x0~articles/3925134721/0266554465.jpeg",scale: 1.0,header: null,useDiskCache: false,retryLimit: 5,retryDuration: 0:00:00.500000,retryDurationFactor: 1.5,timeoutDuration: 0:00:05.000000)
Image key: AdvancedNetworkImage("https://4.img-dpreview.com/files/p/E~TS590x0~articles/3925134721/0266554465.jpeg",scale: 1.0,header: null,useDiskCache: false,retryLimit: 5,retryDuration: 0:00:00.500000,retryDurationFactor: 1.5,timeoutDuration: 0:00:05.000000)
════════════════════════════════════════════════════════════════════════════════
mchome commented 4 years ago

You have to set fallbackImage or else, otherwise the error will raise. You can also wrap a TransitionToImage to avoid the crash.

kentcb commented 4 years ago

@mchome Thanks - confirmed. However, I think this is really poor design for a library that is intended to shield you from network issues!

Why is a fallback required? I want to display nothing (no image) if there is any problem resolving it, so I guess I have to manually provide a one-pixel transparent image.

And if a fallback is genuinely required for some reason, why isn't it required by the API? i.e. the API should error if you don't provide a fallback so that people aren't tripped up by this problem.

mchome commented 4 years ago

I have TransitionToImage, so Image widget has zero usage. TransitionToImage can catch all the network issues in imageprovider.

If you don't like "Exception caught by image resource service", you should blame flutter.

kentcb commented 4 years ago

Hmm, maybe I'm not being very clear.

For starters, TransitionToImage cannot always be used. For example, CircleAvatar.backgroundImage is an ImageProvider, not an Image.

Secondly, I'm not suggesting Flutter is perfect, but rather that flutter_advanced_networkimage could be improved. Here are some specific suggestions. I'd be interested to hear if they would help or not.

Option 1: a default fallback

If no fallback is provided to AdvancedNetworkImage, default to using a single transparent pixel. That is, change this line to:

return await PaintingBinding.instance.instantiateImageCodec(bytesForTransparentPixelPng);

Option 2: require a fallback when creating AdvancedNetworkImage

This one's really simple, but breaking. Add an assertion to the AdvancedNetworkImage constructor like this:

assert(fallbackAssetImage != null || fallbackImage != null, 'Must provide a fallback via either fallbackAssetImage or fallbackImage')

This way developers can't use the API in an unsafe manner.

Option 3: documentation

This option should probably be done in addition to options 1 or 2. The README does not even mention fallback images, or the fact that failing to provide one can result in your application crashing.

mchome commented 4 years ago

Default fallback will make TransitionToImage unusable. The error is expected to be raised. If you want CircleAvatar to support TransitionToImage, you should rewrite your own CircleAvatar.

Btw you just cannot avoid the crash if you use flutter's default imageprovider, and you cannot find any doc about it too :(

mchome commented 4 years ago

fallbackImage will be cached in memory, so it is not a good practice.

farkerhaiku commented 4 years ago

Documentation about the nature of the expected crash should be included on the README so that people do not accidentally use this library incorrectly. I also think that the second suggestion @kentcb made is worth doing.