[Image View] Fix multiple contentPadding issues

[jpmcosta]

Solution

Closes #2063.

Should fix the following contentPadding issues:

• contentPadding getters returning outdated values when app:contentPaddingStart or app:contentPaddingEnd are defined in xml;
• contentPadding being applied twice, if setContentPadding or setContentPaddingRelative are called before the view is measured;
  • in this case, the shape mask is also affected to have padding even if only contentPadding is defined.

[google-cla]

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).

For more information, open the CLA check for this pull request.

[drchen]

This seems not right to update paddings before layout direction is resolved? The content padding values thus got might be wrong.

[jpmcosta]

It seems that, once padding is set in View, resetResolvedPadding() ensures that resolvePadding() will be called when appropriate. I also couldn't find any documentation stating that setPadding() or setPaddingRelative() should only be called when layout direction is resolved.

So, I would say that the previous workflow was unnecessary. However, I might be missing something and I'm sorry if that's the case.

I couldn't reproduce any issues while testing.

[drchen]

It's not about get/setPadding() but how we get content padding, which relies on View.getLayoutDirection() to decide which content padding should be used. If this call happens before the layout direction is resolved, there might be a problem.

[jpmcosta]

View.resolvePadding() should adjust the padding accordingly, once the layout direction is resolved.

Again, I might be missing something. Do you have an example, even if theoretical, that I can test?

[drchen]

View.resolvePadding() also makes use of View.getLayoutDirection() to decide if it's RTL, which, again, may not be resolved yet in the constructor.

[jpmcosta]

This is how I see it:

• View.resolvePadding() is usually not called in the constructor;
• In this case, it will be called, because we need to get the current padding values (e.g., calling super.getPaddingStart(), etc.);
• However, setting padding also calls View.resetResolvedPaddingInternal(), ensuring that View.resolvePadding() will be called again later, when View.getLayoutDirection() is properly resolved;
• In the meantime, even if View.getLayoutDirection() is not accurate, everything will be consistent.

So, I don't see any issues that don't already occur when setting/getting the regular padding.

[drchen]

So, imagine this scenario -

If you set paddingStart and contentPaddingRight at the same time. Before RTL is resolved, in the overridden setPaddingRelative(), contentPaddingRight will be added to paddingEnd instead of paddingStart - which is wrong, because it should be added to paddingStart instead.

[jpmcosta]

That's definitely an issue. Somehow I wasn't considering cases where one would mix absolute regular padding with relative content padding, or the other way around. 👍