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

[flutter_svg] Error handling enhancement #8016

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/vector_graphics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 1.1.14
* Adds error handling when parsing an invalid svg string.
* Adds error handling when downloading svg string from network.
* Adds error handling when reading svg file from asset.
* Expose ErrorWidgetBuilder.

## 1.1.13

* Fix execution on the web with WebAssembly.
Expand Down
2 changes: 1 addition & 1 deletion packages/vector_graphics/lib/src/listener.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ final Map<BytesLoader, Completer<void>> _pendingDecodes =
/// Decode a vector graphics binary asset into a [Picture].
///
/// Throws a [StateError] if the data is invalid.
Future<PictureInfo> decodeVectorGraphics(
Future<PictureInfo?> decodeVectorGraphics(
ByteData data, {
required Locale? locale,
required TextDirection? textDirection,
Expand Down
68 changes: 40 additions & 28 deletions packages/vector_graphics/lib/src/vector_graphics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'dart:math' as math;
import 'dart:ui' as ui;

import 'package:flutter/cupertino.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';
import 'package:vector_graphics_codec/vector_graphics_codec.dart';
Expand Down Expand Up @@ -279,8 +280,7 @@ class _PictureData {

@immutable
class _PictureKey {
const _PictureKey(
this.cacheKey, this.locale, this.textDirection, this.clipViewbox);
const _PictureKey(this.cacheKey, this.locale, this.textDirection, this.clipViewbox);

final Object cacheKey;
final Locale? locale;
Expand All @@ -306,10 +306,8 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
Locale? locale;
TextDirection? textDirection;

static final Map<_PictureKey, _PictureData> _livePictureCache =
<_PictureKey, _PictureData>{};
static final Map<_PictureKey, Future<_PictureData>> _pendingPictures =
<_PictureKey, Future<_PictureData>>{};
static final Map<_PictureKey, _PictureData?> _livePictureCache = <_PictureKey, _PictureData?>{};
static final Map<_PictureKey, Future<_PictureData?>> _pendingPictures = <_PictureKey, Future<_PictureData?>>{};

@override
void didChangeDependencies() {
Expand Down Expand Up @@ -345,29 +343,38 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
}
}

Future<_PictureData> _loadPicture(
BuildContext context, _PictureKey key, BytesLoader loader) {
Future<_PictureData?> _loadPicture(BuildContext context, _PictureKey key, BytesLoader loader) {
if (_pendingPictures.containsKey(key)) {
return _pendingPictures[key]!;
}
final Future<_PictureData> result =
loader.loadBytes(context).then((ByteData data) {
return decodeVectorGraphics(
data,
locale: key.locale,
textDirection: key.textDirection,
clipViewbox: key.clipViewbox,
loader: loader,
onError: (Object error, StackTrace? stackTrace) {
return _handleError(
error,
stackTrace,
);
},
);
}).then((PictureInfo pictureInfo) {

final Future<_PictureData?> result = loader.loadBytes(context).then((ByteData data) async {
if (data.lengthInBytes == 0) {
debugPrint('_VectorGraphicWidgetState.decodeVectorGraphics: empty');
_handleError(const FormatException('Empty SVG xml content'), null);
return null;
} else {
return decodeVectorGraphics(data,
locale: key.locale,
textDirection: key.textDirection,
clipViewbox: key.clipViewbox,
loader: loader, onError: (Object error, StackTrace? stackTrace) {
debugPrintStack(
stackTrace: stackTrace, label: '_VectorGraphicWidgetState.decodeVectorGraphics.onError: $error');
_handleError(error, stackTrace);
});
}
}).onError((Object? error, StackTrace stackTrace) {
debugPrintStack(stackTrace: stackTrace, label: '_VectorGraphicWidgetState._loadPictureInfo.onError: $error');
_handleError(error ?? '', stackTrace);
return null;
}).then((PictureInfo? pictureInfo) {
if (pictureInfo == null) {
return null;
}
return _PictureData(pictureInfo, 0, key);
});

_pendingPictures[key] = result;
result.whenComplete(() {
_pendingPictures.remove(key);
Expand All @@ -376,6 +383,9 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
}

void _handleError(Object error, StackTrace? stackTrace) {
if (!mounted) {
return;
}
setState(() {
_error = error;
_stackTrace = stackTrace;
Expand All @@ -385,8 +395,7 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
void _loadAssetBytes() {
// First check if we have an avilable picture and use this immediately.
final Object loaderKey = widget.loader.cacheKey(context);
final _PictureKey key =
_PictureKey(loaderKey, locale, textDirection, widget.clipViewbox);
final _PictureKey key = _PictureKey(loaderKey, locale, textDirection, widget.clipViewbox);
final _PictureData? data = _livePictureCache[key];
if (data != null) {
data.count += 1;
Expand All @@ -398,7 +407,10 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
}
// If not, then check if there is a pending load.
final BytesLoader loader = widget.loader;
_loadPicture(context, key, loader).then((_PictureData data) {
_loadPicture(context, key, loader).then((_PictureData? data) {
if (data == null) {
return;
}
data.count += 1;

// The widget may have changed, requesting a new vector graphic before
Expand Down Expand Up @@ -674,7 +686,7 @@ class VectorGraphicUtilities {
///
/// It is the caller's responsibility to handle disposing the picture when
/// they are done with it.
Future<PictureInfo> loadPicture(
Future<PictureInfo?> loadPicture(
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a breaking change, so will need to be evaluated in the context of flutter/flutter#157626

If the decision there is (as I hope) to start using semver for vector_graphics*, this would need to be versioned accordingly, or would need a non-breaking alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than making it null, we can throw/catch an exception I think.

BytesLoader loader,
BuildContext? context, {
bool clipViewbox = true,
Expand Down
2 changes: 1 addition & 1 deletion packages/vector_graphics/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+
# See https://github.com/flutter/flutter/issues/157626 before publishing a new
# version.
publish_to: none
version: 1.1.13
version: 1.1.14

environment:
sdk: ^3.4.0
Expand Down
16 changes: 8 additions & 8 deletions packages/vector_graphics/test/listener_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -43,31 +43,31 @@ void main() {
});

test('decode without clip', () async {
final PictureInfo info = await decodeVectorGraphics(
final PictureInfo? info = await decodeVectorGraphics(
vectorGraphicBuffer,
locale: ui.PlatformDispatcher.instance.locale,
textDirection: ui.TextDirection.ltr,
clipViewbox: true,
loader: const AssetBytesLoader('test'),
);
final ui.Image image = info.picture.toImageSync(15, 15);
final Uint32List imageBytes =
(await image.toByteData())!.buffer.asUint32List();
expect(info, isNotNull);
final ui.Image image = info!.picture.toImageSync(15, 15);
final Uint32List imageBytes = (await image.toByteData())!.buffer.asUint32List();
expect(imageBytes.first, 0xFF000000);
expect(imageBytes.last, 0x00000000);
}, skip: kIsWeb);

test('decode with clip', () async {
final PictureInfo info = await decodeVectorGraphics(
final PictureInfo? info = await decodeVectorGraphics(
vectorGraphicBuffer,
locale: ui.PlatformDispatcher.instance.locale,
textDirection: ui.TextDirection.ltr,
clipViewbox: false,
loader: const AssetBytesLoader('test'),
);
final ui.Image image = info.picture.toImageSync(15, 15);
final Uint32List imageBytes =
(await image.toByteData())!.buffer.asUint32List();
expect(info, isNotNull);
final ui.Image image = info!.picture.toImageSync(15, 15);
final Uint32List imageBytes = (await image.toByteData())!.buffer.asUint32List();
expect(imageBytes.first, 0xFF000000);
expect(imageBytes.last, 0xFF000000);
}, skip: kIsWeb);
Expand Down
40 changes: 20 additions & 20 deletions packages/vector_graphics/test/render_vector_graphics_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import 'package:vector_graphics/vector_graphics.dart';
import 'package:vector_graphics_codec/vector_graphics_codec.dart';

void main() {
late PictureInfo pictureInfo;
late PictureInfo? pictureInfo;

tearDown(() {
// Since we don't always explicitly dispose render objects in unit tests, manually clear
Expand All @@ -41,7 +41,7 @@ void main() {

test('Rasterizes a picture to a draw image call', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -62,15 +62,15 @@ void main() {

test('Multiple render objects with the same scale share a raster', () async {
final RenderVectorGraphic renderVectorGraphicA = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
null,
1.0,
);
final RenderVectorGraphic renderVectorGraphicB = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -91,15 +91,15 @@ void main() {

test('disposing render object release raster', () async {
final RenderVectorGraphic renderVectorGraphicA = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
null,
1.0,
);
final RenderVectorGraphic renderVectorGraphicB = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -126,15 +126,15 @@ void main() {
'Multiple render objects with the same scale share a raster, different load order',
() async {
final RenderVectorGraphic renderVectorGraphicA = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
null,
1.0,
);
final RenderVectorGraphic renderVectorGraphicB = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -158,7 +158,7 @@ void main() {

test('Changing color filter does not re-rasterize', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -185,7 +185,7 @@ void main() {
test('Changing device pixel ratio does re-rasterize and dispose old raster',
() async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -210,7 +210,7 @@ void main() {

test('Changing scale does re-rasterize and dispose old raster', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -235,7 +235,7 @@ void main() {

test('The raster size is increased by the inverse picture scale', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -254,7 +254,7 @@ void main() {

test('The raster size is increased by the device pixel ratio', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
2.0,
Expand All @@ -273,7 +273,7 @@ void main() {
test('The raster size is increased by the device pixel ratio and ratio',
() async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
2.0,
Expand All @@ -292,7 +292,7 @@ void main() {
test('Changing size asserts if it is different from the picture size',
() async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -313,7 +313,7 @@ void main() {
test('Does not rasterize a picture when fully transparent', () async {
final FixedOpacityAnimation opacity = FixedOpacityAnimation(0.0);
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -339,7 +339,7 @@ void main() {
test('paints partially opaque picture', () async {
final FixedOpacityAnimation opacity = FixedOpacityAnimation(0.5);
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -355,7 +355,7 @@ void main() {

test('Disposing render object disposes picture', () async {
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand All @@ -376,7 +376,7 @@ void main() {
test('Removes listeners on detach, dispose, adds then on attach', () async {
final FixedOpacityAnimation opacity = FixedOpacityAnimation(0.5);
final RenderVectorGraphic renderVectorGraphic = RenderVectorGraphic(
pictureInfo,
pictureInfo!,
'test',
null,
1.0,
Expand Down Expand Up @@ -412,7 +412,7 @@ void main() {

test('Color filter applies clip', () async {
final RenderPictureVectorGraphic render = RenderPictureVectorGraphic(
pictureInfo,
pictureInfo!,
const ui.ColorFilter.mode(Colors.green, ui.BlendMode.difference),
null,
);
Expand Down
6 changes: 6 additions & 0 deletions third_party/packages/flutter_svg/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 2.0.12
* Adds error handling when parsing an invalid svg string.
* Adds error handling when downloading svg string from network.
* Adds error handling when reading svg file from asset.
* Expose ErrorWidgetBuilder.

## 2.0.11

* Transfers the package source from https://github.com/dnfield/flutter_svg
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading