[Glic] Creates ActorUiTabController Observer and Updates UI in Tab Grid [chromium/src : main]
Calder Kitagawa (Gerrit)
Calder Kitagawa added 6 comments![]( "Open in Gerrit")
View actorContainer = fastFindViewById(R.id.actor_ui_container);
if (visible && actorContainer == null) {
LayoutInflater.from(getContext())
.inflate(R.layout.actor_tab_grid_view_overlay, this, true);
actorContainer = fastFindViewById(R.id.actor_ui_container);
if (actorContainer != null) {
actorContainer.setLayoutParams(
new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
}
}Let's turn this into a helper: `getActorUi(boolean inflateIfMissing)`?
@madhav...@google.com is also working on a spinner for loading thumbnails can you sync with him so we can coordinate on not doing this all twice or at least sharing some infrastructure?
if (!ViewCompat.isAttachedToWindow(view)) {
return;
}
if (tabGridView.isActorUiVisible() != isDynamic) {
if (view.getWidth() <= 0) {
view.post(
() -> {
if (ViewCompat.isAttachedToWindow(view)) {
tabGridView.setActorActiveUiVisible(isDynamic);
}
});
} else {
tabGridView.setActorActiveUiVisible(isDynamic);
}
}I think we should be able to remove most of this logic. It should be possible to set things up even without the view attached to anything. I suspect we need a different fix inside `TabGridView` to gracefully handle not being attached when this is set. Otherwise if we get attached later we might fail to show something.
Also since this adds an animation inside a recyclerview adding @sk...@chromium.org who knows some gotchas about this we should be looking for.
for (int i = 0; i < mModelList.size(); i++) {
PropertyModel model = mModelList.get(i).model;
int tabId = model.get(TabProperties.TAB_ID);
Tab tab = getCurrentTabModelChecked().getTabById(tabId);
if (tab != null) {
ActorUiTabController.from(tab).removeObserver(mActorObserver);
}
}We should be doing this inside `removeObservers` to avoid an extra loop and leverage the existing tab handling logic there.
// Add this line to ensure real-time updates for NEW tabs too:
controller.removeObserver(mActorObserver);
controller.addObserver(mActorObserver);I don't think we should be relying on toggling the observer attachment here, that seems like it is papering over a bug.
There are two places where we handle updates
`addTabInfoToModel` new additions, `updateTab` for large mutations. What exactly is the removal and addition trying to accomplish? Should we be checking some state here?
@Mock ActorOverlayState mActorOverlayState; // Import from your actor package?
LOG(ERROR) << "BBETINI" << "Actor Keyed Service - CreateTaskImpl";Remove these?
Related details
- Bhuvana Betini
- Madhav Pruthi
- Sky Malice
- 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. |
Bhuvana Betini (Gerrit)
Bhuvana Betini added 3 comments![]( "Open in Gerrit")
View actorContainer = fastFindViewById(R.id.actor_ui_container);
if (visible && actorContainer == null) {
LayoutInflater.from(getContext())
.inflate(R.layout.actor_tab_grid_view_overlay, this, true);
actorContainer = fastFindViewById(R.id.actor_ui_container);
if (actorContainer != null) {
actorContainer.setLayoutParams(
new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
}
}Let's turn this into a helper: `getActorUi(boolean inflateIfMissing)`?
@madhav...@google.com is also working on a spinner for loading thumbnails can you sync with him so we can coordinate on not doing this all twice or at least sharing some infrastructure?
Done.
Discussing with Madhav, but seems like UI is at a very high-risk state so I will default to just showing an icon for now.
@Mock ActorOverlayState mActorOverlayState; // Import from your actor packageBhuvana Betini?
Done
LOG(ERROR) << "BBETINI" << "Actor Keyed Service - CreateTaskImpl";Bhuvana BetiniRemove these?
Done
Related details
- Calder Kitagawa
- Madhav Pruthi
- Sky Malice
- 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 6 comments![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
6 comments
if (!ViewCompat.isAttachedToWindow(view)) {
return;
}
if (tabGridView.isActorUiVisible() != isDynamic) {
if (view.getWidth() <= 0) {
view.post(
() -> {
if (ViewCompat.isAttachedToWindow(view)) {
tabGridView.setActorActiveUiVisible(isDynamic);
}
});
} else {
tabGridView.setActorActiveUiVisible(isDynamic);
}
}I think we should be able to remove most of this logic. It should be possible to set things up even without the view attached to anything. I suspect we need a different fix inside `TabGridView` to gracefully handle not being attached when this is set. Otherwise if we get attached later we might fail to show something.
Also since this adds an animation inside a recyclerview adding @sk...@chromium.org who knows some gotchas about this we should be looking for.
Drive by on this, WRT animation, I think UX design is not finalized yet, so if that's a problem for this CL we can remove it and just have it to be a plain colors.
But we will likely need to have an animated view somewhere around in terms of few weeks
public static final WritableObjectPropertyKey<UiTabState> ACTOR_UI_STATE =nit: Javadoc to be consistent
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"Can you put a comment what this is? Currently this has a similar name with actor_overlay which is for much different purposes.
Otherwise naming suggestion: actor_gts_tab_indicator.xml or something down the line
mTab.getUserDataHost().removeUserData(USER_DATA_KEY);Is removing this intended?
#include "chrome/browser/actor/actor_keyed_service_factory.h"nit: IS this still needed? The rest of the file is not changed.
Related details
- Bhuvana Betini
- Calder Kitagawa
- Sky Malice
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 1 comment![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +0 |
1 comment
(Sorry misclick)
Related details
- Bhuvana Betini
- Calder Kitagawa
- Sky Malice
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. |
Bhuvana Betini (Gerrit)
Bhuvana Betini added 7 comments![]( "Open in Gerrit")
if (!ViewCompat.isAttachedToWindow(view)) {
return;
}
if (tabGridView.isActorUiVisible() != isDynamic) {
if (view.getWidth() <= 0) {
view.post(
() -> {
if (ViewCompat.isAttachedToWindow(view)) {
tabGridView.setActorActiveUiVisible(isDynamic);
}
});
} else {
tabGridView.setActorActiveUiVisible(isDynamic);
}
}Wenyu FuI think we should be able to remove most of this logic. It should be possible to set things up even without the view attached to anything. I suspect we need a different fix inside `TabGridView` to gracefully handle not being attached when this is set. Otherwise if we get attached later we might fail to show something.
Also since this adds an animation inside a recyclerview adding @sk...@chromium.org who knows some gotchas about this we should be looking for.
Drive by on this, WRT animation, I think UX design is not finalized yet, so if that's a problem for this CL we can remove it and just have it to be a plain colors.
But we will likely need to have an animated view somewhere around in terms of few weeks
Done, I think!
for (int i = 0; i < mModelList.size(); i++) {
PropertyModel model = mModelList.get(i).model;
int tabId = model.get(TabProperties.TAB_ID);
Tab tab = getCurrentTabModelChecked().getTabById(tabId);
if (tab != null) {
ActorUiTabController.from(tab).removeObserver(mActorObserver);
}
}We should be doing this inside `removeObservers` to avoid an extra loop and leverage the existing tab handling logic there.
Done
// Add this line to ensure real-time updates for NEW tabs too:
controller.removeObserver(mActorObserver);
controller.addObserver(mActorObserver);I don't think we should be relying on toggling the observer attachment here, that seems like it is papering over a bug.
There are two places where we handle updates
`addTabInfoToModel` new additions, `updateTab` for large mutations. What exactly is the removal and addition trying to accomplish? Should we be checking some state here?
The purpose of this is to add an observer when a new tab is added.. but I suppose this may not even be neccessary? We would only need an observer when we get a UiTabState update.
public static final WritableObjectPropertyKey<UiTabState> ACTOR_UI_STATE =nit: Javadoc to be consistent
Done
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"Can you put a comment what this is? Currently this has a similar name with actor_overlay which is for much different purposes.
Otherwise naming suggestion: actor_gts_tab_indicator.xml or something down the line
Renamed.
mTab.getUserDataHost().removeUserData(USER_DATA_KEY);Bhuvana BetiniIs removing this intended?
thanks for catching!
#include "chrome/browser/actor/actor_keyed_service_factory.h"nit: IS this still needed? The rest of the file is not changed.
- Calder Kitagawa
- Sky Malice
- 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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 5 comments![]( "Open in Gerrit")
@Nullable
private View getActorUi(boolean inflateIfMissing) {```suggestion
private @Nullable View getActorUi(boolean inflateIfMissing) {
```
if (actorContainer != null) {Change this to an assert it shouldn't be null anymore.
bringChildToFront(actorContainer);Can we just do this once when inflating the view?
// Add this line to ensure real-time updates for NEW tabs too:
controller.removeObserver(mActorObserver);
controller.addObserver(mActorObserver);Bhuvana BetiniI don't think we should be relying on toggling the observer attachment here, that seems like it is papering over a bug.
There are two places where we handle updates
`addTabInfoToModel` new additions, `updateTab` for large mutations. What exactly is the removal and addition trying to accomplish? Should we be checking some state here?
The purpose of this is to add an observer when a new tab is added.. but I suppose this may not even be neccessary? We would only need an observer when we get a UiTabState update.
Then we should add/remove the observer wherever `mTabObserver` is added/removed. (Can create helper methods for this). I don't want to include the new observer logic inside `addTabInfoToModel` since that is concerned with property model updates.
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Related details
- Bhuvana Betini
- Sky Malice
- Wenyu Fu
Bhuvana Betini (Gerrit)
Bhuvana Betini added 6 comments![]( "Open in Gerrit")
@Nullable
private View getActorUi(boolean inflateIfMissing) {```suggestion
private @Nullable View getActorUi(boolean inflateIfMissing) {
```
Done
Change this to an assert it shouldn't be null anymore.
Done
Can we just do this once when inflating the view?
Done
// Add this line to ensure real-time updates for NEW tabs too:
controller.removeObserver(mActorObserver);
controller.addObserver(mActorObserver);Bhuvana BetiniI don't think we should be relying on toggling the observer attachment here, that seems like it is papering over a bug.
There are two places where we handle updates
`addTabInfoToModel` new additions, `updateTab` for large mutations. What exactly is the removal and addition trying to accomplish? Should we be checking some state here?
Calder KitagawaThe purpose of this is to add an observer when a new tab is added.. but I suppose this may not even be neccessary? We would only need an observer when we get a UiTabState update.
Then we should add/remove the observer wherever `mTabObserver` is added/removed. (Can create helper methods for this). I don't want to include the new observer logic inside `addTabInfoToModel` since that is concerned with property model updates.
Done. Created helpers and replaced calls to add/removeObserver.
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Done
mTab.getUserDataHost().removeUserData(USER_DATA_KEY);Bhuvana BetiniIs removing this intended?
thanks for catching!
I think this may actually be necessary since its redundant to call removeUserData on the tab itself. I keep failing tests on this line since I don't think we should be able to call removeUserData after the destroy.
Related details
- Calder Kitagawa
- Sky Malice
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 2 comments![]( "Open in Gerrit")
int tabIndex = mModelList.indexFromTabId(state.tabId);
if (tabIndex != TabModel.INVALID_TAB_INDEX) {
mModelList.get(tabIndex).model.set(TabProperties.ACTOR_UI_STATE, state);
}For `mActionsOnAllRelatedTabs = true` you will need to find the group entry and see if any or all members of the group are actuating before changing this.
.with(TabProperties.ACTOR_UI_STATE, initialState)For `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
Related details
- Bhuvana Betini
- Sky Malice
- 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 5 comments![]( "Open in Gerrit")
private @Nullable View getActorUi(boolean inflateIfMissing) {When will this be null?
LayoutInflater.from(getContext()).inflate(R.layout.actor_gts_tab_indicator, this, true);nit: `inflate` already returns the view that's inflated, so no need to do `fastFindViewById` afterwards.
Or do we want to call `fastFindViewById` so that it is cached?
View actorContainer = fastFindViewById(R.id.actor_ui_container);nit: What's the scenario this method is useful (ie. when layout not inflated)?
I wonder if we can just track `mActorUiVisible && mIsAttachedToWindow`?
.with(TabProperties.ACTOR_UI_STATE, initialState)For `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
We'll need this....
Is it ok to leave a TODO for the next CL?
Related details
- Bhuvana Betini
- Sky Malice
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
LayoutInflater.from(getContext()).inflate(R.layout.actor_gts_tab_indicator, this, true);nit: `inflate` already returns the view that's inflated, so no need to do `fastFindViewById` afterwards.
Or do we want to call `fastFindViewById` so that it is cached?
If you inflate and `attachToRoot` is true the method returns the `root` you passed in rather than the inflated view.
Bhuvana Betini (Gerrit)
Bhuvana Betini added 5 comments![]( "Open in Gerrit")
private @Nullable View getActorUi(boolean inflateIfMissing) {When will this be null?
I believe fastFindViewById returns a Nullable View, so we would need this I think
View actorContainer = fastFindViewById(R.id.actor_ui_container);nit: What's the scenario this method is useful (ie. when layout not inflated)?
I wonder if we can just track `mActorUiVisible && mIsAttachedToWindow`?
Replaced to just track mActorUiVisible and mIsAttachedToWindow
int tabIndex = mModelList.indexFromTabId(state.tabId);
if (tabIndex != TabModel.INVALID_TAB_INDEX) {
mModelList.get(tabIndex).model.set(TabProperties.ACTOR_UI_STATE, state);
}For `mActionsOnAllRelatedTabs = true` you will need to find the group entry and see if any or all members of the group are actuating before changing this.
Done
.with(TabProperties.ACTOR_UI_STATE, initialState)For `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
I think for this Cl since UX is at a high-risk state, I'll just add the indicator to the overall tab group thumbnail as well as the thumbnail in the expanded Tab Group view.
But I think it would be helpful to know how much of a technical complexity it would be to show the indicator on a specific sub-quadrant in a tab group of the tab being acted on in the full grid view. How feasible is this?
Related details
- Calder Kitagawa
- Sky Malice
- 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 3 comments![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
3 comments
private @Nullable View getActorUi(boolean inflateIfMissing) {Bhuvana BetiniWhen will this be null?
I believe fastFindViewById returns a Nullable View, so we would need this I think
For this method though, can we do `assertNonNull(actorContainer)`?
LayoutInflater.from(getContext()).inflate(R.layout.actor_gts_tab_indicator, this, true);nit: `inflate` already returns the view that's inflated, so no need to do `fastFindViewById` afterwards.
Or do we want to call `fastFindViewById` so that it is cached?
If you inflate and `attachToRoot` is true the method returns the `root` you passed in rather than the inflated view.
Acknowledged - thanks Calder!
.with(TabProperties.ACTOR_UI_STATE, initialState)Bhuvana BetiniFor `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
I think for this Cl since UX is at a high-risk state, I'll just add the indicator to the overall tab group thumbnail as well as the thumbnail in the expanded Tab Group view.
But I think it would be helpful to know how much of a technical complexity it would be to show the indicator on a specific sub-quadrant in a tab group of the tab being acted on in the full grid view. How feasible is this?
I think we'd have to modify the thumbnail in order to do that.
I really want to press UX to no going into that route to avoid complexity; that shouldn't impact the structure of this CL too much, though
Related details
- Bhuvana Betini
- Calder Kitagawa
- Sky Malice
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 3 comments![]( "Open in Gerrit")
PropertyModel groupModel =
mModelList.getModelFromTabId(tab.getRootId());
if (groupModel != null) {
groupModel.set(TabProperties.ACTOR_UI_STATE, state);
}I think you should extract the logic from `addTabInfoToModel` and reuse it here. The state of one tab switching might not mean all tabs in the group switched. Also `getRootId()` is deprecated and will not find you the right model, you need to iterate over all the tabs in the tab group until you find a valid propertymodel from one of their tab ids.
if (mActionsOnAllRelatedTabs && isInTabGroup && tab.isInitialized()) {I think this should always be true? Was there a reason you encountered to add it?
.with(TabProperties.ACTOR_UI_STATE, initialState)Bhuvana BetiniFor `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
I think for this Cl since UX is at a high-risk state, I'll just add the indicator to the overall tab group thumbnail as well as the thumbnail in the expanded Tab Group view.
But I think it would be helpful to know how much of a technical complexity it would be to show the indicator on a specific sub-quadrant in a tab group of the tab being acted on in the full grid view. How feasible is this?
You'd need to add the logic into the [`MultiThumbnailFetcher`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/MultiThumbnailCardProvider.java) that generates the quadrants. It would be non-trivial, but feasible.
Related details
- Bhuvana Betini
- Sky Malice
Bhuvana Betini (Gerrit)
Bhuvana Betini added 4 comments![]( "Open in Gerrit")
private @Nullable View getActorUi(boolean inflateIfMissing) {Bhuvana BetiniWhen will this be null?
Wenyu FuI believe fastFindViewById returns a Nullable View, so we would need this I think
For this method though, can we do `assertNonNull(actorContainer)`?
Done
PropertyModel groupModel =
mModelList.getModelFromTabId(tab.getRootId());
if (groupModel != null) {
groupModel.set(TabProperties.ACTOR_UI_STATE, state);
}I think you should extract the logic from `addTabInfoToModel` and reuse it here. The state of one tab switching might not mean all tabs in the group switched. Also `getRootId()` is deprecated and will not find you the right model, you need to iterate over all the tabs in the tab group until you find a valid propertymodel from one of their tab ids.
Done
if (mActionsOnAllRelatedTabs && isInTabGroup && tab.isInitialized()) {I think this should always be true? Was there a reason you encountered to add it?
You're right, this shouldn't be needed. I added it originally because there seemed to be ocassional crashing when the Actor Task is creating a tab on the native side, but I think that needs to be addressed separately. This doesn't seem to fix it regardless.
.with(TabProperties.ACTOR_UI_STATE, initialState)Bhuvana BetiniFor `mActionsOnAllRelatedTabs == true` a single "tab" represents all tabs in its group (This is bad design from a long time ago). Do we want to show the indicator if ANY tab in the group is being actuated? If so we probably need to iterate over all tabs in the group here.
Calder KitagawaI think for this Cl since UX is at a high-risk state, I'll just add the indicator to the overall tab group thumbnail as well as the thumbnail in the expanded Tab Group view.
But I think it would be helpful to know how much of a technical complexity it would be to show the indicator on a specific sub-quadrant in a tab group of the tab being acted on in the full grid view. How feasible is this?
You'd need to add the logic into the [`MultiThumbnailFetcher`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/MultiThumbnailCardProvider.java) that generates the quadrants. It would be non-trivial, but feasible.
Makes sense. And we could support animations on the quadrants too?
Related details
- Calder Kitagawa
- Sky Malice
- 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. |