-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Conversation
1fd763a
to
14c3634
Compare
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.
LGTM
new OnSuccessListener<StorageMetadata>() { | ||
@Override | ||
public void onSuccess(StorageMetadata storageMetadata) { | ||
HashMap<String, Object> map = new HashMap(); |
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.
Map<String, Object> map = new HashMap<>();
It is idiomatic Java to "code against interfaces", where a suitable one exists, like Map
or List
. This means all variables and parameters are given the interface type.
You get a compiler warning by leaving out the diamond operator <>
. Unfortunately, the class-level annotation @SuppressWarnings("unchecked")
suppresses all the help the compiler might provide. I suggest moving that annotation to the innermost level possible; it can be applied to individual methods or (better) to declarations of local variables.
Maybe you'll find that you don't need it at all.
.addOnFailureListener( | ||
new OnFailureListener() { | ||
@Override | ||
public void onFailure(@NonNull Exception e) { |
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.
If we are to use @NonNull
, we should do it consistently. I'd vote for not using nullability annotations at all, because they are not present in the Flutter APIs.
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.
It should to have the annotation since it is overriding a @nonnull parameter. Will add it where it's missing.
Map<String, String> arguments = (Map<String, String>) call.arguments; | ||
String filename = arguments.get("filename"); | ||
String path = arguments.get("path"); | ||
Map<String, Object> arguments = (Map<String, Object>) call.arguments; |
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.
Casting call.arguments
leads to unnecessary fights with the type checker. Just do
String filename = call.argument("filename");
String path = call.argument("path");
Map<String, Object> metadata = call.argument("metadata");
File file = new File(filename); | ||
StorageReference ref = firebaseStorage.getReference().child(path); | ||
UploadTask uploadTask = ref.putFile(Uri.fromFile(file)); | ||
UploadTask uploadTask; | ||
if (metadata != null) { |
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.
[nit] I'd invert this condition to avoid negatives where possible.
@@ -120,6 +166,16 @@ public void onFailure(Exception e) { | |||
}); | |||
} | |||
|
|||
private StorageMetadata buildMetadataFromMap(HashMap<String, Object> metadataMap) { |
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.
The parameter should be a Map
, not a HashMap
. I'd rename it map
.
@@ -29,9 +29,9 @@ class StorageReference { | |||
} | |||
|
|||
/// Asynchronously uploads a file to the currently specified StorageReference, without additional metadata. |
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.
The dart doc should be updated.
updatedTimeMillis = null, | ||
md5Hash = null; | ||
|
||
StorageMetadata._fromMap(Map<String, dynamic> map) |
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.
The argument needs to be Map<dynamic, dynamic>
because that is what you get on the channel.
'contentDisposition': contentDisposition, | ||
'contentLanguage': contentLanguage, | ||
'contentType': contentType, | ||
'contentEncoding': contentEncoding, |
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 ignore most of this information on the platform side. I find it a bit misleading to package up all of it here. Is toMap
used by anyone except StorageUploadTask
? If not, then perhaps it can be renamed toUploadMap
or the responsibility for populating a Map
with upload metadata can moved to StorageUploadTask
.
builder.setContentType((String) metadataMap.get("contentType")); | ||
return builder.build(); | ||
} | ||
|
||
private void getData(MethodCall call, final Result result) { | ||
Map<String, Object> arguments = (Map<String, Object>) call.arguments; |
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.
Same here.
Integer maxSize = call.argument("maxSize");
String path = call.argument("path");
@@ -42,7 +42,9 @@ class _MyHomePageState extends State<MyHomePage> { | |||
final String rand = "${new Random().nextInt(10000)}"; | |||
final StorageReference ref = | |||
FirebaseStorage.instance.ref().child("foo$rand.txt"); | |||
final StorageUploadTask uploadTask = ref.put(file); | |||
final StorageUploadTask uploadTask = ref.put( | |||
file, new StorageMetadata(contentLanguage: "en")); |
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.
Could be a const
constructor?
/// The time the [StorageReference] was created. | ||
final num creationTimeMillis; | ||
|
||
/// The time the [StorageReference] was last updated. |
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.
... in milliseconds since the epoch.
Should be an int
. Same for the other time stamp.
|
||
/// The content type of the [StorageReference]. | ||
String contentType; | ||
|
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.
Would be awesome to provide an example of each of these five settable pieces of metadata.
8d88921
to
cca9f76
Compare
69a2fa3
to
5994ed6
Compare
5994ed6
to
51cf620
Compare
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.
LGTM
'filename': file.absolute.path, | ||
'path': path, | ||
'metadata': metadata != null ? buildMetadataUploadMap(metadata) : null, |
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.
[nit] metadata == null ? null : buildMetadataUploadMap(metadata)
}, | ||
); | ||
_completer | ||
.complete(new UploadTaskSnapshot(downloadUrl: Uri.parse(downloadUrl))); | ||
} | ||
|
||
Map<String, dynamic> buildMetadataUploadMap(StorageMetadata metadata) { |
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.
Make private?
Add StorageMetadata class, getMetadata method on StorageReference, and optional metadata when uploading a file.