Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] Implements ImageDecoders to decouple decoding from downloading. #1890
base: master
Are you sure you want to change the base?
[WIP] Implements ImageDecoders to decouple decoding from downloading. #1890
Changes from 10 commits
53948f0
4d686a2
a448949
bac7e19
fb2508f
b14c340
90e9044
77b57c3
a2f09cf
df5b56f
21aa3c8
c2fa402
9aa2877
7efb773
ac7cfe9
89081a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Feels like we should hand a peeked source here rather than let consumers worry about 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.
Fixed in 89081a3
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.
is it sufficient to check for xml header bytes?
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 library internally handles gzipped content here, so we'd have to move that logic out into this, and we'd also have to validate the SVG. It could be done but I think that should live in the SVG library.
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.
OkHttp should be transparently un-gzipping. Or are you worried about reading local files?
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.
That's a fair point. My point was simply that determining if the stream is an SVG is a little more complex that check that it (1) is an XML file and (2) has a DOCTYPE or something. Taking out the GZipInputStream case (which I guess is for http streams), the existing SVG library will validate the SVG as well as load it, which is something that makes sense in an SVG parser, as opposed to reimplementing here.
One example of the added complexity, from what I can see, the existing library checks the DOCTYPE, which might contain
<!ENTITY
in which case it falls back to a different parser. I think understanding the complexities around XML, and SVG in particular, make sense to be handled in the parser.Ideally the library had another method that would simply check if the stream is an SVG without parsing the whole thing, but this one doesn't.
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 wonder if we'd be better off with a design where
canHandleSource
returned either:Object
(or maybeT
) which is forwarded to this method. In this case it would be theSVG
so it doesn't need re-decoded. Returnnull
if you can't handle.Decoder
and this would change to aDecoder.Factory
. That way you could propagate as much information as you wanted. Returnnull
if you can't handle. I guess technically you can already do this with case Start actual website content. #1 by just using some data class as your type.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 that simplify things?
In the SVG case it would remove a parse, since we can't currently detect an SVG without parsing it, but I think that's sort of a special case. Bitmap, where we just decode the bounds to identify if it's parseable, doesn't have that limitation, and also nothing useful to return. In those cases I think this dead simple API is nice, though I'll admit I'm not entirely sure what case #2 would look like.
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 bounds are useful to propagate for Bitmap so we can avoid re-reading them to apply transformations during decode.
Case #2 would work like Retrofit Converter.Factory or CallAdapter.Factory or Moshi JsonAdapter.Factory. If you can handle, return a handler.
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.
Was looking for a Picasso related sample SVG to use, but couldn't find much. This should, perhaps, be replaced with something we can guarantee.
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.
This code is repeated in each RequestHandler and should probably be extracted into the abstract base class.
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.
mmm, i think this whole method should just always true. an I/O problem and all the other things are different from not being able to handle the request.
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 IOException is just because that's required to be handled, the main idea here is that the stream might not be something Bitmap factory can handle (an SVG, an animated GIF, a fat jpeg, a live photo), so this needs to test that it can decode a bitmap from the stream.
The fastest way I could think of, without needing to load the entire bitmap into memory, was but simply decoding the bounds.