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

[vector_graphics] handle errors from bytes loader #8080

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

navaronbracke
Copy link
Contributor

@navaronbracke navaronbracke commented Nov 14, 2024

This PR updates the VectorGraphicsWidget to handle errors from the underlying bytes loader using the existing error handling in its implementation.

List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#158862

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@@ -376,13 +371,17 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
}

void _handleError(Object error, StackTrace? stackTrace) {
if (!mounted) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this function is invoked after the future has been initiated, the widget could be unmounted.

@@ -358,12 +359,6 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
textDirection: key.textDirection,
clipViewbox: key.clipViewbox,
loader: loader,
onError: (Object error, StackTrace? stackTrace) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error handler has been moved higher up, so this one should not be needed anymore.

setState(() {
_error = error;
_stackTrace = stackTrace;
});
}

void _loadAssetBytes() {
Future<void> _loadAssetBytes() async {
Copy link
Contributor Author

@navaronbracke navaronbracke Nov 14, 2024

Choose a reason for hiding this comment

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

I initially tried using a catchError() on the _loadPicture() call, but that still caused the written test to fail.

Since async work in a void method smells of https://dart.dev/tools/linter-rules/avoid_void_async (even though we did not use async here) I opted to change this to Future<void>, which did fix my test.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

LGTM

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2024
Copy link
Contributor

auto-submit bot commented Nov 15, 2024

auto label is removed for flutter/packages/8080, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2024
@auto-submit auto-submit bot merged commit 913b99e into flutter:main Nov 21, 2024
77 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 21, 2024
flutter/packages@e95f6d8...913b99e

2024-11-21 brackenavaron@gmail.com [vector_graphics] handle errors from
bytes loader (flutter/packages#8080)
2024-11-21 8276375+derdilla@users.noreply.github.com [flutter_svg] Fix
SvgNetworkLoader not closing internal http client
(flutter/packages#8126)
2024-11-20 30872003+misos1@users.noreply.github.com
[video_player_avfoundation] send video load failure even when eventsink
was initialized late (flutter/packages#7194)
2024-11-20 kevmoo@users.noreply.github.com [flutter_markdown] enable
Wasm support (flutter/packages#8120)
2024-11-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[url_launcher] Add Swift Package Manager integration to example app
(#8128)" (flutter/packages#8136)
2024-11-20 737941+loic-sharma@users.noreply.github.com [url_launcher]
Add Swift Package Manager integration to example app
(flutter/packages#8128)
2024-11-20 stuartmorgan@google.com [pigeon] Enable example app build in
CI (flutter/packages#8119)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
sinyu1012 added a commit to sinyu1012/packages that referenced this pull request Nov 22, 2024
* main: (64 commits)
  [quick_actions_plaform_interface] add localizedSubtitle (flutter#8112)
  [tools] Don't check license of generated Swift package (flutter#8137)
  Roll Flutter from 8536b96ebb3e to 93d772c5cdd8 (37 revisions) (flutter#8147)
  [go_router] Fix: Consistent PopScope Handling on Root Routes issue #140869 (flutter#8045)
  [in_app_purchase_storekit] fix price displayed with wrong precision (flutter#8127)
  [pigeon] Removes the `@protected` annotation from the InstanceManager field of the   `PigeonInternalProxyApiBaseClass` (flutter#8125)
  [google_maps_flutter] Use structured Pigeon data on iOS (flutter#8142)
  [vector_graphics] handle errors from bytes loader (flutter#8080)
  [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter#8126)
  [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter#7194)
  [flutter_markdown] enable Wasm support (flutter#8120)
  Reverts "[url_launcher] Add Swift Package Manager integration to example app (flutter#8128)" (flutter#8136)
  [url_launcher] Add Swift Package Manager integration to example app (flutter#8128)
  [pigeon] Enable example app build in CI (flutter#8119)
  [in_app_purchase_storekit] disallow ios versions lower than supported from enabling storekit (flutter#8110)
  [interactive_media_ads]: Bump com.google.ads.interactivemedia.v3:interactivemedia from 3.35.1 to 3.36.0 in /packages/interactive_media_ads/android (flutter#8046)
  [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.1 in /packages/interactive_media_ads/android (flutter#7980)
  [webview_flutter_android] Updates plugin to use `ProxyApis`s (flutter#7794)
  [interactive_media_ads] Adds support to define parameters that control the rendering of ads (flutter#8057)
  Roll Flutter from b3818f6b5979 to 8536b96ebb3e (22 revisions) (flutter#8124)
  ...

# Conflicts:
#	packages/quick_actions/quick_actions_platform_interface/CHANGELOG.md
mchudy pushed a commit to leancodepl/packages that referenced this pull request Nov 25, 2024
This PR updates the VectorGraphicsWidget to handle errors from the underlying bytes loader using the existing error handling in its implementation.

*List which issues are fixed by this PR. You must list at least one issue.*
Fixes flutter/flutter#158862
@navaronbracke navaronbracke deleted the bytes_loader_on_error branch November 26, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: vector_graphics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vector_graphics] adjust vector_graphics errorBuilder (to trigger on BytesLoader errors)
3 participants