actions revamp: Add actions list to the toolbar rankings [chromium/src : main]
Masa Fujita (Gerrit)
Masa Fujita has uploaded the change for review![]( "Open in Gerrit")
Commit message
actions revamp: Add actions list to the toolbar rankings
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/ExtensionActionListMediatorTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListRecyclerView.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionToolbarCoordinatorImpl.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 has uploaded the change for review![]( "Open in Gerrit")
Commit message
actions revamp: Add actions list to the toolbar rankings
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/ExtensionActionListMediatorTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListRecyclerView.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionToolbarCoordinatorImpl.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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- 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
- 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
- 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
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
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")
// This method gets called when icons are being dragged, but we don't want width update
// to happen then.Curious - why does this get called during icon drag?
// We have to calculate the width ourselves because {@link RecyclerView} updates its Views
// async.Are we seeing issues if we use recyclerView.getWidth() instead? I thought recyclerView requestLayout should be sync..
mContext.getResources()
.getDimensionPixelSize(
org.chromium.chrome.browser.toolbar.R.dimen.toolbar_button_width);nit: Can we get this from mediator - maybe expose a mMediator.getViewWidth()?
int maxNumberOfItems = Integer.MAX_VALUE;
if (mAvailableWidth != null) {
int itemWidth =
mContext.getResources().getDimensionPixelSize(R.dimen.toolbar_button_width);
assert itemWidth > 0;
maxNumberOfItems = mAvailableWidth / itemWidth;
}
// O(N) for removals/no-ops; O(N^2) for reordering/insertions.
int currentModelIndex = 0;
for (String actionId : actionIds) {
if (currentModelIndex >= maxNumberOfItems) {
break;
}iiuc this method updates mModels when new action is added, pinned , unpinned..
1. Is onActionAdded() is called when a new extension is installed? If not, when is this invoked? I see on desktop we temporarily add a button to the toolbar which could show a popup or hide after a few seconds.
2. onActionRemoved() removed item from the models. iiuc this is invoked if extension is uninstalled? Do we need to reconcile the items to since we now have space to show 1 more?
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 4 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
4 comments
// This method gets called when icons are being dragged, but we don't want width update
// to happen then.Curious - why does this get called during icon drag?
Items swapping triggers a layout pass for the RecyclerView.
// We have to calculate the width ourselves because {@link RecyclerView} updates its Views
// async.Are we seeing issues if we use recyclerView.getWidth() instead? I thought recyclerView requestLayout should be sync..
Yeah sorry, this is remnants from my previous impl attempt
mContext.getResources()
.getDimensionPixelSize(
org.chromium.chrome.browser.toolbar.R.dimen.toolbar_button_width);nit: Can we get this from mediator - maybe expose a mMediator.getViewWidth()?
Acknowledged
int maxNumberOfItems = Integer.MAX_VALUE;
if (mAvailableWidth != null) {
int itemWidth =
mContext.getResources().getDimensionPixelSize(R.dimen.toolbar_button_width);
assert itemWidth > 0;
maxNumberOfItems = mAvailableWidth / itemWidth;
}
// O(N) for removals/no-ops; O(N^2) for reordering/insertions.
int currentModelIndex = 0;
for (String actionId : actionIds) {
if (currentModelIndex >= maxNumberOfItems) {
break;
}iiuc this method updates mModels when new action is added, pinned , unpinned..
1. Is onActionAdded() is called when a new extension is installed? If not, when is this invoked? I see on desktop we temporarily add a button to the toolbar which could show a popup or hide after a few seconds.
2. onActionRemoved() removed item from the models. iiuc this is invoked if extension is uninstalled? Do we need to reconcile the items to since we now have space to show 1 more?
1. On Clank, the UX requirements are different than Desktop (per UX decisions) - on install we show a dialog instead of popped out actions (temporarily pinned action). So as for what's displayed on the toolbar, we just have to match it to the stored pinned_actions. That said, popped out actions will be necessary (e.g. for popups opened via the extensions menu) and it's implemented in the stacked CL
2. When an extension is uninstalled, that model should be removed from the ModelList and the width of the RecyclerView should shrink, unless there are hidden icons due to space concerns, in which case this logic should swap the model (I'm not sure if I understand the question, does this answer it?)
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 3 comments![]( "Open in Gerrit")
// This method gets called when icons are being dragged, but we don't want width update
// to happen then.Masa FujitaCurious - why does this get called during icon drag?
Items swapping triggers a layout pass for the RecyclerView.
Acknowledged
/** Returns the number of actions that are visible in the {@link RecyclerView}. */
int getNumberOfVisibleActions() {
return mModels.size();
}Not needed anymore?
int maxNumberOfItems = Integer.MAX_VALUE;
if (mAvailableWidth != null) {
int itemWidth =
mContext.getResources().getDimensionPixelSize(R.dimen.toolbar_button_width);
assert itemWidth > 0;
maxNumberOfItems = mAvailableWidth / itemWidth;
}
// O(N) for removals/no-ops; O(N^2) for reordering/insertions.
int currentModelIndex = 0;
for (String actionId : actionIds) {
if (currentModelIndex >= maxNumberOfItems) {
break;
}Masa Fujitaiiuc this method updates mModels when new action is added, pinned , unpinned..
1. Is onActionAdded() is called when a new extension is installed? If not, when is this invoked? I see on desktop we temporarily add a button to the toolbar which could show a popup or hide after a few seconds.
2. onActionRemoved() removed item from the models. iiuc this is invoked if extension is uninstalled? Do we need to reconcile the items to since we now have space to show 1 more?
1. On Clank, the UX requirements are different than Desktop (per UX decisions) - on install we show a dialog instead of popped out actions (temporarily pinned action). So as for what's displayed on the toolbar, we just have to match it to the stored pinned_actions. That said, popped out actions will be necessary (e.g. for popups opened via the extensions menu) and it's implemented in the stacked CL
2. When an extension is uninstalled, that model should be removed from the ModelList and the width of the RecyclerView should shrink, unless there are hidden icons due to space concerns, in which case this logic should swap the model (I'm not sure if I understand the question, does this answer it?)
On #2 - the usecase I am referring to:
- We have 10 pinned actions but availableWidth can only accommodate 5. mModel has 5 items that are visible
- User then uninstalls an extension -> onActionRemoved() is called which just removes the action from mModel. Don't we need to update mModel to add another item from 9 pinned actions since we available width hasn't changed and we have space to show 1 more? Where is this being handled?
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. |
Masa Fujita (Gerrit)
Masa Fujita voted and added 2 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
2 comments
/** Returns the number of actions that are visible in the {@link RecyclerView}. */
int getNumberOfVisibleActions() {
return mModels.size();
}Not needed anymore?
Sorry!
int maxNumberOfItems = Integer.MAX_VALUE;
if (mAvailableWidth != null) {
int itemWidth =
mContext.getResources().getDimensionPixelSize(R.dimen.toolbar_button_width);
assert itemWidth > 0;
maxNumberOfItems = mAvailableWidth / itemWidth;
}
// O(N) for removals/no-ops; O(N^2) for reordering/insertions.
int currentModelIndex = 0;
for (String actionId : actionIds) {
if (currentModelIndex >= maxNumberOfItems) {
break;
}Masa Fujitaiiuc this method updates mModels when new action is added, pinned , unpinned..
1. Is onActionAdded() is called when a new extension is installed? If not, when is this invoked? I see on desktop we temporarily add a button to the toolbar which could show a popup or hide after a few seconds.
2. onActionRemoved() removed item from the models. iiuc this is invoked if extension is uninstalled? Do we need to reconcile the items to since we now have space to show 1 more?
Sirisha Kavuluru1. On Clank, the UX requirements are different than Desktop (per UX decisions) - on install we show a dialog instead of popped out actions (temporarily pinned action). So as for what's displayed on the toolbar, we just have to match it to the stored pinned_actions. That said, popped out actions will be necessary (e.g. for popups opened via the extensions menu) and it's implemented in the stacked CL
2. When an extension is uninstalled, that model should be removed from the ModelList and the width of the RecyclerView should shrink, unless there are hidden icons due to space concerns, in which case this logic should swap the model (I'm not sure if I understand the question, does this answer it?)
On #2 - the usecase I am referring to:
- We have 10 pinned actions but availableWidth can only accommodate 5. mModel has 5 items that are visible
- User then uninstalls an extension -> onActionRemoved() is called which just removes the action from mModel. Don't we need to update mModel to add another item from 9 pinned actions since we available width hasn't changed and we have space to show 1 more? Where is this being handled?
Ah I understand! We were relient on the next `onPinnedActionsChanged()` call but I think it might be better to just use `reconcileActionItems()` here too. It's conceptually only very worse performance worse, but this is only called when the user uninstalls an extension
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sirisha Kavuluru
| Code-Review | +1 |
1 comment
/** Returns the number of actions that are visible in the {@link RecyclerView}. */
int getNumberOfVisibleActions() {
return mModels.size();
}Masa FujitaNot needed anymore?
Sorry!
- Masa Fujita
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 | +2 |
1 comment
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListMediatorTest.java
Insertions: 0, Deletions: 23.
The diff is too large to show. Please review the diff.
```
Change information
actions revamp: Add actions list to the toolbar rankings
This CL adds the actions list to the toolbar ranking. We follow the
request by `ToolbarTablet` and add or remove actions from the
`ModelList` to make sure that the actions list width is under the
provided `availableWidth`.
Demo: http://screencast/cast/NTM1NTUyMzcyODczNjI1NnxiMDliNzU5Mi1jOQ
- M chrome/browser/ui/android/extensions/java/res/values/dimens.xml
- 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/ExtensionActionListMediatorTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListRecyclerView.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionToolbarCoordinatorImpl.java
Code-Review: +1 by Sirisha Kavuluru
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |