Skip to content

Allow onlyScaleDown() with fit() method #1600

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.

Sign up for GitHub

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

Open
kwangsu61 wants to merge 1 commit into
base: master
Choose a base branch
from
+20 −4
Open

Allow onlyScaleDown() with fit() method #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions picasso/src/main/java/com/squareup/picasso/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,9 +390,6 @@ public Builder clearCenterInside() {
* specified by {@link #resize(int, int)}.
*/
public Builder onlyScaleDown() {
if (targetHeight == 0 && targetWidth == 0) {
throw new IllegalStateException("onlyScaleDown can not be applied without resize");
}
onlyScaleDown = true;
return this;
}
Expand Down Expand Up @@ -500,6 +497,10 @@ public Request build() {
throw new IllegalStateException(
"Center inside requires calling resize with positive width and height.");
}
if (onlyScaleDown && (targetWidth == 0 && targetHeight == 0)) {
Copy link
Copy Markdown

@magneticflux- magneticflux- May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be if(deferred || (targetWidth == 0 && targetHeight == 0))?

Copy link
Copy Markdown
Collaborator

@JakeWharton JakeWharton May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this check should be moved back to the onlyScaleDown method.

Copy link
Copy Markdown

@magneticflux- magneticflux- May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakeWharton This method is in the Request class, which seems to be in line with the other checks such as for null lists, invalid properties, and center[Crop|Inside] checks.

Copy link
Copy Markdown

@magneticflux- magneticflux- May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, the Request class has no knowledge of fitting, which is done in the RequestCreator class. I'm going to make my own pull request that will hopefully fix these issues cleanly.

throw new IllegalStateException(
"onlyScaleDown requires calling resize with positive width and height.");
}
if (priority == null) {
priority = Priority.NORMAL;
}
Expand Down
17 changes: 16 additions & 1 deletion picasso/src/test/java/com/squareup/picasso/RequestCreatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,16 @@ public void intoImageViewWithSkipMemoryCachePolicy() {
verify(picasso, never()).quickMemoryCacheCheck(URI_KEY_1);
}

@Test
public void intoImageViewWithFitAndOnlyScaleDown() {
ImageView target = mockFitImageViewTarget(true);
when(target.getWidth()).thenReturn(100);
when(target.getHeight()).thenReturn(100);
new RequestCreator(picasso, URI_1, 0).fit().onlyScaleDown().into(target);
verify(picasso).enqueueAndSubmit(actionCaptor.capture());
assertThat(actionCaptor.getValue().getRequest().onlyScaleDown).isTrue();
}

@Test
public void intoImageViewWithFitAndResizeThrows() {
try {
Expand Down Expand Up @@ -544,7 +554,7 @@ public void intoRemoteViewsNotificationWithFitThrows() {
}

@Test
public void intoTargetNoResizeWithCenterInsideOrCenterCropThrows() {
public void intoTargetNoResizeWithCenterInsideOrCenterCropOrOnlyScaleDownThrows() {
Copy link
Copy Markdown

@magneticflux- magneticflux- May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also be named intoTargetNoResizeOrFitWithCenterInsideOrCenterCropOrOnlyScaleDownThrows.

try {
new RequestCreator(picasso, URI_1, 0).centerInside().into(mockTarget());
fail("Center inside with unknown width should throw exception.");
Expand All @@ -555,6 +565,11 @@ public void intoTargetNoResizeWithCenterInsideOrCenterCropThrows() {
fail("Center inside with unknown height should throw exception.");
} catch (IllegalStateException ignored) {
}
try {
new RequestCreator(picasso, URI_1, 0).onlyScaleDown().into(mockTarget());
fail("onlyScaleDown with unknown height should throw exception.");
Copy link
Copy Markdown

@magneticflux- magneticflux- May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this fail when .onlyScaleDown() is called without a call to .fit() or .resize(int, int)?

} catch (IllegalStateException ignored) {
}
}

@Test public void appWidgetActionWithDefaultPriority() throws Exception {
Expand Down