-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Version 0.6.0, FirebaseUI Storage #324
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
Conversation
37e6c89
to
5f075ca
Compare
Not sure who should review this, likely @puf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few general comments, but otherwise LGTM.
setContentView(R.layout.activity_image); | ||
ButterKnife.bind(this); | ||
|
||
// Authenticate so that uploading/downloading works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the other samples work or do we just set rules? I'd consider making auth global (across all sample features--though I guess it doesn't make sense for auth ;), or just getting devs to change their rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since authenticated downloads is a feature of the lib I'd like to keep auth, but I will add a comment here to explain and log some better resolution steps.
if (!task.isSuccessful()) { | ||
Log.w(TAG, "signInAnonymously", task.getException()); | ||
Toast.makeText(ImageActivity.this, | ||
"Authentication failed, uploads and downloads will not work.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include a resolution step here: enable auth or change your rules (or something strange happened)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added log message with detailed instructions.
|
||
## Using FirebaseUI to download and display images | ||
|
||
FirebaseUI provides bindings to download an image file from a `StorageReference` and display it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include links to StorageReference and Glide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also mention what Storage is with a brief blurb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
FirebaseUI provides bindings to download an image file from a `StorageReference` and display it | ||
using the popular `Glide` library. This technique allows you to get all of Glide's performance | ||
benefits while leveraging Firebase Storage's authenticated hosting capabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"authenticated storage..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
.into(imageView); | ||
``` | ||
|
||
Images displayed using `FirebaseImageLoader` are cached by their path in Firebase Storage, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disk or memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever the user sets. Adding link.
|
||
compile 'com.github.bumptech.glide:glide:3.7.0' | ||
|
||
compile "com.google.firebase:firebase-auth:${project.ext.firebase_version}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it actually needs auth, right? It can work without authentication if rules are set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch.
@Override | ||
public InputStream loadData(Priority priority) throws Exception { | ||
mStreamTask = mRef.getStream(); | ||
mInputStream = Tasks.await(mStreamTask).getStream(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how are errors propagated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I call .await()
that will throw an Exception
if something goes wrong. That will be handled by Glide, and the user can attach failure listeners to Glide if they want to know about this.
Implements
FirebaseImageLoader
and bumps overall version to0.6.0
.