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

Relax SizeDeterminer to handle only one dimension being WRAP_CONTENT #135

Closed
TWiStErRob opened this issue Sep 14, 2014 · 3 comments
Closed

Comments

@TWiStErRob
Copy link
Collaborator

This has been bugging me since I first tried Glide:

09-14 13:29:21.933 19179-19179/net.twisterrob.glidesongs W/ViewTarget﹕ Trying to load image into ImageView using WRAP_CONTENT, defaulting to screen dimensions: [1080x1920]. Give the view an actual width and height for better performance.

First:
"Trying to load image into ImageView using..." should be "Trying to load image into " + view.getClass() + " using..." since this is the ViewTarget class

Second:
I'm really against hard-coding any numbers in the UI code, or if I have to I'm trying to do it really flexibly.
Would it be possible to relax ViewTarget.isUsingWrapContent by replacing that or with an and? I give the point to you when both are WRAP_CONTENT there's nothing you can do to downsample. But if only one of them is WRAP_CONTENT the other one can still be fixed or MATCH_PARENT in which case one dimension is known and the aspect ratio is clear from the loaded Bitmap.

Say I want to load a 100x96 image into (list_item.xml):

<RelativeLayout
    android:layout_width="match_parent"
    android:layout_height="48dp"
    >
    <ImageView
        android:id="@+id/icon"
        android:layout_width="wrap_content"
        android:layout_height="fill_parent"
        android:adjustViewBounds="true"
        android:padding="@dimen/margin"
        />

in this case it's clear that you can downsample to: 50x48 regardless wrap_content being there and SizeDeterminer could still read one dimension when rendered, the other one could be -1 or 0?
This should be a biggie If it's well documented in com.bumptech.glide.load.ResourceDecoder#decode that the sizes are not necessarily always valid numbers and try to load with keeping aspect ratio anyway.

Also it may worth to take padding into account when isViewSizeValid.

@sjudd
Copy link
Collaborator

sjudd commented Sep 14, 2014

1 - I totally agree, should use the view class rather than ImageView.

2 - I agree in principle, my main objection is that passing along wrap_content as a width or height would require not just the decoder, but also every transformation and transcoder to add a special case (or cases) for wrap content.

For fit center and similar transformations that don't change the aspect ratio of the image, wrap_content can make sense, but for others that don't maintain the aspect ratio, like most kinds of crops, it makes less sense. What happens if you try to center crop the image in the example you gave?

One option I suppose would be allowing transformations to simply throw if they can't handle wrap_content, but since users of the library can define their own transformations, it might be hard to make that consistent.

I'm not at all against what you propose, I just struggle to come up with a neat way to handle these cases that would actually produce a result a user would expect.

3 - Padding is an interesting point, we probably should take that into account when determining the size of the image to load.

@TWiStErRob
Copy link
Collaborator Author

It looks like this works as I tried in #835. However the handling of wrap_content still needs some special care: I think to not need those workarounds SizeDeterminer could check if exactly one of the dimensions is wrap_content and if it is then consider getWidth/getHeight on that view invalid, leading to Glide waiting a further layout. Because the image will be cleared before starting the load the layout will happen and wrap will end up wrapping nothing resulting in 0 width, which looks like it's handled properly somewhere already (see imageView.layout(0,0,0,0) in #835). The problematic part may be if there's a placeholder, in which case waiting for layout still results in a size that is not really "valid" for the image Glide's about to load. So there are more cases to consider.

@sjudd
Copy link
Collaborator

sjudd commented Nov 12, 2017

I think we've addressed this to the best of my ability in #2431.

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

No branches or pull requests

2 participants