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

Allow null transformations? #910

Closed
jacobtabak opened this issue Feb 28, 2015 · 8 comments
Closed

Allow null transformations? #910

jacobtabak opened this issue Feb 28, 2015 · 8 comments

Comments

@jacobtabak
Copy link
Contributor

Would you merge a PR that allowed you to set a null transformation?

Currently, you'll encounter the following exception:

 java.lang.IllegalArgumentException: Transformation must not be null.

Our loading code originally looked something like this:

    picasso
        .load(uri)
        .into(target);

Now, we're allowing a subclasses to specify an optional transformation, like this:

    RequestCreator requestCreator = picasso.load(uri);
    Transformation transformation = getTransformation();
    if (transformation != null) {
      requestCreator.transform(transformation);
    }
    requestCreator.into(target);

It would be much nicer if we could instead do this:

    picasso
        .load(uri)
        .transform(getTransformation() /* can be null */)
        .into(target);
@jacobtabak
Copy link
Contributor Author

Nevermind, I didn't realize you could apply multiple transformations.

@JakeWharton
Copy link
Member

What about taking a Collection that could be empty?
On Feb 28, 2015 10:35 AM, "Jacob Tabak" notifications@github.com wrote:

Closed #910 #910.


Reply to this email directly or view it on GitHub
#910 (comment).

@jacobtabak
Copy link
Contributor Author

Yes, perhaps a setTransforms() or setTransformations() would be a bit nicer. Anything to avoid breaking the builder syntax.

@JakeWharton
Copy link
Member

We've had this problem as well. Want to PR?
On Feb 28, 2015 10:43 AM, "Jacob Tabak" notifications@github.com wrote:

Yes, perhaps a setTransforms() or setTransformations() would be a bit
nicer. Anything to avoid breaking the builder syntax.


Reply to this email directly or view it on GitHub
#910 (comment).

@jacobtabak
Copy link
Contributor Author

Sure, on it

@ruckus
Copy link

ruckus commented Mar 19, 2015

I, too, didnt want to break the builder syntax, so that means always needing to call Picasso.transform with a non-null argument. But I always wanted to conditionally apply the transformation.

I solved this by first creating a no-op transformation:

public class NoOpTransformation implements Transformation {
    @Override
    public Bitmap transform(Bitmap source) {
        return source;
    }

    @Override
    public String key() {
        return "noop";
    }
}

And then I can keep a local instance variable representing this transformation, otherwise I can re-set the instance variable to my "real" transformation as needed and invocation always look like:

/* somewhere else ... */
backgroundTransformation = new NoOpTransformation();

/* or elsewhere */
backgroundTransformation = new StrokeTransformation();

// later
            mPicasso
                    .load(mMetric.photoURL)
                    .noFade()
                    .transform(backgroundTransformation)
                    .placeholder(R.drawable.grid_gradient)
                    .fit()
                    .into(mViewHolder.background);

Are there any downsides to this?

@JakeWharton
Copy link
Member

Yes. An image loaded with that transformation and an image loaded without
it will have different memory cache keys. I would highly recommend changing
to using lists and potentially passing an empty list with the just-released
2.5.1 version.

On Thu, Mar 19, 2015 at 1:14 PM Cody Caughlan notifications@github.com
wrote:

I, too, didnt want to break the builder syntax, so that means always
needing to call Picasso.transform with a non-null argument. But I always
wanted to conditionally apply the transformation.

I solved this by first creating a no-op transformation:

public class NoOpTransformation implements Transformation {
@OverRide
public Bitmap transform(Bitmap source) {
return source;
}

@Override
public String key() {
    return "noop";
}

}

And then I can keep a local instance variable representing this
transformation, otherwise I can re-set the instance variable to my "real"
transformation as needed and invocation always look like:

/* somewhere else ... /
backgroundTransformation = new NoOpTransformation();
/
or elsewhere */
backgroundTransformation = new StrokeTransformation();
// later
mPicasso
.load(mMetric.photoURL)
.noFade()
.transform(backgroundTransformation)
.placeholder(R.drawable.grid_gradient)
.fit()
.into(mViewHolder.background);

Are there any downsides to this?


Reply to this email directly or view it on GitHub
#910 (comment).

@ruckus
Copy link

ruckus commented Mar 19, 2015

Gotcha! Thanks for the response. I'll implement 2.5.1 and lists now. Thanks again for a great library.

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

No branches or pull requests

3 participants