[TIMOB-15747] Use CENTER_CROP to scale and fit the image

[salachi]

https://jira.appcelerator.org/browse/TIMOB-15747

[pingwang2011]

This will introduce many regressions, such as this test case https://jira.appcelerator.org/browse/TIMOB-15747?focusedCommentId=282170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-282170. Please also take a look at https://jira.appcelerator.org/browse/TIMOB-10358 for the details of the ImageView parity for scaling behavior.

[hieupham007]

Any update on this?

[hieupham007]

I've tested this. https://jira.appcelerator.org/browse/TIMOB-15747?focusedCommentId=282170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-282170 is a valid regression. Please run this test case on with and without your PR, and you will see the scaling difference. With your PR, the image takes up the whole screen - i.e ignoring aspect ratio.

[salachi]

I am little confused about the behaviour. What I see is the image being scaled to fill the height and width is chopped keeping the aspect ratio. As per https://jira.appcelerator.org/browse/TIMOB-10358, it looks like the IOS behaviour is to scale the image keeping the aspect ratio if either height or width is size behaviour which is the case here.

[hieupham007]

I'm just doing a comparison before and after on Android only, as I haven't tested with iOS. There is a behavior difference with and without this PR. I.e: Before when you run https://jira.appcelerator.org/browse/TIMOB-15747?focusedCommentId=282170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-282170 you would see the image in the middle, but after it scaled to take the whole imageview. Are you saying that the behavior before was incorrect?

[salachi]

Let me check in IOS and understand the behaviour.

[ingo]

@salachi Any updates here?

[salachi]

Ingo, I am not very clear on the behaviour. It will really help if Vishal or Hieu can define the behaviour.

[salachi]

New fix added. Scale the imageview based on the image aspect ration if width and height is not defined.

[salachi]

Merged with latest code from master

[mokesmokes]

Any updates? This PR is here for ages....

[ingo]

I've moved it into the next sprint for re-review.

[pingwang2011]

Why do you treat this as a special case but ignore the similar condition in line 391? Or why not add
if (!viewHeightDefinedLocal)
in line 391 too?

[salachi]

It was a miss on my part, I should have done the same checking.

[pingwang2011]

Please take a look at my last comment. It seems it's not necessary to do these manipulations here.

[salachi]

I debugged it and just changing the line 374 seems to be working as viewWidthDefined is false when left and right are set. Also, the special condition you mentioned above in the code is not required for both height and width as the condition will be always true.

[salachi]

Sorry, what I meant was doesn't seems to be working.

[pingwang2011]

Actually, the original ticket is a duplicate of TIMOB-14395. The issue is due to a bug in the fix of TIMOB-14395 (#4738). The fix should be

1. changing line 374 to:
   aspectRatio = (1f * ih) / iw;
2. when left and right are both defined, it is equivalent to width is defined. In this case, viewWidthDefined should be true. Same as top and bottom.

[salachi]

Changing the line 374 doesn't seems to be working as viewWidthDefined is false when left and right are set. Also, the special condition I had in the code (that you mentioned above) is not required for both height and width as the condition will be always true.

[pingwang2011]

The two steps in my last comment together will fix the issue. If you only tried step 1, it would not work. For step 2, you can do a similar check for left and right and decide how to set viewWidthDefined, just like https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUIImageView.java#L761

[salachi]

The original issue mentioned in the bug is fixed with this changes but the one Hieu mentioned above with sample https://jira.appcelerator.org/browse/TIMOB-15747?focusedCommentId=282170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-282170 is scaling the image to the full screen which I think is the right behaviour but old behaviour was not to scale to full width. Since both left and right is defined, and height is 100%, I think it should scale to fill the screen and that is the behaviour now. Can you confirm if this is the right behaviour? I am checking in the changes anyway.

[pingwang2011]

Yes. That's the right behavior. For the second sample, your very first fix cropped the image in width which is not correct. The correct behavior is:

1. Specifying either a width or height property for this view will scale its image(s) with the aspect ratio maintained, up to a maximum size that does not exceed its parent view.
2. Specifying both width and height, the image will be scaled to fill the width and height without maintaining the aspect ratio.

Code reviewed and functionally tested. Accepted