Conversation
|
|
||
| super.onDraw(canvas); | ||
| if(mGifMovie == null){ | ||
| super.onDraw(canvas); |
There was a problem hiding this comment.
Is it possible that not calling onDraw in this case would lead to some instability on different vendors? ImageView and View might do some low level calls to hardware to invoke redrawing.
There was a problem hiding this comment.
I would rather keep calls to super unless you are absolutely sure that it is safe to avoid those.
|
@przybylski thanks for the code review. |
| canvas.getWidth() / movie.width()); | ||
| } | ||
|
|
||
| return scale; |
There was a problem hiding this comment.
Don't be afraid of early return ;)
- onMeasure sets always width/height
AndyScherzinger
force-pushed
the
animatedGif
branch
from
144e89f to
82f9f33
Compare
| } | ||
|
|
||
| public void setGIFImage(OCFile file) { | ||
| try { |
There was a problem hiding this comment.
There is some inconsistency here, setGIFImage takes OCFile and setImageBitmap takes bitmap. Latter is fine but former not quite. Altho I am not sure what would be the best way to resolve it.
Maybe setGIFImageFromPath(String) ?
There was a problem hiding this comment.
I'll change it to setGIFImageFromStoragePath(String storagePath) if that is fine with you
There was a problem hiding this comment.
@przybylski @tobiasKaminsky DONE, see d00cf73 I also reformatted the (new) code and added some javaDoc
|
I'll test it now on my device... |
|
LGTM |
|
After a final test by @AndyScherzinger it LGTM |
|
@tobiasKaminsky I found one small issue which might not be related to the animated gif itself. When I view animated gifs that are way smaller than my display resolution I can see the image_fail.png in the background behind the gif itself. Any idea as to why that happens? try for example the following gif. which gets displayed like this: |
|
This is probably related with calling super.onDraw |
|
What I mean by that is because |
|
Hah! You are right! So I removed it. |
|
@AndyScherzinger you probably don't see a problem here but try to open any non gif image |
|
Haha :D it made this pull request as looks good to me even without telling it :D |
|
Ouch.. Can see anything anymore... |
|
The ideal implementation would so something like:
but the current code is good enough, just call |
|
Thanks @przybylski added what you described abe2b24
anyone fancying opening an issue (maybe plus PR) to work on the proposed solution by @przybylski ? |
|
@tobiasKaminsky It's yours to hit the merge button :) |
|
🎉 my first merge, thanks for this great feeling 👍 |
|
Well, you very much deserved it! 👍 |
3 participants
Add functionality to show animated gif files in slideshow (image preview)
rebased original PR done by @tobiasKaminsky so all kudos belongs to him.
Please review @tobiasKaminsky @przybylski