Add a shortcut to apply multiple transformations on a single request. #2138

realdadfish · 2017-07-13T23:11:16Z

Description

As per discussion here:
#2086

Motivation and Context

Makes the possibility to apply multiple transformations easier discoverable via API.

I'd like to have broaden the test context a bit more, but this would have meant to introduce @VisibleForTesting getters to internals in a couple of places which I neglected to do for now.

googlebot · 2017-07-13T23:11:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

realdadfish · 2017-07-13T23:14:00Z

Signed the CLA.

googlebot · 2017-07-13T23:14:02Z

CLAs look good, thanks!

TWiStErRob

I would advise to revert the changes that are automatically made by the IDE (except the ill-indented test annotation fix). Please also check Travis for checkstyle failure and fix that.
Double-check that your formatter is coming from the checked out sources. Some people ignore .idea folder in the global config, it can confuse things really bad.

TWiStErRob · 2017-07-13T23:40:56Z

library/src/main/java/com/bumptech/glide/request/RequestOptions.java

+   * @see #optionalTransform(Transformation)
+   * @see #optionalTransform(Class, Transformation)
+   */
+  public RequestOptions multiTransform(@NonNull Transformation<Bitmap>... transformations) {


I think transforms would be a better name (alphabetically closer to transform). Is there any particular reason why not just use an overload to the existing transform method and introduce a new name?

Well, it closer resembled the name of the class that actually does the transformation, but I have no strong obligations there, so I will rename it.

TWiStErRob · 2017-07-13T23:42:19Z

library/src/main/java/com/bumptech/glide/request/RequestOptions.java

-          .fitCenter()
-          .autoClone();
+              .fitCenter()
+              .autoClone();


Looks like you may have a different formatter that the project. Glide uses 2-space indents and continuation is usually double indent, so 2*2=4, 8 is weird in this code base.

TWiStErRob · 2017-07-13T23:42:47Z

library/src/main/java/com/bumptech/glide/request/RequestOptions.java

@@ -804,7 +805,7 @@ public RequestOptions centerInside() {

  /**
   * Applies {@link CircleCrop} to all default types, and ignores unknown types.
-   *
+   * <p>


Paragraph is already there in next line, these shouldn't be added.

sjudd · 2017-07-14T01:20:46Z

Thanks for doing this!

If it helps Glide uses the google style guide: https://google.github.io/styleguide/javaguide.html

I believe you can apply the style guide in intellij/android studio using this: https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml.

If I can remember exactly how I did it, I'll update our contributing page on the docs site and link that here.

sjudd · 2017-07-14T01:40:03Z

Added steps to import the style guide into Android Studio here (it should update shortly): http://bumptech.github.io/glide/dev/contributing.html

googlebot · 2017-07-17T11:21:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

realdadfish · 2017-07-17T11:23:13Z

Reformatted and cleaned.

sjudd

Also double check that the email address in your commit matches the email address you registered the CLA for, it looks like they don't match right now.

sjudd · 2017-07-21T14:16:54Z

library/src/test/java/com/bumptech/glide/request/RequestOptionsTest.java

@@ -1,9 +1,12 @@
 package com.bumptech.glide.request;

-import static com.google.common.truth.Truth.assertThat;


You'll need to revert this import removal.

See bumptech#2086

googlebot · 2017-08-03T16:31:17Z

CLAs look good, thanks!

realdadfish · 2017-08-03T16:31:46Z

Sorry, was on vacation, that's why it took a bit longer.

sjudd · 2017-08-04T15:43:44Z

Great thanks for doing this, I appreciate it!

TWiStErRob requested changes Jul 13, 2017

View reviewed changes

realdadfish force-pushed the multi-transform-shortcut branch from fdb89e2 to ded9ac6 Compare July 17, 2017 11:21

realdadfish force-pushed the multi-transform-shortcut branch from ded9ac6 to 7c61baf Compare July 19, 2017 22:51

sjudd requested changes Jul 21, 2017

View reviewed changes

Add a shortcut to apply multiple transformations on a single request.

7743db7

See bumptech#2086

realdadfish force-pushed the multi-transform-shortcut branch from 7c61baf to 7743db7 Compare August 3, 2017 16:31

sjudd approved these changes Aug 4, 2017

View reviewed changes

sjudd merged commit 00886e7 into bumptech:master Aug 4, 2017

realdadfish deleted the multi-transform-shortcut branch August 8, 2017 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a shortcut to apply multiple transformations on a single request. #2138

Add a shortcut to apply multiple transformations on a single request. #2138

realdadfish commented Jul 13, 2017

googlebot commented Jul 13, 2017

realdadfish commented Jul 13, 2017

googlebot commented Jul 13, 2017

TWiStErRob left a comment

TWiStErRob Jul 13, 2017

realdadfish Jul 17, 2017

TWiStErRob Jul 13, 2017

TWiStErRob Jul 13, 2017

sjudd commented Jul 14, 2017

sjudd commented Jul 14, 2017

googlebot commented Jul 17, 2017

realdadfish commented Jul 17, 2017

sjudd left a comment

sjudd Jul 21, 2017

googlebot commented Aug 3, 2017

realdadfish commented Aug 3, 2017

sjudd commented Aug 4, 2017

		@@ -1,9 +1,12 @@
		package com.bumptech.glide.request;

		import static com.google.common.truth.Truth.assertThat;

Add a shortcut to apply multiple transformations on a single request. #2138

Add a shortcut to apply multiple transformations on a single request. #2138

Conversation

realdadfish commented Jul 13, 2017

Description

Motivation and Context

googlebot commented Jul 13, 2017

realdadfish commented Jul 13, 2017

googlebot commented Jul 13, 2017

TWiStErRob left a comment

Choose a reason for hiding this comment

TWiStErRob Jul 13, 2017

Choose a reason for hiding this comment

realdadfish Jul 17, 2017

Choose a reason for hiding this comment

TWiStErRob Jul 13, 2017

Choose a reason for hiding this comment

TWiStErRob Jul 13, 2017

Choose a reason for hiding this comment

sjudd commented Jul 14, 2017

sjudd commented Jul 14, 2017

googlebot commented Jul 17, 2017

realdadfish commented Jul 17, 2017

sjudd left a comment

Choose a reason for hiding this comment

sjudd Jul 21, 2017

Choose a reason for hiding this comment

googlebot commented Aug 3, 2017

realdadfish commented Aug 3, 2017

sjudd commented Aug 4, 2017