Implemented CENTER_INSIDE support

[josemontiel]

Fixes #591

[TWiStErRob]

Pool may return null (at least in 3.0 it did)

[TWiStErRob]

I have one bigger concern (other than my small line comments):
Applying .centerInside() to a load may produce an unnecessarily large Bitmap. Imagine one's loading a 16x16px icon into a 512x512px space. This means that Glide's centerInside will create a 512px Bitmap, but there will be 240px padding inside where there's nothing. This is a waste of memory considering the ImageView would handle this for us if it's ScaleType is set to centerInside. So I'm inclined to say that the automatic transformation for ImageViews should be a little different: return the original Bitmap if it's smaller than the requested size, and fitCenter if it's bigger. But, the padded version is still viable in situations like toBytes() or asBitmap() with custom targets because there it is better to have a Bitmap that has the requested size than implementing centerInside somehow every single time. The best solution would be to parameterize all centerInside calls with a boolean padSmaller argument, so the user can/has to decide which logic to use.

[TWiStErRob]

I think this needs to upgrade to 8888 if there's a padding, otherwise we'll end up with a color that someone won't like (BLACK/WHITE are both wrong choices), it should be TRANSPARENT.

[josemontiel]

Thanks for the feedback @TWiStErRob. Right on it!

[sjudd]

I think you can disregard padding entirely, it's hard for me to imagine a scenario where you want extra transparency baked into the Bitmap.

I'd just check if the width and height are greater than the target, and if so, fitCenter, and otherwise just return the original Bitmap.

Transformations never promise that the Bitmap they return will exactly match the requested width and height.

Thanks for putting this up!

[TWiStErRob]

@sjudd what if the requested transformation doesn't match scaleType?

ImageView.scaleType = fitCenter
.centerInside() on load

I would still expect a small image to be centered in a middle of a bigger space.

[sjudd]

Oh I wouldn't, I'd expect the Bitmap to remain at the smallest possible size (to minimize memory usage), but for the Bitmap to be stretched by the view when it's drawn to match the requested scale type.

None of our other transformations try to override a conflicting scale type set on a view, I don't think we need to do so here either.

[TWiStErRob]

@sjudd ah, you're right; the ImageView wins; @josemontiel sorry about that.

[josemontiel]

@sjudd so just to be sure, it should check if the Bitmap's dimensions are larger than the target and call fitCenter, otherwise return the input as is?

[sjudd]

@josemontiel I believe so? I think that matches what ImageView does?

[TWiStErRob]

What do you both think about the Downsampler issue? NONE will prevent huge images from loading with centerInside.

[josemontiel]

That definitely would be a problem, perhaps the NONE strategy could be overridden in the Downsampler when the source's dimensions are larger than the target's?

[TWiStErRob]

Hmm, there's already a DownsamplerStrategy.CENTER_INSIDE but it doesn't match what we need, let's see what @sjudd says about this, he knows that part better. I think that current CENTER_INSIDE strategy should be called FIT_INSIDE.

[josemontiel]

Hmm interesting @TWiStErRob . It could be something like this:

private static class RealCenterInside extends DownsampleStrategy {
    @Override
    public float getScaleFactor(int sourceWidth, int sourceHeight, int requestedWidth,
        int requestedHeight) {

      if (sourceWidth > requestedWidth || sourceHeight > requestedHeight) {
        float widthPercentage = requestedWidth / (float) sourceWidth;
        float heightPercentage = requestedHeight / (float) sourceHeight;
        return Math.min(widthPercentage, heightPercentage);
      } else {
        return 1f;
      }
    }

    @Override
    public SampleSizeRounding getSampleSizeRounding(int sourceWidth, int sourceHeight,
        int requestedWidth, int requestedHeight) {
      return SampleSizeRounding.QUALITY;
    }
}

[sjudd]

Yeah I think you're going to need a new downsample strategy that's equivalent to CENTER_INSIDE but that doesn't upscale.

The names here are pretty poor, sorry for that. Is there some standard naming convention for these transformations? Alternative suggestions welcome. I'm not totally sure that FIT_INSIDE is much clearer than CENTER_INSIDE, though I agree CENTER_INSIDE is poor. If we rename CENTER_INSIDE to FIT_INSIDE, what do we call the equivalent strategy that doesn't downsample?

@josemontiel That looks right, but I think you could simplify by just returning Math.min(1f, CENTER_INSIDE.getScaleFactor(...))? We want the same transformation, except that we don't want to upscale?

[TWiStErRob]

@sjudd I'm not aware of any standards here, I created #730 with my suggestions/questions.

@josemontiel I think you should just go with your "Real" naming and we'll rename it by fixing #730
or call it DOWNSCALE_ONLY as I proposed.

[josemontiel]

@TWiStErRob I'll go with DownscaleOnly, makes sense.

[TWiStErRob]

gradlew :library:checkstyle failed because of long lines in DownsampleStrategy, use Format code in IDEA to wrap automatically. Can you please fix that and squash your commits into 1?

[TWiStErRob]

@sjudd this looks ready.

[sjudd]

LGTM, I'll merge internally as soon as I can and push out.

I'll probably rename DOWNSCALE_ONLY per our brief discussion on your doc @TWiStErRob since this isn't the only strategy that doesn't upscale.

[sjudd]

Thanks!