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

API suggestion - RequestOptions #2875

Open
MFlisar opened this issue Feb 5, 2018 · 2 comments
Open

API suggestion - RequestOptions #2875

MFlisar opened this issue Feb 5, 2018 · 2 comments

Comments

@MFlisar
Copy link

MFlisar commented Feb 5, 2018

Glide Version: 4.5

Integration libraries: -

Device/Android Version: LL

Issue details / Repro steps / Use case background:

Current RequestOptions differentiate between one and multiple transformations. Why? I see that internally the transformations are handled by a MultiTransformation if multiple transformations are used, but this could be totally hidden from the user I think. Additional, currently a unnecessary MultiTransformation object is created if only one transformation is passed to the transforms function

Suggestion

Why not change the API to look like following (combine transform and transforms):

public RequestOptions transform(@NonNull Transformation<Bitmap>... transformations) {
	if (transformations.lengh > 1) {
		return transform(new MultiTransformation<>(transformations), /*isRequired=*/ true);
	} else if (transformations.lengh == 1) {
		return transform(transformation, /*isRequired=*/ true);
	} else {
		return this;
	}
}

Usage before

This way as a user using an array (potentially empty) I could change following:

RequestOptions ro = new RequestOptions();
if (transformations != null && transformations.size() > 0) {
	if (transformations .size() == 1) {
		ro = ro.transform(transformations .get(0));
	} else {
		ro = ro.transforms(transformations .toArray(new Transformation[transformations .size()]));
	}
}

Usage after

RequestOptions ro = new RequestOptions();
if (transformations != null) {
	ro = ro.transform(transformations .toArray(new Transformation[transformations .size()]));
}
@sjudd
Copy link
Collaborator

sjudd commented Feb 5, 2018

Using varargs allocates an extra array, but you can solve that with an additional overload.

If you want to send a pull request it should:

  1. Deprecate transforms
  2. Add a varargs version of transform

You won't need to do any bounds checking because the single argument method overload will be used if only one item is passed in anyway.

@TacoTheDank
Copy link
Contributor

I think this issue was meant to be closed with the merge...

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

3 participants