Better error handling for image loading errors on iOS/Android #849

hartez · 2017-03-30T22:28:00Z

Description of Change

Prevents the Android and iOS ImageRenderer classes from throwing uncatchable exceptions from async void image loading methods and crashing the application
Adds logging for image loading errors to iOS and Android IImageSourceHandler implementations and ImageRenderer classes
Provides a mechanism for custom ImageRenderer subclasses to use an alternative error handling method
Updates Image.IsLoading when image loading errors occur so it's not left at IsLoading == true forever.
Adds automated tests for Image.IsLoading and error logging when image loading errors occur

Bugs Fixed

51173 – ImageRenderer, async void SetImage - Cannot catch exceptions

API Changes

Updated the ToString implementations on FileImageSource and UriImageSource to be more informative.

Behavioral Changes

The messages logged to the Xamarin.Forms Log class when an image cannot be loaded have changed to be slightly more specific.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense

rmarinho · 2017-04-06T17:15:44Z

@hartez do you mind rebase please.

Thanks

rmarinho · 2017-04-11T09:42:07Z

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla37625.cs

    {
        protected override async void Init ()
        {
            int retry = 5;
            while (retry-- >= 0) {
-                var imageUri = new Uri ("https://xamarin.com/content/images/pages/products/platform.png");
+                var imageUri = new Uri ("https://raw.githubusercontent.com/xamarin/Xamarin.Forms/master/Xamarin.Forms.ControlGallery.Android/Assets/WebImages/XamarinLogo.png");


think this is failing the tests

seems to only happen when there's no cache of the image.

Consistent error logging and IsLoading on Android,iOS,UWP Move error logging into image handlers for better messages Add demo of custom ImageRenderer error handling Update docs Make the test smaller so the results don't get pushed offscreen Fix namespace error

…rations

…IsLoading

Use local disposed member where possible for disposed check

rmarinho

Minor comments that could or not be address.. not important stuff to block the pr

rmarinho · 2017-04-21T15:47:39Z

Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj

@@ -43,19 +46,20 @@
    <ErrorReport>prompt</ErrorReport>
    <WarningLevel>4</WarningLevel>
    <AndroidUseSharedRuntime>False</AndroidUseSharedRuntime>
-    <AndroidLinkMode>Full</AndroidLinkMode>
+    <AndroidLinkMode>None</AndroidLinkMode>


Do we need this ?

No, better revert the linker, multidex, etc.

rmarinho · 2017-04-21T15:49:03Z

Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs

 			if (Device.IsInvokeRequired)
 				throw new InvalidOperationException("Image Bitmap must not be updated from background thread");

 			if (previousImage != null && Equals(previousImage.Source, newImage.Source))
 				return;

-			((IImageController)newImage).SetIsLoading(true);
+			((IImageController)newImage)?.SetIsLoading(true);


can we use as IImageController since we are changing ?

rmarinho · 2017-04-21T15:50:25Z

Xamarin.Forms.Platform.Android/Extensions/ImageViewExtensions.cs

-				catch (IOException ex)
-				{
-					Internals.Log.Warning("Xamarin.Forms.Platform.Android.ImageRenderer", "Error updating bitmap: {0}", ex);
+					((IImageController)newImage).SetIsLoading(false);


same as above maybe :

( newImage as IImageController)?.SetIsLoading

rmarinho · 2017-04-21T15:51:43Z

Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs

-
-			if (_element != null)
-				_element.PropertyChanged -= OnElementPropertyChanged;
+			base.Dispose(disposing);


If order is important maybe leave a comment

rmarinho · 2017-04-21T15:52:38Z

Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs

+
+		protected async Task UpdateBitmap(Image previous = null)
+		{
+			if (_element == null || _disposed)


i would drop the { :)

samhouts

Need to turn the linker back on, but looks good otherwise.

samhouts · 2017-04-21T17:47:56Z

Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj

@@ -43,19 +46,20 @@
    <ErrorReport>prompt</ErrorReport>
    <WarningLevel>4</WarningLevel>
    <AndroidUseSharedRuntime>False</AndroidUseSharedRuntime>
-    <AndroidLinkMode>Full</AndroidLinkMode>
+    <AndroidLinkMode>None</AndroidLinkMode>


No, better revert the linker, multidex, etc.

samhouts · 2017-04-21T17:58:31Z

Xamarin.Forms.Platform.Android/FastRenderers/ImageRenderer.cs

+		{
+			// By default we'll just catch and log any exceptions thrown by UpdateBitmap so they don't bring down
+			// the application; a custom renderer can override this method and handle exceptions from
+			// UpdateBitmap differently if it wants to


…Extensions

* GH-23: Added Tizen backend (#555) * Adding the initial work to get Tizen started. #23 * Adding the initial work to get Tizen started. #23 * Use the overloads properly. * Use the overloads properly. * Tizen needs to have a background explicitly set * Added the Vibration API * Added the Vibration API * Refactoring a little bit * Refactoring a little bit * Added the Browser API and some basic permissions checking * Added the Browser API and some basic permissions checking * Added the Battery API * Added the Battery API * Added the Acceleromerter API * Added the Acceleromerter API * Added the Filesystem API * Added the Filesystem API * Update Accelerometer Initialize using GetDefaultSensor * Update Accelerometer Initialize using GetDefaultSensor * Added the Gyroscope API * Added the Gyroscope API * Added the Magnetometer API * Added the Magnetometer API * Added the Compass API * Added the Compass API * Added the Connectivity API * Added the Connectivity API * Added the Flashlight API * Added the Flashlight API * Added the SecureStorage API * Added the SecureStorage API * Added the Vibration API * Added the Vibration API * Fixed build break * Fixed build break * Added the OrientationSensor API * Fixed build break * Initialized Maps/Launcher for Tizen * Added the Permissions API * Added the Geolocation API * Added the Geocoding API * Fixed build break * Initialize Barometer API * Added the Barometer API * Added the TextToSpeech API * Added the Launcher APIs * Fixed bugs * Update Location property name * Update several modules - Update AppInfo, Browser, Clipboard, Compass, Connectivity, DisplayInfo, DeviceInfo, Geocoding, TextToSpeech modules - Change module name DataTrasfer to Share, Maps to Map - Integrated module Power to Battery, ScreenLock to DeviceDisplay * Throw PlatformNotSupportedException for Tizen * Change the exception for the power saver feature * Add tizen privileges * Add FileBase * Add ShareFileRequest * Change enum for Browser * Change parameter for Permissions * Add SensorSpeedExtensions * Add Launcher for OpenFileRequest * Fix .csproj for VS2019 * Fix sample for watch * Fix Geolocation speed * Fix TextToSpeech ptich * Fixed missing using System; This was needed for the Math calls * Add Support for ⌚OS and 📺OS (#827) * Add Support for watchOS and tvOS * Fix up exception from shared netstandard code to figure out what to send. * Update Battery.ios.watchos.cs * Integrate Tizen into the netstandard not supported area. * [Tizen] Fix reference for avoid duplication (#850) * Fix reference to avoid version collision * Remove Vector3 reference on Tizen * Fix iPad not show share sheet (#873) Need to specify bottom center of the screen. * Added the Main Thread helpers from Xamarin.Forms (#849) * Added Invoke methods from Xamarin.Forms * Updated to use Essentials approach to calling the MainThread * Updated the docs with the new MainThread methods * Make overload more readable The old code was I think incorrect in that the `funcTask()` was never awaited or returned as a task. This is the same code as the overload with the generic type parameter, but without the type param. * Updated to use UrlEncode in GetMailToUrl (#848) * Updated to use UrlEncode in GetMailToUrl Fixes #843 * Added missing using * Fixed Using order and spacings * Use WebUtility.UrlEncode on placemark extensions * Added volatile to MainThread.Android (#877) Fixes #838 * Add Launcher.TryOpenAsync (#839) * Add Launcher.TryOpenAsync * Added ConfigureAwait(false) * Removed unnecessary async * Updated launcher docs * Updated the docs * Added Launch Tests * Add aka.ms for release notes (#883) * Remove experimental flags & fix launcher teasts (#887)

dnfclas added the cla-required label Mar 30, 2017

hartez added cla-not-required and removed cla-required labels Mar 30, 2017

rmarinho self-requested a review April 6, 2017 17:16

hartez added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Apr 7, 2017

hartez force-pushed the fix-bugzilla51173 branch from e9f74b2 to d4f4ae9 Compare April 8, 2017 17:00

hartez removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Apr 8, 2017

rmarinho reviewed Apr 11, 2017

View reviewed changes

hartez added 3 commits April 13, 2017 12:54

Update error handling for fast image renderer

ff564f4

Update 37625 test to use image we control

a2089ca

hartez force-pushed the fix-bugzilla51173 branch from 2f9f3e5 to a2089ca Compare April 13, 2017 18:54

rmarinho and others added 6 commits April 18, 2017 10:30

Merge branch 'master' into fix-bugzilla51173

302f7b3

Add java disposed check to avoid ObjectDisposedException in async ope…

8a5bb17

…rations

Add disposed checks to legacy renderer; null check element before Set…

b6cfbc2

…IsLoading

Check disposed on GetDesiredSize for fast renderer

dbd4657

Use local disposed member where possible for disposed check

Check for disposal after async handlers in iOS

85f0ca5

Add disposal checks after async methods in Windows

0cdbed5

rmarinho approved these changes Apr 21, 2017

View reviewed changes

jassmith approved these changes Apr 21, 2017

View reviewed changes

samhouts approved these changes Apr 21, 2017

View reviewed changes

Reset linker settings on project; reduce redundant casts in ImageView…

db9d8aa

…Extensions

rmarinho merged commit cdc4055 into master Apr 25, 2017

rmarinho deleted the fix-bugzilla51173 branch April 26, 2017 11:39

samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

samhouts added this to the 2.3.5 milestone Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error handling for image loading errors on iOS/Android #849

Better error handling for image loading errors on iOS/Android #849

hartez commented Mar 30, 2017 •

edited by samhouts

Loading

rmarinho commented Apr 6, 2017

rmarinho Apr 11, 2017

rmarinho Apr 11, 2017

rmarinho left a comment

rmarinho Apr 21, 2017

samhouts Apr 21, 2017

rmarinho Apr 21, 2017

rmarinho Apr 21, 2017

rmarinho Apr 21, 2017

rmarinho Apr 21, 2017

samhouts left a comment

samhouts Apr 21, 2017

samhouts Apr 21, 2017

Better error handling for image loading errors on iOS/Android #849

Better error handling for image loading errors on iOS/Android #849

Conversation

hartez commented Mar 30, 2017 • edited by samhouts Loading

Description of Change

Bugs Fixed

API Changes

Behavioral Changes

PR Checklist

rmarinho commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samhouts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartez commented Mar 30, 2017 •

edited by samhouts

Loading