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

[Android] Allow path-based icons to be loaded as toolbar icons #437

Merged
merged 10 commits into from Nov 3, 2016

Conversation

Projects
None yet
8 participants
@adrianknight89
Contributor

adrianknight89 commented Oct 10, 2016

Description of Change

Currently, AppCompat does not let you load toolbar icons from the local file system. This PR adds code so that drawables will be searched as resources first and then as files if they can't be found.

Bugs Fixed

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR (no but MacOS stuff shouldn't be conflicting)
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Oct 20, 2016

Member

Looks good needs a performance review.

Member

jassmith commented Oct 20, 2016

Looks good needs a performance review.

@Emasoft

This comment has been minimized.

Show comment
Hide comment
@Emasoft

Emasoft Oct 24, 2016

Please approve this. It is a very urgent fix.

Emasoft commented Oct 24, 2016

Please approve this. It is a very urgent fix.

@samhouts

I see no appreciable performance hit. Code looks good. I'd like to see a test page showing the bug this fixes, if possible.

@Emasoft

This comment has been minimized.

Show comment
Hide comment
@Emasoft

Emasoft Oct 27, 2016

What are we waiting for to merge this? Please, we need this fix to be included in the next XF release.

Emasoft commented Oct 27, 2016

What are we waiting for to merge this? Please, we need this fix to be included in the next XF release.

@samhouts

This comment has been minimized.

Show comment
Hide comment
@samhouts

samhouts Oct 28, 2016

Member

@Emasoft It's been approved and will more than likely be merged before the next release. I'm giving @adrianknight89 an opportunity to add a test page for it if he has time. :)

Member

samhouts commented Oct 28, 2016

@Emasoft It's been approved and will more than likely be merged before the next release. I'm giving @adrianknight89 an opportunity to add a test page for it if he has time. :)

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 31, 2016

Contributor

Hi. I'm quite busy with work. How would the test for this work? Do you take a screenshot once the icon appears? Point to an example if you can so I can work off of it.

Contributor

adrianknight89 commented Oct 31, 2016

Hi. I'm quite busy with work. How would the test for this work? Do you take a screenshot once the icon appears? Point to an example if you can so I can work off of it.

@samhouts

This comment has been minimized.

Show comment
Hide comment
@samhouts

samhouts Oct 31, 2016

Member

@adrianknight89 This one can just be a test for manual review; omit the actual test part and just provide a page where we can visually inspect the icons. Whatever you used to test it yourself is probably sufficient. Thanks!

Member

samhouts commented Oct 31, 2016

@adrianknight89 This one can just be a test for manual review; omit the actual test part and just provide a page where we can visually inspect the icons. Whatever you used to test it yourself is probably sufficient. Thanks!

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 31, 2016

Contributor

@samhouts I used @Emasoft's sample which is quite complex for a test page.

For your use case, I used PCLStorage to save a byte array and read it back as a toolbar item icon with a local file path. I'm not sure if it's acceptable that a new library has been added, but I can't think of an easier way to interact with the file system.

Contributor

adrianknight89 commented Oct 31, 2016

@samhouts I used @Emasoft's sample which is quite complex for a test page.

For your use case, I used PCLStorage to save a byte array and read it back as a toolbar item icon with a local file path. I'm not sure if it's acceptable that a new library has been added, but I can't think of an easier way to interact with the file system.

@samhouts samhouts merged commit 84995a9 into xamarin:master Nov 3, 2016

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

For your use case, I used PCLStorage to save a byte array and read it back as a toolbar item icon with a local file path. I'm not sure if it's acceptable that a new library has been added, but I can't think of an easier way to interact with the file system.

it breaks the build...

Member

StephaneDelcroix commented Nov 9, 2016

For your use case, I used PCLStorage to save a byte array and read it back as a toolbar item icon with a local file path. I'm not sure if it's acceptable that a new library has been added, but I can't think of an easier way to interact with the file system.

it breaks the build...

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Nov 9, 2016

Member

@samhouts @adrianknight89 I had to remove the test case from master because it broke the build; I also removed the PCLStorage library references until the problems with the test case get resolved.

Member

hartez commented Nov 9, 2016

@samhouts @adrianknight89 I had to remove the test case from master because it broke the build; I also removed the PCLStorage library references until the problems with the test case get resolved.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 9, 2016

Contributor

@hartez Thanks for the update.

Contributor

adrianknight89 commented Nov 9, 2016

@hartez Thanks for the update.

@alexpetin

This comment has been minimized.

Show comment
Hide comment
@alexpetin

alexpetin Mar 23, 2017

This PR disallows to use any android XML drawables - selector, vector, etc!

alexpetin commented Mar 23, 2017

This PR disallows to use any android XML drawables - selector, vector, etc!

@samhouts samhouts added D15.4 and removed cla-already-signed labels Oct 10, 2017

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

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