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

Update StorageUI integration with SDWebImage #841

Merged
merged 6 commits into from
Mar 9, 2020

Conversation

dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Mar 7, 2020

Hey there! So you want to contribute to FirebaseUI? Before you file this pull request, follow these steps:

  • Read the contribution guidelines.
  • If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
  • If this is a new feature please co-ordinate with someone on FirebaseUI-Android to make sure that we can implement this on both platforms and maintain feature parity.

This PR add the following features:

  1. Use SDWebImage 5.6.0 's loader context option, instead of dummy manager instance. This is easier to use. And can avoid user who overrde the default manager's property (like transformer), which cause the undefined behavior when using with FirebaseUI Storage
  2. Add the support to load gs:// URl directly, not always need to create a wrapper FIRStorageReference.
  3. Add the support to progressive downloading and decoding of images, this can be the use case who want to load partial images and display for users.

The dependency of SDWebImage bumped to 5.6.0, beucase of the PR feature 1 need the new API.

CC @morganchen12

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

Thanks for updating the README!

@morganchen12 morganchen12 merged commit 75b5ebe into firebase:master Mar 9, 2020
@dreampiggy
Copy link
Contributor Author

dreampiggy commented Mar 10, 2020

@morganchen12 Hi.

After I update the demo to try the code, I found the strange exception thrown from Storage. I use the URI gs://hackemist-sdwebimage/joy.gif

image

However, the official documentation told me it's OK to pass the Full URI into the storageWithURL:

image

https://firebase.google.com/docs/storage/ios/create-reference

Which is correct ? Is this exception the designed behavior ? And whatever, this need to be fixed (I need another PR)

@dreampiggy
Copy link
Contributor Author

dreampiggy commented Mar 10, 2020

If I can not init a full URI into the FIRStorageReference directly, what can I do now ? Should I parse the URL path and use this arg to call child: ?


Found solution, currerntly I use the gs:bucket/object pattern to construct the FIRStorageReference, it works for multiple storage bucket. But looks a little tricky (isn't here any other way ?)

image

Tetsted two bucket's URI, works fine. It's public bucket so you can test it as well.

  • gs://hackemist-sdwebimage/joy.gif
  • gs://lizhuoli941126.appspot.com/joy.gif

image

@morganchen12
Copy link
Contributor

@dreampiggy the reason the crash happens is because you're trying to initialize a bucket with a full path to a specific file. The bucket is just the host (as you've found out).

I'm not sure why we don't allow creating a reference without first initializing the bucket.

@dreampiggy
Copy link
Contributor Author

dreampiggy commented Mar 11, 2020

However, the official documentation told me it's OK to pass the Full URI into the storageWithURL:

@morganchen12 So you means this is the correct behavior but not bug, that it's not allowed to init a full gs URI into StorageReference ? But seems we need to update the documentation there. It cause misunderstanding, because actually, you can not write code like this in document:

// This is equivalent to creating the full reference
let storagePath = "\(your_firebase_storage_bucket)/images/space.jpg"
spaceRef = storage.reference(forURL: storagePath)    

This will cause crash on exception, and worse, Swift can not even handle Objective-C's NSException, dangerous.


PS: Actuallly I think it's really convenient to construct a full gs URI into StorageReference, not a bug. What reason cause the Core Firebase iOS Team make decision to disable this usage ?

@dreampiggy
Copy link
Contributor Author

Seems the code comes from the Day 1 of FirebaseStorage 😭

https://github.com/firebase/firebase-ios-sdk/blame/master/FirebaseStorage/Sources/FIRStorage.m#L132

The documentation does not write correctly for that storageWithURL: behavior.

@morganchen12
Copy link
Contributor

I'll see what we can do to change the behavior of Firebase Storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants