[ReactToolbar] Fixed icon handling #5753
Conversation
|
By analyzing the blame information on this pull request, we identified @foghina and @crm416 to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
Thanks for the PR and the explanation @astuetz! Cherry-picks into the release branch are intended for small fixes of bugs discovered during the two weeks of RC. One exception are the new Android components as adding new components won't break other code and we special-case them to get them out as quickly as possible. While this one looks like a bugfix, it's a bit of large change to be released without testing in an RC. That said, the next RC is already next week and 0.21.0 goes out in two weeks - the releases are frequent enough so that there shouldn't be a hurry to get things into the release in general. Hope that explains it a bit. |
|
I'll let @foghina review this as he worked on the image loading code in the |
|
@mkonicek thanks, sounds good :) |
| .build(); | ||
| holder.setController(controller); | ||
| } else { | ||
| callback.setDrawable(getDrawableByName(uri)); |
There was a problem hiding this comment.
Couldn't we reuse IconControllerListener here? Then we could get rid of SetIconSourceCallback completely.
There was a problem hiding this comment.
Good point, if it's fine for you I will remove SetIconSource and rename IconControllerListener.onDrawableReady to IconControllerListener.setDrawable which sounds better for all usages then.
|
@astuetz updated the pull request. |
|
Looks legit, thanks for the PR! @facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1524731804522669/int_phab to review. |
ghost
closed this
in
|
Thanks a lot for the review @foghina! |
Summary: [This commit](facebook@e730a9f) (_Load assets from same folder as JSbundle (Android)_) causes React Native to look for assets inside the same folder the JSBundle was loaded from and generates asset URIs containing the absolute path to the asset (e.g. _/sdcard/bundle/drawable-xxhdpi/ic_back.png_). While this is fine for a normal `ImageView`, `ToolbarAndroid`/`ReactToolbar` currently crashes if the icons are located on the file system. This happens because when setting an icon on `ReactToolbar`, Fresco is only used if the icon URI contains `http:// `or `https://`. For all other cases (like in this case where it starts with `file://`), the view tries to load the Drawable from the Android App Resources by it's name (which in this case is an absolute file-URI) and therefore causes it to crash (`getDrawableResourceByName` returns 0 if the Drawable was not found, then `getResources().getDrawable(DrawableRes int id)` throws an Exception if th Closes facebook#5753 Reviewed By: svcscm Differential Revision: D2921418 Pulled By: foghina fb-gh-sync-id: 7a3f81b530a8c1530e98e7b592ee7e44c8f19df1 shipit-source-id: 7a3f81b530a8c1530e98e7b592ee7e44c8f19df1
This commit (Load assets from same folder as JSbundle (Android)) causes React Native to look for assets inside the same folder the JSBundle was loaded from and generates asset URIs containing the absolute path to the asset (e.g. /sdcard/bundle/drawable-xxhdpi/ic_back.png).
While this is fine for a normal
ImageView,ToolbarAndroid/ReactToolbarcurrently crashes if the icons are located on the file system. This happens because when setting an icon onReactToolbar, Fresco is only used if the icon URI containshttp://orhttps://. For all other cases (like in this case where it starts withfile://), the view tries to load the Drawable from the Android App Resources by it's name (which in this case is an absolute file-URI) and therefore causes it to crash (getDrawableResourceByNamereturns 0 if the Drawable was not found, thengetResources().getDrawable(@DrawableRes int id)throws an Exception if the Drawable was not found).Additionally, this PR also fixes scaling issues of Toolbar icons if they are located on the local filesystem or on a server (as the density-based scale factor / scaled size was completely ignored by
ReactToolbar)