actions revamp: Introduce animations to toolbar actions [chromium/src : main]
Masa Fujita (Gerrit)
Masa Fujita has uploaded the change for review![]( "Open in Gerrit")
Commit message
actions revamp: Introduce animations to toolbar actions
Change diff
Change information
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListCoordinator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListMediator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListRecyclerView.java
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 1 comment![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
1 comment
@skav...@google.com PTAL ToolbarTablet.java, ToolbarUtils.java
@clh...@google.com PTAL generally dynamic toolbar ranking impls
@n...@chromium.org PTAL overall
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charles Hager (Gerrit)
Charles Hager added 6 comments![]( "Open in Gerrit")
Overall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
* 10;Out of curiosity, why 10 times the length of a button / touch target?
// RecyclerView} do not have sliding animations and only have fading animations, but we wantDo the edge items get created but aren't initially visible, then slide into view during the animation?
if (mAnimating && mAvailableWidth != null && mAvailableWidth == availableWidth) {Just to check - is it ever possible for the action items to change during an animation? Or, it is safe to assume that if mAnimating == true, that the items can't change until after the animation is ended? Should we add an assert maybe to make sure changes never happen mid-animation?
// This method gets called due to our own animation too. We already make sure that theWording nit
```suggestion
// This method gets called due to our own animation too. We already made sure that the
```
reconcileActionItems(/* animate= */ false);How often do we expect this to be called mid-animation? Are there any performance considerations, any edge cases where this could cause lagging during animations?
Related details
- Masa Fujita
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita added 1 comment![]( "Open in Gerrit")
Overall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
Thanks for taking a look! The performance seems... fine at best? 🙃 It works well on my high end test device but I'm not sure what the performance hit is going to be on lower end devices. It seems to be not as bad as the tab switcher animation but not too sure.
Do you know how we can quantitatively test animation performances?
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 10 comments![]( "Open in Gerrit")
Thanks for the patch! Here's my first pass.
private Boolean mIsDragging = false;`boolean` if this is never null.
class ExtensionActionListMediator implements Destroyable {This class got more complicated to deal with anchors. I hope we can isolate the logic to handle anchors in a separate class so that this mediator can focus on a simple list of actions.
Rough idea:
- Create a ModelList that derives from another ModelList so that the former's items are the latter's items plus sandwiching anchors. The mediator focuses on updating the latter.
- As for handling `extension_action_anchor_width`, we can subtract it from `availableWidth` before passing it from the coordinator.
I know this can be an armchair argument, but I hope we can unentangle logic as far as possible...
private @Nullable Integer mAvailableWidth;Can you add a comment on this? Describing this variable precisely seems important.
// The anchor items are placed at both ends of the {@link RecyclerView} to act as theWow, nice job figuring this out!
if (mAvailableWidth != null) {In what cases `mAvailableWidth == null`?
Is it ever possible to change this to either:
- assertion: `assert mAvailableWidth != null`
- early return: `if (mAvailableWidth == null) return`
* @return Null if either of the anchors don't exist, which can happen during launch.It simplifies things if we can assume the anchors always exist. Can we synchronously bind anchor items on initialization?
That said, is it possible to write this function without relying on anchors? For example, by summing up widths of all children.
public class ExtensionActionListRecyclerView extends RecyclerView {Can we replace this with `RecyclerView` now? It can be a separate patch though.
return mContainer.getVisibility() == View.VISIBLE;qq: do we ever return false here? This code is fine, but I'm just curious.
return mExtensionActionListCoordinator.fitActionsWithinWidth(availableWidth);Don't we need to account for the puzzle piece button's width?
Related details
- Charles Hager
- Masa Fujita
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 14 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
Masa FujitaOverall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
Thanks for taking a look! The performance seems... fine at best? 🙃 It works well on my high end test device but I'm not sure what the performance hit is going to be on lower end devices. It seems to be not as bad as the tab switcher animation but not too sure.
Do you know how we can quantitatively test animation performances?
(unresolving)
private Boolean mIsDragging = false;`boolean` if this is never null.
Done
* 10;Out of curiosity, why 10 times the length of a button / touch target?
This is honestly not based on much. We need this padding because even though the entire `RecyclerView` should always theoretically remain visible, the animation can get janky and `RecyclerView` might decide to destroy the anchors' View instances.
class ExtensionActionListMediator implements Destroyable {This class got more complicated to deal with anchors. I hope we can isolate the logic to handle anchors in a separate class so that this mediator can focus on a simple list of actions.
Rough idea:
- Create a ModelList that derives from another ModelList so that the former's items are the latter's items plus sandwiching anchors. The mediator focuses on updating the latter.
- As for handling `extension_action_anchor_width`, we can subtract it from `availableWidth` before passing it from the coordinator.
I know this can be an armchair argument, but I hope we can unentangle logic as far as possible...
I ended up creating a wrapper class that automatically adds anchor items (and moved most of the animation related code to the coordinator).
private @Nullable Integer mAvailableWidth;Can you add a comment on this? Describing this variable precisely seems important.
Done
// RecyclerView} do not have sliding animations and only have fading animations, but we wantDo the edge items get created but aren't initially visible, then slide into view during the animation?
Without the anchors, if the edge items' visibilities change:
- the edge item at the start (far right (because `stackFromEnd = true`)) gets created outside the viewport and does not slide in automatically. We would have to programatically scroll to the top.
- the edge item at the end (far left) gets created with a "fade in" animation. No sliding animation for anyone, so nothing for the omnibox to track.
but, with the anchors, no icons are edge items, so everything should be always stay visible and within the bounds of the `RecyclerView`.
if (mAvailableWidth != null) {In what cases `mAvailableWidth == null`?
Is it ever possible to change this to either:
- assertion: `assert mAvailableWidth != null`
- early return: `if (mAvailableWidth == null) return`
I think early return makes sense here.
if (mAnimating && mAvailableWidth != null && mAvailableWidth == availableWidth) {Just to check - is it ever possible for the action items to change during an animation? Or, it is safe to assume that if mAnimating == true, that the items can't change until after the animation is ended? Should we add an assert maybe to make sure changes never happen mid-animation?
good catch - I made it so that if we do receive a change during animation, we abandon the ongoing animation and update to the new state instantly (we don't expect this to happen often).
// This method gets called due to our own animation too. We already make sure that theWording nit
```suggestion
// This method gets called due to our own animation too. We already made sure that the
```
Done
reconcileActionItems(/* animate= */ false);How often do we expect this to be called mid-animation? Are there any performance considerations, any edge cases where this could cause lagging during animations?
This should be very rare - this only happens when we get another `updateVisibility()` update (e.g. the user resizes window) during pinning / unpinning animation. In that case we abandon the ongoing animation and just update the UI instantly
* @return Null if either of the anchors don't exist, which can happen during launch.It simplifies things if we can assume the anchors always exist. Can we synchronously bind anchor items on initialization?
That said, is it possible to write this function without relying on anchors? For example, by summing up widths of all children.
It's very difficult to assert the lifetime of Views inside RecyclerViews. I think we can return 0 here if the anchors don't exist and avoid all the nullableness.
We could calculate the max and min occupied by the Views but it's a bit easier to rely on the anchors (some Views exist but are transparent, etc)
public class ExtensionActionListRecyclerView extends RecyclerView {Can we replace this with `RecyclerView` now? It can be a separate patch though.
Probably, but I'm still not 100% sure if we will need to customize the `RecyclerView`. If it turns out we won't then I'll make sure to remove this.
return mContainer.getVisibility() == View.VISIBLE;qq: do we ever return false here? This code is fine, but I'm just curious.
I think this always returns true (I stole this from crrev.com/c/7382037).
return mExtensionActionListCoordinator.fitActionsWithinWidth(availableWidth);Don't we need to account for the puzzle piece button's width?
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Charles Hager (Gerrit)
Charles Hager added 1 comment![]( "Open in Gerrit")
return mContainer.getVisibility() == View.VISIBLE;Masa Fujitaqq: do we ever return false here? This code is fine, but I'm just curious.
I think this always returns true (I stole this from crrev.com/c/7382037).
This is totally fine, this is currently only used to decide whether to display that icon / button at the top of the 3-button menu on narrow screens.
Related details
- Masa Fujita
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 7 comments![]( "Open in Gerrit")
* want the omnibox to slide.Nice explanation!
public static class ModelListWithAnchors extends ModelList {I think we don't need to make a derived class, but just can create a ModelList instance. Then we can avoid implementation inheritances.
e.g.
```
private static ModelList createModelListWithAnchors(ModelList sourceList) {
ModelList newList = new ModelList();
// ...Set up newList's contents...
sourceList.addObserver(new ListObserver<Void>() {
...
});
return newList;
}
``` private final ModelList mModels;You can remove `mModels` now.
// The available width that the {@link RecyclerView} can use up.Can you mention:
- When it is null?
- When it is updated?
/**This time, we got a bunch of RecyclerView-related logic in the coordinator. It's nice if you can isolate them into a separate class rather than embedding them in the coordinator... so that the coordinator just coordinates things.
for (int i = 1; i < mModelsWithAnchors.size() - 1; i++) {This logic can be simpler if we use `mModels`.
This might sound contradictory to my comment above, but there's chance this can work well after extracting the RecyclerView logic into a separate class.
if (mAvailableWidth == null) {I guess that, if we return early, it should be near the beginning of the function, not here? Otherwise we may make incomplete modifications to the models.
Also, can you add a comment when this condition is hit?
Related details
- Charles Hager
- Masa Fujita
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 7 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
@skav...@google.com PTAL ToolbarTablet.java, ToolbarUtils.java
@clh...@google.com PTAL generally dynamic toolbar ranking impls
@n...@chromium.org PTAL overall
@emil...@chromium.org PTAL overall (moved @n...@chromium.org to CC)
public static class ModelListWithAnchors extends ModelList {I think we don't need to make a derived class, but just can create a ModelList instance. Then we can avoid implementation inheritances.
e.g.
```
private static ModelList createModelListWithAnchors(ModelList sourceList) {
ModelList newList = new ModelList();// ...Set up newList's contents...sourceList.addObserver(new ListObserver<Void>() {
...
});
return newList;
}
```
I was about to do this but actually it turns out we have to override `move` too, so let me keep this.
You can remove `mModels` now.
Done
// The available width that the {@link RecyclerView} can use up.Can you mention:
- When it is null?
- When it is updated?
Done
This time, we got a bunch of RecyclerView-related logic in the coordinator. It's nice if you can isolate them into a separate class rather than embedding them in the coordinator... so that the coordinator just coordinates things.
Ended up moving animation related stuff to `ExtensionActionListRecyclerView` - I think it makes the most sense.
for (int i = 1; i < mModelsWithAnchors.size() - 1; i++) {This logic can be simpler if we use `mModels`.
This might sound contradictory to my comment above, but there's chance this can work well after extracting the RecyclerView logic into a separate class.
Done
I guess that, if we return early, it should be near the beginning of the function, not here? Otherwise we may make incomplete modifications to the models.
Also, can you add a comment when this condition is hit?
I now think that we should probably treat `availableWidth` as a constraint - sorry for the flip flop
Related details
- Charles Hager
- Shuhei Takahashi
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Renae “iamwren” Riley (Gerrit)
Renae “iamwren” Riley added 1 comment![]( "Open in Gerrit")
- Charles Hager
- Emilia Paz
- Masa Fujita
- Sirisha Kavuluru
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. |
Renae “iamwren” Riley (Gerrit)
Renae “iamwren” Riley added 1 comment![]( "Open in Gerrit")
waze
iamwren
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
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. |
Emilia Paz (Gerrit)
Emilia Paz added 5 comments![]( "Open in Gerrit")
Thanks Masa! The idea makes sense to me. However, I'm still getting familiar with Android and I think a reviewer from there would have better context (not sure I would be able to spot any animation issues)
.inflate(R.layout.extension_action_anchor, parent, false),nit: add `/* parameter_name= */` when parameter name cannot be inferred from variable
// TODO(crbug.com/478113313): If the action is unpinned, pop it out beforehand.for-future-cl: This means this function would do something more thank `getButton`. Should we handle this on a separate function, `popOutButtonView`?
*/can we explain what does `animate` param mean?
reconcileActionItems(/* animate= */ false);Masa FujitaHow often do we expect this to be called mid-animation? Are there any performance considerations, any edge cases where this could cause lagging during animations?
This should be very rare - this only happens when we get another `updateVisibility()` update (e.g. the user resizes window) during pinning / unpinning animation. In that case we abandon the ongoing animation and just update the UI instantly
- Charles Hager
- Masa Fujita
- Sirisha Kavuluru
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. |
Emilia Paz (Gerrit)
Emilia Paz added 1 comment![]( "Open in Gerrit")
question: in Desktopm, puzzle piece is always visible. On Desktop Android, puzzle piece can be hidden (user can unpin it). Have we asked UX what do we want when puzzle piece is unpinned and an extension action is triggered? Do we just show the action, and keep the puzzle piece hidden?
Ben Chin (Gerrit)
Ben Chin added 1 comment![]( "Open in Gerrit")
Masa FujitaOverall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
Masa FujitaThanks for taking a look! The performance seems... fine at best? 🙃 It works well on my high end test device but I'm not sure what the performance hit is going to be on lower end devices. It seems to be not as bad as the tab switcher animation but not too sure.
Do you know how we can quantitatively test animation performances?
(unresolving)
I’ve taken a look at the [Perfetto traces](https://ui.perfetto.dev/#!/query?local_cache_key=0209f219-1707-0712-1c68-c17f06eab805) specifically to check for main thread blocking or layout starvation, and I haven't seen any negative impact on timing so far (It took 0.02 ms).
https://screenshot.googleplex.com/JfDHU93jXkjdNoL
Ben Chin (Gerrit)
Ben Chin added 1 comment![]( "Open in Gerrit")
Masa FujitaOverall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
Masa FujitaThanks for taking a look! The performance seems... fine at best? 🙃 It works well on my high end test device but I'm not sure what the performance hit is going to be on lower end devices. It seems to be not as bad as the tab switcher animation but not too sure.
Do you know how we can quantitatively test animation performances?
Ben Chin(unresolving)
I’ve taken a look at the [Perfetto traces](https://ui.perfetto.dev/#!/query?local_cache_key=0209f219-1707-0712-1c68-c17f06eab805) specifically to check for main thread blocking or layout starvation, and I haven't seen any negative impact on timing so far (It took 0.02 ms).
https://screenshot.googleplex.com/JfDHU93jXkjdNoL
Attach Perfetto traces logs: https://ui.perfetto.dev/#!/?s=d4efaf939722f3b71fce2fd23ba3c0837eaf89fb
Masa Fujita (Gerrit)
Masa Fujita added 4 comments![]( "Open in Gerrit")
Masa FujitaOverall things seem fine, though I think I'll take a look at some point to see how things look frame-by-frame. Have you checked at all whether this has an impact on performance / timing? I know that's been a complication for some of the other toolbar components.
Masa FujitaThanks for taking a look! The performance seems... fine at best? 🙃 It works well on my high end test device but I'm not sure what the performance hit is going to be on lower end devices. It seems to be not as bad as the tab switcher animation but not too sure.
Do you know how we can quantitatively test animation performances?
Ben Chin(unresolving)
Ben ChinI’ve taken a look at the [Perfetto traces](https://ui.perfetto.dev/#!/query?local_cache_key=0209f219-1707-0712-1c68-c17f06eab805) specifically to check for main thread blocking or layout starvation, and I haven't seen any negative impact on timing so far (It took 0.02 ms).
https://screenshot.googleplex.com/JfDHU93jXkjdNoL
Attach Perfetto traces logs: https://ui.perfetto.dev/#!/?s=d4efaf939722f3b71fce2fd23ba3c0837eaf89fb
That's fantastic news! I'm so glad to hear this. Thanks for verifying!
question: in Desktopm, puzzle piece is always visible. On Desktop Android, puzzle piece can be hidden (user can unpin it). Have we asked UX what do we want when puzzle piece is unpinned and an extension action is triggered? Do we just show the action, and keep the puzzle piece hidden?
Yes, though it's a bit complicated (especially with popped out actions). chatted offline and under discussion with UX
Masa FujitaThanks Masa! The idea makes sense to me. However, I'm still getting familiar with Android and I think a reviewer from there would have better context (not sure I would be able to spot any animation issues)
Thanks for the review as always Emilia! They're always insightful.
@skav...@google.com Can you take a look at this CL overall from the Android side?
private class MenuButtonWidthConsumer implements ToolbarWidthConsumer {@clh...@google.com I think I'm now understanding how I should be using ToolbarWidthConsumer correctly! I updated this part, ptal
Related details
- Ben Chin
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
Masa Fujita (Gerrit)
Masa Fujita voted and added 4 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
4 comments
.inflate(R.layout.extension_action_anchor, parent, false),nit: add `/* parameter_name= */` when parameter name cannot be inferred from variable
Done
// TODO(crbug.com/478113313): If the action is unpinned, pop it out beforehand.for-future-cl: This means this function would do something more thank `getButton`. Should we handle this on a separate function, `popOutButtonView`?
Yep, will handle this in the next cl.
can we explain what does `animate` param mean?
Done
reconcileActionItems(/* animate= */ false);Masa FujitaHow often do we expect this to be called mid-animation? Are there any performance considerations, any edge cases where this could cause lagging during animations?
Emilia PazThis should be very rare - this only happens when we get another `updateVisibility()` update (e.g. the user resizes window) during pinning / unpinning animation. In that case we abandon the ongoing animation and just update the UI instantly
Lets add that as a comment :)
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 1 comment![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
1 comment
private class MenuButtonWidthConsumer implements ToolbarWidthConsumer {@clh...@google.com I think I'm now understanding how I should be using ToolbarWidthConsumer correctly! I updated this part, ptal
Done
Related details
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emilia Paz
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita added 1 comment![]( "Open in Gerrit")
* 10;Masa FujitaOut of curiosity, why 10 times the length of a button / touch target?
Masa FujitaThis is honestly not based on much. We need this padding because even though the entire `RecyclerView` should always theoretically remain visible, the animation can get janky and `RecyclerView` might decide to destroy the anchors' View instances.
Decided to use a window width (10 was a bit too random).
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Emilia Paz
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 6 comments![]( "Open in Gerrit")
Left comments related to toolbar width consumption. Looking at animations next
We take advantage of the dynamic toolbar ranking implementations and
route the layout request to `ExtensionActionListMediator` to decide the
proper action to hide when the window gets too narrow.It looks like this CL implements 3 things - adding an anchor view, animating toolbar actions and consuming toolbar width.
Would it be possible to split these into separate CLs inorder to evaluate changes required for each separately? We can have one for anchor + toolbar width consumption and then another one for animations support?
if (mProfile.shutdownStarted()) {
// TODO(crbug.com/459079170): This is to prevent tests from breaking. {@code
// ExtensionToolbarCoordinatorImpl} should ideally be destroyed following {@code
// ChromeAndroidTask}'s destruction, and it is currently being worked on.
return null;
}How is profile destruction related to the change in this CL?
this,This creates a cyclic dependency where coordinator has access to mediator and vice versa. We should instead define a delegate interface that has methods required by mediator, have this coordinator implement that interface and then pass the interface reference to mediator instead
mContainer.scheduleRequestLayout();Why do we need to call this?
(view) -> onPrimaryClick("aapbdbdomjkkjkaonfhkkikfgjllcleb"))revert
Related details
- Masa Fujita
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 4 comments![]( "Open in Gerrit")
actionModels,Curious - why do we not want to pass mModelsWithAnchors to mediator?
reconcileActionItems(/* animate= */ false);why do we want this to be false?
reconcileActionItems(/* animate= */ false);What happens if we just set an animator on recycler view and never disable it? Do we see issues in this initialization flow? It looks like animate is false for this one and for fitToWidth - wondering if we should set to true for both these cases too
Havent seen this pattern where we need to explicitly set item animator to value vs null. The only other example I am aware of is we do this for tab switcher where we disable recycler view animation when it might conflict with another swipe animation : https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java;l=1388;drc=4e7bd600fd6278b17f3697832a13268593416e4b;bpv=0;bpt=1
wrapRecyclerViewWidth();Trying to understand this better. I assume this maps to "During `RecyclerView`'s
animation, we request layout to the `RecyclerView` every frame to make
sure that the omnibox perfectly follows the `RecyclerView`'s animation." in the description?
Could we draft the flow without and then with this change to understand better. IIUC, current flow without this change is:
Extension action is added or removed from list -> recycler view item animator is kicked off and recycler view shrinks or expands. Do we see a delay in omnibox resize during this flow? Can test this on my device tomorrow.
With this change when we call setLayoutParams on this recycler view, does it trigger an onMeasure on ToolbarTablet which then updates the width of omnibox?
Historically, EditText#onMeasure is an expensive operation and should be avoided if possible. Just from online search, I see one approach to animation an Edittext view along with a recycler view is to register a registerAdapterDataObserver() on Recyclerview adapter's and use the onInsert / onRemove to trigger animation on sibling view. We could use TransitionManager here which might have inbuilt optimizations for EditText animations.
Masa Fujita (Gerrit)
Masa Fujita added 4 comments![]( "Open in Gerrit")
if (mProfile.shutdownStarted()) {
// TODO(crbug.com/459079170): This is to prevent tests from breaking. {@code
// ExtensionToolbarCoordinatorImpl} should ideally be destroyed following {@code
// ChromeAndroidTask}'s destruction, and it is currently being worked on.
return null;
}How is profile destruction related to the change in this CL?
Sorry, I moved this to a separate CL (crrev.com/c/7568682). Without the patch, this CL made some tests crash due to our new callbacks. During test destructions, sometimes the profile was destroyed before the UI, which caused the callback to run on a broken bridge. The fundamental problem is that extensions UI outlives the bridge.
actionModels,Curious - why do we not want to pass mModelsWithAnchors to mediator?
We decided that we should consolidate the anchors logic and let mediator deal with the actual items instead of the one with anchors (the `reconcileActionItems()` got pretty complicated, hence `ExtensionActionListAnchoredModelList`)
reconcileActionItems(/* animate= */ false);why do we want this to be false?
Animations during ToolbarTablet's `updateVisibility()` doesn't work too well, unfortunately :(
The flow would be something like:
```
ToolbarTablet requests new width
-> We incrementaly increase / decrease width
-> ToolbarTablet comes up with a new width
-> ...
```
and gets stuck sometimes.
wrapRecyclerViewWidth();Trying to understand this better. I assume this maps to "During `RecyclerView`'s
animation, we request layout to the `RecyclerView` every frame to make
sure that the omnibox perfectly follows the `RecyclerView`'s animation." in the description?Could we draft the flow without and then with this change to understand better. IIUC, current flow without this change is:
Extension action is added or removed from list -> recycler view item animator is kicked off and recycler view shrinks or expands. Do we see a delay in omnibox resize during this flow? Can test this on my device tomorrow.With this change when we call setLayoutParams on this recycler view, does it trigger an onMeasure on ToolbarTablet which then updates the width of omnibox?
Historically, EditText#onMeasure is an expensive operation and should be avoided if possible. Just from online search, I see one approach to animation an Edittext view along with a recycler view is to register a registerAdapterDataObserver() on Recyclerview adapter's and use the onInsert / onRemove to trigger animation on sibling view. We could use TransitionManager here which might have inbuilt optimizations for EditText animations.
The main problem that we have here that the `RecyclerView`'s width updates instantly, and does not follow the animation of its contents. Without this CL, the omnibox jumps instantly, and then the icons would move into place. If we want the container (and the omnibox) to follow the contents' animation, we have to manually measure the contents and their transforms and request the appropriate width to the layouter.
I thought about using separate but similar / same animations on the container too, hoping that it would be better performance-wise, but it's very difficult to completely synchronize the animation of the outer container with the contents because the `RecyclerView` animates async + we would have to completely reconstruct `RecyclerView`'s animations (which is non-linear with things like fade in -> slide etc).
I uploaded a new patch where we determine the width on `onMeasure()` phase, which hopefully is a bit cleaner. The flow would be something like:
```
We update data
while (RecyclerView's animation is going on)
mRequestLayoutRunnable is run
onMeasure() measures the contents and returns the 'actual' width
mRequestLayoutRunnable adds itself again to request layout in the next frame
```
Ben's analysis ([1]) seems to suggest that the performance hit is hopefully neglegible, and I'm thinking that we could maybe turn off animations for lower end devices altogether. But another option is to give up perfect synchronization and just add another animation on the container (but it ended up looking very janky even with my best effort...)
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7521864/comments/3a5eece5_ffd2a254
Related details
- Sirisha Kavuluru
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita added 3 comments![]( "Open in Gerrit")
We take advantage of the dynamic toolbar ranking implementations and
route the layout request to `ExtensionActionListMediator` to decide the
proper action to hide when the window gets too narrow.It looks like this CL implements 3 things - adding an anchor view, animating toolbar actions and consuming toolbar width.
Would it be possible to split these into separate CLs inorder to evaluate changes required for each separately? We can have one for anchor + toolbar width consumption and then another one for animations support?
Good point, I'll work on splitting it into 3 - the resulting code should look about the same, can you review on this one for now? I will work on splitting it tomorrow
This creates a cyclic dependency where coordinator has access to mediator and vice versa. We should instead define a delegate interface that has methods required by mediator, have this coordinator implement that interface and then pass the interface reference to mediator instead
Done
reconcileActionItems(/* animate= */ false);What happens if we just set an animator on recycler view and never disable it? Do we see issues in this initialization flow? It looks like animate is false for this one and for fitToWidth - wondering if we should set to true for both these cases too
Havent seen this pattern where we need to explicitly set item animator to value vs null. The only other example I am aware of is we do this for tab switcher where we disable recycler view animation when it might conflict with another swipe animation : https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java;l=1388;drc=4e7bd600fd6278b17f3697832a13268593416e4b;bpv=0;bpt=1
As mentioned in the other reply, I don't think we should enable animations for `fitToWidth`. For initialization I'm fine with either (to me it looks about the same whether we pass true or false here).
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 1 comment![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
1 comment
We take advantage of the dynamic toolbar ranking implementations and
route the layout request to `ExtensionActionListMediator` to decide the
proper action to hide when the window gets too narrow.Masa FujitaIt looks like this CL implements 3 things - adding an anchor view, animating toolbar actions and consuming toolbar width.
Would it be possible to split these into separate CLs inorder to evaluate changes required for each separately? We can have one for anchor + toolbar width consumption and then another one for animations support?
Good point, I'll work on splitting it into 3 - the resulting code should look about the same, can you review on this one for now? I will work on splitting it tomorrow
Split into 3 CLs - done
Related details
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita added 5 comments![]( "Open in Gerrit")
// RecyclerView} do not have sliding animations and only have fading animations, but we wantMasa FujitaDo the edge items get created but aren't initially visible, then slide into view during the animation?
Masa FujitaWithout the anchors, if the edge items' visibilities change:
- the edge item at the start (far right (because `stackFromEnd = true`)) gets created outside the viewport and does not slide in automatically. We would have to programatically scroll to the top.
- the edge item at the end (far left) gets created with a "fade in" animation. No sliding animation for anyone, so nothing for the omnibox to track.
but, with the anchors, no icons are edge items, so everything should be always stay visible and within the bounds of the `RecyclerView`.
Done
Why do we need to call this?
ditto
(view) -> onPrimaryClick("aapbdbdomjkkjkaonfhkkikfgjllcleb"))Masa Fujitarevert
Sorry!
Masa Fujitawhy do we want this to be false?
Animations during ToolbarTablet's `updateVisibility()` doesn't work too well, unfortunately :(
The flow would be something like:
```
ToolbarTablet requests new width
-> We incrementaly increase / decrease width
-> ToolbarTablet comes up with a new width
-> ...
```
and gets stuck sometimes.
Decided to keep animations ON for everything for now with our new approach - if we use TransitionManager the "bounds" are updated instantly, so it should not fight the ranking system.
reconcileActionItems(/* animate= */ false);Masa FujitaWhat happens if we just set an animator on recycler view and never disable it? Do we see issues in this initialization flow? It looks like animate is false for this one and for fitToWidth - wondering if we should set to true for both these cases too
Havent seen this pattern where we need to explicitly set item animator to value vs null. The only other example I am aware of is we do this for tab switcher where we disable recycler view animation when it might conflict with another swipe animation : https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java;l=1388;drc=4e7bd600fd6278b17f3697832a13268593416e4b;bpv=0;bpt=1
As mentioned in the other reply, I don't think we should enable animations for `fitToWidth`. For initialization I'm fine with either (to me it looks about the same whether we pass true or false here).
- Charles Hager
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 2 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
Masa FujitaThanks Masa! The idea makes sense to me. However, I'm still getting familiar with Android and I think a reviewer from there would have better context (not sure I would be able to spot any animation issues)
Thanks for the review as always Emilia! They're always insightful.
@skav...@google.com Can you take a look at this CL overall from the Android side?
Done
wrapRecyclerViewWidth();Masa FujitaTrying to understand this better. I assume this maps to "During `RecyclerView`'s
animation, we request layout to the `RecyclerView` every frame to make
sure that the omnibox perfectly follows the `RecyclerView`'s animation." in the description?Could we draft the flow without and then with this change to understand better. IIUC, current flow without this change is:
Extension action is added or removed from list -> recycler view item animator is kicked off and recycler view shrinks or expands. Do we see a delay in omnibox resize during this flow? Can test this on my device tomorrow.With this change when we call setLayoutParams on this recycler view, does it trigger an onMeasure on ToolbarTablet which then updates the width of omnibox?
Historically, EditText#onMeasure is an expensive operation and should be avoided if possible. Just from online search, I see one approach to animation an Edittext view along with a recycler view is to register a registerAdapterDataObserver() on Recyclerview adapter's and use the onInsert / onRemove to trigger animation on sibling view. We could use TransitionManager here which might have inbuilt optimizations for EditText animations.
The main problem that we have here that the `RecyclerView`'s width updates instantly, and does not follow the animation of its contents. Without this CL, the omnibox jumps instantly, and then the icons would move into place. If we want the container (and the omnibox) to follow the contents' animation, we have to manually measure the contents and their transforms and request the appropriate width to the layouter.
I thought about using separate but similar / same animations on the container too, hoping that it would be better performance-wise, but it's very difficult to completely synchronize the animation of the outer container with the contents because the `RecyclerView` animates async + we would have to completely reconstruct `RecyclerView`'s animations (which is non-linear with things like fade in -> slide etc).
I uploaded a new patch where we determine the width on `onMeasure()` phase, which hopefully is a bit cleaner. The flow would be something like:
```
We update data
while (RecyclerView's animation is going on)
mRequestLayoutRunnable is run
onMeasure() measures the contents and returns the 'actual' width
mRequestLayoutRunnable adds itself again to request layout in the next frame
```Ben's analysis ([1]) seems to suggest that the performance hit is hopefully neglegible, and I'm thinking that we could maybe turn off animations for lower end devices altogether. But another option is to give up perfect synchronization and just add another animation on the container (but it ended up looking very janky even with my best effort...)
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7521864/comments/3a5eece5_ffd2a254
Hey Sirisha, decided to do a 180 on the approach and use TransitionManager for animating the RecyclerView width. We try to recreate RecyclerView's DefaultItemAnimator animations.
This does result in some minor visual discrepancies between the icon animation and omnibox animations because animations are done async, but the CPU load is much much lower because we are not requesting layout every frame and now it's a "normal" animation. The visual discrepancies are usually not too bad (and never critical from UX perspective). Hopefully it's much more maintainable too. PTAL!
Related details
- Charles Hager
- Emilia Paz
- Sirisha Kavuluru
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
- Sirisha Kavuluru
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. |
Masa Fujita (Gerrit)
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 7 comments![]( "Open in Gerrit")
android:layout_width="2dp"Why can't we leave this as wrap_content? iiuc the 2dp here is for the 2 anchors?
// We put enough padding to make sure the View instances at the edge do not
// disappear.
int windowWidth = context.getResources().getDisplayMetrics().widthPixels;
extraLayoutSpace[0] = windowWidth;
extraLayoutSpace[1] = windowWidth;iiuc this method allows you to add start and end padding to the RecyclerView list. Why do we set these to the full window width? Do we need the anchor items if we can add space here?
@Override
protected void onAttachedToWindow() {
super.onAttachedToWindow();
if (mLayoutChangeListener != null) {
removeOnLayoutChangeListener(mLayoutChangeListener);
addOnLayoutChangeListener(mLayoutChangeListener);
}
}
@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
if (mLayoutChangeListener != null) {
removeOnLayoutChangeListener(mLayoutChangeListener);
}
}For my own understanding, when are these methods called?
ViewUtils.requestLayout(
ExtensionActionListRecyclerView.this,
"ExtensionActionListRecyclerView.updateRecyclerViewWidth");optional nit: I think this might not be required? Or we could do recyclerView#setLayoutParams which iirc internally requests layout
wrapRecyclerViewWidth();Masa FujitaTrying to understand this better. I assume this maps to "During `RecyclerView`'s
animation, we request layout to the `RecyclerView` every frame to make
sure that the omnibox perfectly follows the `RecyclerView`'s animation." in the description?Could we draft the flow without and then with this change to understand better. IIUC, current flow without this change is:
Extension action is added or removed from list -> recycler view item animator is kicked off and recycler view shrinks or expands. Do we see a delay in omnibox resize during this flow? Can test this on my device tomorrow.With this change when we call setLayoutParams on this recycler view, does it trigger an onMeasure on ToolbarTablet which then updates the width of omnibox?
Historically, EditText#onMeasure is an expensive operation and should be avoided if possible. Just from online search, I see one approach to animation an Edittext view along with a recycler view is to register a registerAdapterDataObserver() on Recyclerview adapter's and use the onInsert / onRemove to trigger animation on sibling view. We could use TransitionManager here which might have inbuilt optimizations for EditText animations.
Masa FujitaThe main problem that we have here that the `RecyclerView`'s width updates instantly, and does not follow the animation of its contents. Without this CL, the omnibox jumps instantly, and then the icons would move into place. If we want the container (and the omnibox) to follow the contents' animation, we have to manually measure the contents and their transforms and request the appropriate width to the layouter.
I thought about using separate but similar / same animations on the container too, hoping that it would be better performance-wise, but it's very difficult to completely synchronize the animation of the outer container with the contents because the `RecyclerView` animates async + we would have to completely reconstruct `RecyclerView`'s animations (which is non-linear with things like fade in -> slide etc).
I uploaded a new patch where we determine the width on `onMeasure()` phase, which hopefully is a bit cleaner. The flow would be something like:
```
We update data
while (RecyclerView's animation is going on)
mRequestLayoutRunnable is run
onMeasure() measures the contents and returns the 'actual' width
mRequestLayoutRunnable adds itself again to request layout in the next frame
```Ben's analysis ([1]) seems to suggest that the performance hit is hopefully neglegible, and I'm thinking that we could maybe turn off animations for lower end devices altogether. But another option is to give up perfect synchronization and just add another animation on the container (but it ended up looking very janky even with my best effort...)
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7521864/comments/3a5eece5_ffd2a254
Hey Sirisha, decided to do a 180 on the approach and use TransitionManager for animating the RecyclerView width. We try to recreate RecyclerView's DefaultItemAnimator animations.
This does result in some minor visual discrepancies between the icon animation and omnibox animations because animations are done async, but the CPU load is much much lower because we are not requesting layout every frame and now it's a "normal" animation. The visual discrepancies are usually not too bad (and never critical from UX perspective). Hopefully it's much more maintainable too. PTAL!
Acknowledged - Thanks for revisiting and simplifying this 😊
if (holder.getItemViewType() == ListItemType.ANCHOR) {
mHasPendingAnchorMove = true;
}This method would be called for anchor view for every pin / unpin action, right? When might mHasPendingAnchorMove be false during runPendingAnimations??
ViewGroup layoutRoot = getRootView().findViewById(R.id.toolbar_tablet_layout);Could we pass this as parentView or rootView from toolbar to ExtensionCoordinator to this class? This will change if we add the extension list to other toolbar like CCT
Related details
- Masa Fujita
Masa Fujita (Gerrit)
Masa Fujita voted and added 6 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
6 comments
Why can't we leave this as wrap_content? iiuc the 2dp here is for the 2 anchors?
I think with our new approach wrap_content works.
actionModels,Masa FujitaCurious - why do we not want to pass mModelsWithAnchors to mediator?
We decided that we should consolidate the anchors logic and let mediator deal with the actual items instead of the one with anchors (the `reconcileActionItems()` got pretty complicated, hence `ExtensionActionListAnchoredModelList`)
Done
// We put enough padding to make sure the View instances at the edge do not
// disappear.
int windowWidth = context.getResources().getDisplayMetrics().widthPixels;
extraLayoutSpace[0] = windowWidth;
extraLayoutSpace[1] = windowWidth;iiuc this method allows you to add start and end padding to the RecyclerView list. Why do we set these to the full window width? Do we need the anchor items if we can add space here?
We just need *some* width padding. Let's just use a static variable.
@Override
protected void onAttachedToWindow() {
super.onAttachedToWindow();
if (mLayoutChangeListener != null) {
removeOnLayoutChangeListener(mLayoutChangeListener);
addOnLayoutChangeListener(mLayoutChangeListener);
}
}
@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
if (mLayoutChangeListener != null) {
removeOnLayoutChangeListener(mLayoutChangeListener);
}
}For my own understanding, when are these methods called?
onDetached is called during destruction. onAttached was probably not necessary and we can just do it in the constructor, as we `post()` anyways.
ViewUtils.requestLayout(
ExtensionActionListRecyclerView.this,
"ExtensionActionListRecyclerView.updateRecyclerViewWidth");optional nit: I think this might not be required? Or we could do recyclerView#setLayoutParams which iirc internally requests layout
Let's do `setLayoutParams()`.
if (holder.getItemViewType() == ListItemType.ANCHOR) {
mHasPendingAnchorMove = true;
}This method would be called for anchor view for every pin / unpin action, right? When might mHasPendingAnchorMove be false during runPendingAnimations??
(I removed the anchors - with our new approach anchors are not necessary)
Related details
- Sirisha Kavuluru
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. |