[Signin][Android] Implement Drawable class for AI tier status ring [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
triplo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/signin/android/config.gwsq)
Related details
- Tanmoy Mollik
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucia Giorgi (Gerrit)
Lucia Giorgi added 1 comment![]( "Open in Gerrit")
Hey @triplo...@google.com, if you need some context you can take a look at the DD linked in the bug but please feel free to reach out to me as well :)
Tanmoy Mollik (Gerrit)
Tanmoy Mollik added 6 comments![]( "Open in Gerrit")
I think these changes need to be reviewed by someone by the clank team. Not sure if this is the rigth ui pattern to use.
// The ring is drawn outside the bounds.Please fix this WARNING reported by autoreview issue finding: Drawing outside the `Drawable`'s bounds is highly prone to clipping. Because `getIntrinsicWidth()` and `getIntrinsicHeight()` only return the inner drawable's size, an `ImageView` using `wrap_content` will be sized exactly to the inner drawable, and the ring will likely be clipped by the view's boundaries.
It is safer to include the padding in the intrinsic size, and then inset the inner drawable here:
```java
@Override
public int getIntrinsicWidth() {
return mInnerDrawable.getIntrinsicWidth() + (mRingThicknessPx + mRingSpacingPx) * 2;
}
@Override
public int getIntrinsicHeight() {
return mInnerDrawable.getIntrinsicHeight() + (mRingThicknessPx + mRingSpacingPx) * 2;
}
@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);
int padding = mRingThicknessPx + mRingSpacingPx;
// Inset the bounds for the inner drawable
Rect innerBounds = new Rect(bounds);
innerBounds.inset(padding, padding);
mInnerDrawable.setBounds(innerBounds);
// Use the outer bounds for the ring
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
mPaint.setShader(null);Please fix this WARNING reported by autoreview issue finding: The javadoc in `SubscriptionTierBrandingDelegate` mentions returning `null` if no ring should be drawn. However, setting the shader to `null` here will cause `mPaint` to fall back to the default solid blue color (set in the constructor), and `drawArc` will still be called.
If the intention is to completely skip drawing the ring when `getRingShader` returns `null`, we should store a boolean flag to skip the `drawArc` call in `draw()`, or set the color to `Color.TRANSPARENT`.
android.content.Context context, @Nullable Drawable avatar, @Px int ringThicknessPx) {please import this.
public void testgetAvatarWithAiTierRing() {Please fix this WARNING reported by autoreview issue finding: Typo: should be `testGetAvatarWithAiTierRing` (capital `G`).
assertEquals(avatarDrawable.getIntrinsicWidth(), wrappedDrawable.getIntrinsicWidth());Please fix this WARNING reported by autoreview issue finding: If `AiTierRingDrawable` is updated to include the ring in its intrinsic size, remember to update these assertions to account for the extra padding.
Related details
- Lucia Giorgi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucia Giorgi (Gerrit)
Lucia Giorgi added 4 comments![]( "Open in Gerrit")
// The ring is drawn outside the bounds.Please fix this WARNING reported by autoreview issue finding: Drawing outside the `Drawable`'s bounds is highly prone to clipping. Because `getIntrinsicWidth()` and `getIntrinsicHeight()` only return the inner drawable's size, an `ImageView` using `wrap_content` will be sized exactly to the inner drawable, and the ring will likely be clipped by the view's boundaries.
It is safer to include the padding in the intrinsic size, and then inset the inner drawable here:
```java
@Override
public int getIntrinsicWidth() {
return mInnerDrawable.getIntrinsicWidth() + (mRingThicknessPx + mRingSpacingPx) * 2;
}@Override
public int getIntrinsicHeight() {
return mInnerDrawable.getIntrinsicHeight() + (mRingThicknessPx + mRingSpacingPx) * 2;
}@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);int padding = mRingThicknessPx + mRingSpacingPx;
// Inset the bounds for the inner drawable
Rect innerBounds = new Rect(bounds);
innerBounds.inset(padding, padding);
mInnerDrawable.setBounds(innerBounds);// Use the outer bounds for the ring
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
I went with this solution because otherwise I would've needed to add logic to `OptionalButtonView` to accommodate for the bigger footprint of avatar + circle, and I was advised to avoid changing that class because it's widely used. Menghan approved my solution of drawing outside the bounds. Also, here no clipping occurs, and when this feature will be extended to `AccountManagementFragment` and `CentralAccountCardPreference` the fix for the clipping is really easy (literally two lines in the xml file to tell it not to clip it).
Please fix this WARNING reported by autoreview issue finding: The javadoc in `SubscriptionTierBrandingDelegate` mentions returning `null` if no ring should be drawn. However, setting the shader to `null` here will cause `mPaint` to fall back to the default solid blue color (set in the constructor), and `drawArc` will still be called.
If the intention is to completely skip drawing the ring when `getRingShader` returns `null`, we should store a boolean flag to skip the `drawArc` call in `draw()`, or set the color to `Color.TRANSPARENT`.
Right, I forgot to update the javadoc. The intended behavior is to draw a simple blue circle.
android.content.Context context, @Nullable Drawable avatar, @Px int ringThicknessPx) {Lucia Giorgiplease import this.
right, thanks!
Please fix this WARNING reported by autoreview issue finding: Typo: should be `testGetAvatarWithAiTierRing` (capital `G`).
- Tanmoy Mollik
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tanmoy Mollik (Gerrit)
Tanmoy Mollik voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tanmoy Mollik
| Code-Review | +1 |
3 comments
overall it looks okay to me. But I am not an owner and you should ask someone from the clank team to take a look as well.
// The ring is drawn outside the bounds.Please fix this WARNING reported by autoreview issue finding: Drawing outside the `Drawable`'s bounds is highly prone to clipping. Because `getIntrinsicWidth()` and `getIntrinsicHeight()` only return the inner drawable's size, an `ImageView` using `wrap_content` will be sized exactly to the inner drawable, and the ring will likely be clipped by the view's boundaries.
It is safer to include the padding in the intrinsic size, and then inset the inner drawable here:
```java
@Override
public int getIntrinsicWidth() {
return mInnerDrawable.getIntrinsicWidth() + (mRingThicknessPx + mRingSpacingPx) * 2;
}@Override
public int getIntrinsicHeight() {
return mInnerDrawable.getIntrinsicHeight() + (mRingThicknessPx + mRingSpacingPx) * 2;
}@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);int padding = mRingThicknessPx + mRingSpacingPx;
// Inset the bounds for the inner drawable
Rect innerBounds = new Rect(bounds);
innerBounds.inset(padding, padding);
mInnerDrawable.setBounds(innerBounds);// Use the outer bounds for the ring
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
I went with this solution because otherwise I would've needed to add logic to `OptionalButtonView` to accommodate for the bigger footprint of avatar + circle, and I was advised to avoid changing that class because it's widely used. Menghan approved my solution of drawing outside the bounds. Also, here no clipping occurs, and when this feature will be extended to `AccountManagementFragment` and `CentralAccountCardPreference` the fix for the clipping is really easy (literally two lines in the xml file to tell it not to clip it).
Acknowledged
assertEquals(avatarDrawable.getIntrinsicWidth(), wrappedDrawable.getIntrinsicWidth());Tanmoy MollikPlease fix this WARNING reported by autoreview issue finding: If `AiTierRingDrawable` is updated to include the ring in its intrinsic size, remember to update these assertions to account for the extra padding.
Acknowledged
Related details
- Lucia Giorgi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 7 comments![]( "Open in Gerrit")
Just FYI I have moved to a different team within Chrome, no longer with Clank, so response for Android reviews will be slow :)
package org.chromium.components.browser_ui.util;nit: Can this live in //components/browser_ui/widgets?
public AiTierRingDrawable(Context context, Drawable innerDrawable, @Px int ringThicknessPx) {nit: javadoc
mBrandingDelegate = ServiceLoaderUtil.maybeCreate(SubscriptionTierBrandingDelegate.class);nit: Can we pass this in from the ctor? It'd be nice to keep this as a pure drawable class without adding this server loader DEPS
canvas.drawArc(mBounds, 0, 360, false, mPaint);For drawing a full 360-degree circle, `canvas.drawOval(mBounds, mPaint)` is more concise and slightly more efficient than `drawArc`.
// The ring is drawn outside the bounds.This drawable draws the ring outside the `bounds` passed to `onBoundsChange`. Even for static images, this is problematic because most Views (like `ImageView`) will clip the drawing to their own bounds.
A better approach is to treat the `bounds` as the total area for both the avatar and the ring. You should inset the `mInnerDrawable` bounds so it fits inside the ring.
Example:
```java
@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);
int totalPadding = mRingThicknessPx + mRingSpacingPx;
// Inset the inner avatar so it fits inside the ring + spacing.
Rect innerBounds = new Rect(bounds);
innerBounds.inset(totalPadding, totalPadding);
mInnerDrawable.setBounds(innerBounds);
// Set ring bounds so the stroke stays within the allocated area.
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
return mInnerDrawable.getIntrinsicWidth();This should return the total width including the ring and the spacing. If it only returns the inner width, parent views using `wrap_content` will not allocate enough space for the ring.
```java
@Override
public int getIntrinsicWidth() {
int innerWidth = mInnerDrawable.getIntrinsicWidth();
if (innerWidth <= 0) return innerWidth;
return innerWidth + 2 * (mRingThicknessPx + mRingSpacingPx);
}
```
Related details
- Lucia Giorgi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucia Giorgi (Gerrit)
Lucia Giorgi added 2 comments![]( "Open in Gerrit")
Thank you @wen...@chromium.org for the review! Just one quick question before I start addressing your comments in the implementation
Please fix this WARNING reported by autoreview issue finding: Drawing outside the `Drawable`'s bounds is highly prone to clipping. Because `getIntrinsicWidth()` and `getIntrinsicHeight()` only return the inner drawable's size, an `ImageView` using `wrap_content` will be sized exactly to the inner drawable, and the ring will likely be clipped by the view's boundaries.
It is safer to include the padding in the intrinsic size, and then inset the inner drawable here:
```java
@Override
public int getIntrinsicWidth() {
return mInnerDrawable.getIntrinsicWidth() + (mRingThicknessPx + mRingSpacingPx) * 2;
}
@Override
public int getIntrinsicHeight() {
return mInnerDrawable.getIntrinsicHeight() + (mRingThicknessPx + mRingSpacingPx) * 2;
}
@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);
int padding = mRingThicknessPx + mRingSpacingPx;
// Inset the bounds for the inner drawable
Rect innerBounds = new Rect(bounds);
innerBounds.inset(padding, padding);
mInnerDrawable.setBounds(innerBounds);
// Use the outer bounds for the ring
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
Lucia Giorgi
Tanmoy MollikI went with this solution because otherwise I would've needed to add logic to `OptionalButtonView` to accommodate for the bigger footprint of avatar + circle, and I was advised to avoid changing that class because it's widely used. Menghan approved my solution of drawing outside the bounds. Also, here no clipping occurs, and when this feature will be extended to `AccountManagementFragment` and `CentralAccountCardPreference` the fix for the clipping is really easy (literally two lines in the xml file to tell it not to clip it).
Acknowledged
@wen...@chromium.org
This was the motivation behind drawing outside of the bounds since we don't want to shrink the avatar size. Can you confirm that I can make changes to `OptionalButtonView` if I go with your solution?
Related details
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
The current CL that this is implemented as a drawable makes me hesitate - it can be put into any view which the parent will have the ability to clip / scale the drawable within.
If wthis is a case exception for a specific view, it might make sense to do this exception at the view level. We have prior cases that need to draw outer rings that goes beyond the view bounds e.g. [OutlineViewHelper](https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/widget/OutlineOverlayHelper.java;l=54;bpv=0;bpt=1) but I don't know if this is directly applicable here.
Related details
- Lucia Giorgi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucia Giorgi (Gerrit)
Lucia Giorgi added 10 comments![]( "Open in Gerrit")
I've completely changed the approach to ensure no drawing occurs outside the bounds of the drawable.
To prevent the avatar image from being shrunk, I had to adjust the surrounding paddings in the UI instead. To do that, I added a boolean to `DisplayableProfileData` so the views know when to accommodate the extra spacing for the ring.
Wdyt?
/* hasAiTierRing= */ false);Setting to `false` for now, the proper logic will be added in a follow up CL.
int paddingBottom =added padding logic to accommodate for the bigger footprint of avatar + ring
package org.chromium.components.browser_ui.util;nit: Can this live in //components/browser_ui/widgets?
Taking a look at `components/browser_ui/widget/android/BUILD.gn`, we can see that `//components/browser_ui/widgets` [depends](https://source.chromium.org/chromium/chromium/src/+/main:components/browser_ui/widget/android/BUILD.gn;l=156;drc=e90d6417232c42a007ecb0f8ec1cd148351fe2c5) on `//components/browser_ui/util`.
This drawable is used by `AvatarGenerator` which is in `util`, so this would create a circular dependency.
public AiTierRingDrawable(Context context, Drawable innerDrawable, @Px int ringThicknessPx) {Lucia Giorginit: javadoc
Done
mBrandingDelegate = ServiceLoaderUtil.maybeCreate(SubscriptionTierBrandingDelegate.class);nit: Can we pass this in from the ctor? It'd be nice to keep this as a pure drawable class without adding this server loader DEPS
Sure, is it ok now?
For drawing a full 360-degree circle, `canvas.drawOval(mBounds, mPaint)` is more concise and slightly more efficient than `drawArc`.
oh thanks, done!
Ok, got it. I changed approach and now the ring is no longer drawn outside the bounds.
This drawable draws the ring outside the `bounds` passed to `onBoundsChange`. Even for static images, this is problematic because most Views (like `ImageView`) will clip the drawing to their own bounds.
A better approach is to treat the `bounds` as the total area for both the avatar and the ring. You should inset the `mInnerDrawable` bounds so it fits inside the ring.
Example:
```java
@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);
int totalPadding = mRingThicknessPx + mRingSpacingPx;
// Inset the inner avatar so it fits inside the ring + spacing.
Rect innerBounds = new Rect(bounds);
innerBounds.inset(totalPadding, totalPadding);
mInnerDrawable.setBounds(innerBounds);
// Set ring bounds so the stroke stays within the allocated area.
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
Lucia Giorgi
Thank you, I implemented this solution!
This should return the total width including the ring and the spacing. If it only returns the inner width, parent views using `wrap_content` will not allocate enough space for the ring.
```java
@Override
public int getIntrinsicWidth() {
int innerWidth = mInnerDrawable.getIntrinsicWidth();
if (innerWidth <= 0) return innerWidth;
return innerWidth + 2 * (mRingThicknessPx + mRingSpacingPx);
}
```
- Tanmoy Mollik
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 9 comments![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
9 comments
Thanks!
public Builder setHasAiTierRing(boolean hasAiTierRing) {Just thoughts: I wonder if it make sense to group this with `setIsIdentityDisk` so this is limited to avatar for now. Unsure if it will be useful for other buttons?
: buttonSpec.hasAiTierRing()nit: The indentation of using inline if else is quite bad... can we use if blocks to improve this?
mButton.setPaddingRelative(paddingStart, paddingTop, paddingEnd, paddingBottom);Can you please test if this padding changes the toolbar height / space between the buttons? I think they should be fine, but just want to make sure
package org.chromium.components.browser_ui.util;Lucia Giorginit: Can this live in //components/browser_ui/widgets?
Taking a look at `components/browser_ui/widget/android/BUILD.gn`, we can see that `//components/browser_ui/widgets` [depends](https://source.chromium.org/chromium/chromium/src/+/main:components/browser_ui/widget/android/BUILD.gn;l=156;drc=e90d6417232c42a007ecb0f8ec1cd148351fe2c5) on `//components/browser_ui/util`.
This drawable is used by `AvatarGenerator` which is in `util`, so this would create a circular dependency.
Make sense, thanks!
mBrandingDelegate = ServiceLoaderUtil.maybeCreate(SubscriptionTierBrandingDelegate.class);Lucia Giorginit: Can we pass this in from the ctor? It'd be nice to keep this as a pure drawable class without adding this server loader DEPS
Sure, is it ok now?
Thanks!
// The ring is drawn outside the bounds.Lucia GiorgiThis drawable draws the ring outside the `bounds` passed to `onBoundsChange`. Even for static images, this is problematic because most Views (like `ImageView`) will clip the drawing to their own bounds.
A better approach is to treat the `bounds` as the total area for both the avatar and the ring. You should inset the `mInnerDrawable` bounds so it fits inside the ring.
Example:
```java
@Override
protected void onBoundsChange(Rect bounds) {
super.onBoundsChange(bounds);
int totalPadding = mRingThicknessPx + mRingSpacingPx;
// Inset the inner avatar so it fits inside the ring + spacing.
Rect innerBounds = new Rect(bounds);
innerBounds.inset(totalPadding, totalPadding);
mInnerDrawable.setBounds(innerBounds);// Set ring bounds so the stroke stays within the allocated area.
mBounds.set(bounds);
float strokeInset = mRingThicknessPx / 2f;
mBounds.inset(strokeInset, strokeInset);
...
}
```
Thank you, I implemented this solution!
Acknowledged
Context context, @Nullable Drawable avatar, @Px int ringThicknessPx) {optional nit: The contract here is if we pass a null avatar you get a null return value - seems we can just avoid the @Nullable altogether by ensure the `avatar` is not null
* @return The Shader to use for drawing the ring, or null to fall back to the default solid
* blue color.nit: This is a implementation detail for AiTierRingDrawable, not this interface. Let's move it
Related details
- Lucia Giorgi
- Tanmoy Mollik
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucia Giorgi (Gerrit)
Lucia Giorgi added 5 comments![]( "Open in Gerrit")
public Builder setHasAiTierRing(boolean hasAiTierRing) {Just thoughts: I wonder if it make sense to group this with `setIsIdentityDisk` so this is limited to avatar for now. Unsure if it will be useful for other buttons?
Yeah, it makes sense as it won't be used for other buttons. I renamed the method to `setIdentityDiscConfig` and grouped both booleans together.
: buttonSpec.hasAiTierRing()nit: The indentation of using inline if else is quite bad... can we use if blocks to improve this?
Sure, is it better now? Or should I group them like this?
```
if (buttonSpec.hasErrorBadge()) {
paddingBottomRes = ...
paddingTopRes = ...
paddingStartRes = ...
} else if (buttonSpec.hasAiTierRing()) {
paddingBottomRes = ...
paddingTopRes = ...
paddingStartRes = ...
} else {
paddingBottomRes = ...
paddingTopRes = ...
paddingStartRes = ...
}
```
mButton.setPaddingRelative(paddingStart, paddingTop, paddingEnd, paddingBottom);Can you please test if this padding changes the toolbar height / space between the buttons? I think they should be fine, but just want to make sure
Sure, I double checked and everything looks fine!
Context context, @Nullable Drawable avatar, @Px int ringThicknessPx) {optional nit: The contract here is if we pass a null avatar you get a null return value - seems we can just avoid the @Nullable altogether by ensure the `avatar` is not null
Thank you for pointing that out, I removed the `@Nullable`!
* @return The Shader to use for drawing the ring, or null to fall back to the default solid
* blue color.nit: This is a implementation detail for AiTierRingDrawable, not this interface. Let's move it
- Tanmoy Mollik
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |