[Android] Vector files can be used as Image source #564

Merged
merged 6 commits into from Feb 22, 2017

Conversation

Projects
None yet
5 participants
@adrianknight89
Contributor

adrianknight89 commented Nov 26, 2016

Description of Change

Android Image now supports vector files. Note that the bug description also mentions Button.Image not supporting vectors. I have a solution for that, but it's not a complete solution.

The test code has three images in landscape, square, and portrait sizes and tests them in different aspects.

To expand on the Button issue, although vectors can be loaded and shown, when Text is empty, the images do not resize properly. I have done some manual work such as converting vectors to bitmap drawables and then scaled bitmaps on layout change, but this resulted in pixelated images, so this PR only addresses the Image part of the bug.

Bugs Fixed

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
@StephaneDelcroix

don't forget Page.Icon

{
- Log.Warning("Xamarin.Forms.Platform.Android.ImageRenderer", "Error updating bitmap: {0}", ex);
+ Control.SetImageResource(ResourceManager.GetDrawableByName(((FileImageSource)Element.Source).File));

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 27, 2016

Member

can't you get/create a bitmap from the vector source ?

@StephaneDelcroix

StephaneDelcroix Nov 27, 2016

Member

can't you get/create a bitmap from the vector source ?

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 27, 2016

Contributor

The problem with that approach is that converting vectors to bitmaps is an expensive procedure especially on the argb8888 setting which allocates 4 bytes per pixel. If you had a list of vectors and you navigated to and from the page frequently, it's easy to end up with an OutOfMemoryError exception. Of course, one solution would be to cache the results. SetImageResource() seems to be doing everything including resizing out of the box. However, the downside is that decoding is done on the UI thread. If this creates a latency concern, then we'd need to consider caching one again.

This change is by no means an advanced solution. It's just providing basic support for vectors.

P.S. It looks like ImageRenderer's UpdateBitmap() is enforcing UI thread execution.

@adrianknight89

adrianknight89 Nov 27, 2016

Contributor

The problem with that approach is that converting vectors to bitmaps is an expensive procedure especially on the argb8888 setting which allocates 4 bytes per pixel. If you had a list of vectors and you navigated to and from the page frequently, it's easy to end up with an OutOfMemoryError exception. Of course, one solution would be to cache the results. SetImageResource() seems to be doing everything including resizing out of the box. However, the downside is that decoding is done on the UI thread. If this creates a latency concern, then we'd need to consider caching one again.

This change is by no means an advanced solution. It's just providing basic support for vectors.

P.S. It looks like ImageRenderer's UpdateBitmap() is enforcing UI thread execution.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 27, 2016

Contributor

Added support for view cells and updated sample code to demonstrate the changes. Also improved upon the original changes so that they are not applied if the element is no longer available (due to awaiting). Moved variables around so that they are defined when we need them.

Also noticed ToolbarButton and ToolbarImageButton seem to be never used. Should they be removed?

Contributor

adrianknight89 commented Nov 27, 2016

Added support for view cells and updated sample code to demonstrate the changes. Also improved upon the original changes so that they are not applied if the element is no longer available (due to awaiting). Moved variables around so that they are defined when we need them.

Also noticed ToolbarButton and ToolbarImageButton seem to be never used. Should they be removed?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Feb 20, 2017

Member

Can you rebase please @adrianknight89 ? thanks

Member

rmarinho commented Feb 20, 2017

Can you rebase please @adrianknight89 ? thanks

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Feb 20, 2017

Contributor

Done.

Contributor

adrianknight89 commented Feb 20, 2017

Done.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Feb 20, 2017

Member

Sorry @adrianknight89 we made some changes on Docs and need to rebase all pr's with latest master so it builds ok and we can run tests, can you rebase again the latest commits from master.
Thanks.

Member

rmarinho commented Feb 20, 2017

Sorry @adrianknight89 we made some changes on Docs and need to rebase all pr's with latest master so it builds ok and we can run tests, can you rebase again the latest commits from master.
Thanks.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Feb 20, 2017

Contributor

Done.

Contributor

adrianknight89 commented Feb 20, 2017

Done.

@rmarinho rmarinho merged commit 3c34aff into xamarin:master Feb 22, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 352, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3743, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 35…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 350…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 352…
Details

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.0 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment