[Vertical Tabs] Make the top left window drag space draggable [chromium/src : main]
Eleanor Lee (Gerrit)
Eleanor Lee added 1 comment![]( "Open in Gerrit")
This currently makes the entire `desktop_window_spacer` (https://screenshot.googleplex.com/4KLjQ3am9jToJCx) area draggable.
Looking at the slides (https://docs.google.com/presentation/d/19va4-VY0g9XQoN7DGutaI-mAwGUGOKURQsm2_-kAfqo/edit?slide=id.g3db50669601_4_211#slide=id.g3db50669601_4_211), do we want to make changes so that the left most part of the spacer (where the Chrome logo is) is specifically not draggable?
Related details
- Aishwarya Rajesh
- Gazal Agarwal
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. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 1 comment![]( "Open in Gerrit")
This currently makes the entire `desktop_window_spacer` (https://screenshot.googleplex.com/4KLjQ3am9jToJCx) area draggable.
Looking at the slides (https://docs.google.com/presentation/d/19va4-VY0g9XQoN7DGutaI-mAwGUGOKURQsm2_-kAfqo/edit?slide=id.g3db50669601_4_211#slide=id.g3db50669601_4_211), do we want to make changes so that the left most part of the spacer (where the Chrome logo is) is specifically not draggable?
- Aishwarya Rajesh
- Eleanor Lee
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 4 comments![]( "Open in Gerrit")
Sorry for the delay! I have a general comment on the current code changes. Also, can we fix the compile error and run the CQ once we upload a new PS?
Gazal AgarwalThis currently makes the entire `desktop_window_spacer` (https://screenshot.googleplex.com/4KLjQ3am9jToJCx) area draggable.
Looking at the slides (https://docs.google.com/presentation/d/19va4-VY0g9XQoN7DGutaI-mAwGUGOKURQsm2_-kAfqo/edit?slide=id.g3db50669601_4_211#slide=id.g3db50669601_4_211), do we want to make changes so that the left most part of the spacer (where the Chrome logo is) is specifically not draggable?
That'r right. The Chrome logo shouldn't be draggable.
FWIW, we do allow dragging on longpress on the app header app chip icon in a freeform window currently (while rendering horizontal tabs), I don't see why this needs to be different when VT is enabled - did we get specific UX direction to do so?
private static final int DEFAULT_DESKTOP_WINDOW_SPACER_WIDTH_PX = 266;It doesn't look right to hardcode this constant - can't we fetch this from a dimens resource or something?
int spacerId =
context.getResources()
.getIdentifier(
"desktop_window_spacer",
"id",
context.getPackageName());
// Grab the top of the entire window hierarchy and search downward to locate the
// instantiated spacer view object.
View rootView = getRootView();
if (rootView != null && spacerId != 0) {
View spacerView = rootView.findViewById(spacerId);
if (spacerView != null && spacerView.getVisibility() == View.VISIBLE) {
// Capture the spacer view's coordinates relative to the window frame.
int[] location = new int[2];
spacerView.getLocationInWindow(location);
// Left starting point of the vertical rail/spacer.
int spacerX = location[0];
int spacerViewWidth = spacerView.getWidth();
// Ensure that the spacer view has gone through a measure & layout pass.
if (spacerViewWidth > 0) {
targetLeftEdge = spacerX + spacerViewWidth;
}
}
}General remark: I think we should have some dependency injection for these values from a VT class; for example, could we define an Integer supplier that ToolbarControlContainer has access to, to get the width of the spacer view? Some VT class can set the value of this supplier appropriately. I don't think ToolbarControlContainer needs to know the specifics of the desktop_window_spacer view.
Related details
- Eleanor Lee
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. |
Eleanor Lee (Gerrit)
Eleanor Lee added 4 comments![]( "Open in Gerrit")
Will add unit tests after this round of reviews.
Gazal AgarwalThis currently makes the entire `desktop_window_spacer` (https://screenshot.googleplex.com/4KLjQ3am9jToJCx) area draggable.
Looking at the slides (https://docs.google.com/presentation/d/19va4-VY0g9XQoN7DGutaI-mAwGUGOKURQsm2_-kAfqo/edit?slide=id.g3db50669601_4_211#slide=id.g3db50669601_4_211), do we want to make changes so that the left most part of the spacer (where the Chrome logo is) is specifically not draggable?
Aishwarya RajeshThat'r right. The Chrome logo shouldn't be draggable.
FWIW, we do allow dragging on longpress on the app header app chip icon in a freeform window currently (while rendering horizontal tabs), I don't see why this needs to be different when VT is enabled - did we get specific UX direction to do so?
Messaged Julia to confirm, will upload a follow-up CL if needed! Not sure if this is related, but I did notice that in the original code, the entire area to the right of the 3 dot menu also is draggable (the minimize icon, full window icon, exit icon are all draggable).
private static final int DEFAULT_DESKTOP_WINDOW_SPACER_WIDTH_PX = 266;It doesn't look right to hardcode this constant - can't we fetch this from a dimens resource or something?
Done
int spacerId =
context.getResources()
.getIdentifier(
"desktop_window_spacer",
"id",
context.getPackageName());
// Grab the top of the entire window hierarchy and search downward to locate the
// instantiated spacer view object.
View rootView = getRootView();
if (rootView != null && spacerId != 0) {
View spacerView = rootView.findViewById(spacerId);
if (spacerView != null && spacerView.getVisibility() == View.VISIBLE) {
// Capture the spacer view's coordinates relative to the window frame.
int[] location = new int[2];
spacerView.getLocationInWindow(location);
// Left starting point of the vertical rail/spacer.
int spacerX = location[0];
int spacerViewWidth = spacerView.getWidth();
// Ensure that the spacer view has gone through a measure & layout pass.
if (spacerViewWidth > 0) {
targetLeftEdge = spacerX + spacerViewWidth;
}
}
}General remark: I think we should have some dependency injection for these values from a VT class; for example, could we define an Integer supplier that ToolbarControlContainer has access to, to get the width of the spacer view? Some VT class can set the value of this supplier appropriately. I don't think ToolbarControlContainer needs to know the specifics of the desktop_window_spacer view.
- Aishwarya Rajesh
- Gazal Agarwal
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 3 comments![]( "Open in Gerrit")
View controlContainerView =
activity.findViewById(org.chromium.chrome.R.id.control_container);
if (controlContainerView instanceof ToolbarControlContainer controlContainer) {
controlContainer.setDesktopWindowSpacerWidthSupplier(Instead of doing this, could we pass in a settable Integer supplier (defined as a member in `TabbedRootUiCoordinator`) to the ctor of `VerticalTabListCoordinator` and convey this to the `ToolbarControlContainer` as we do for the boolean supplier here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tabbed_mode/TabbedRootUiCoordinator.java;l=1022;drc=3fa1318505ea6d8885ef471d9cc0281f9468ff8d
int width = mSpacerView.getWidth();
if (width == 0) {
return activity.getResources()
.getDimensionPixelSize(
R.dimen.vertical_tabs_desktop_spacer_width);
}
// Get the position of both views relative to the window surface.
int[] spacerLocation = new int[2];
int[] toolbarLocation = new int[2];
mSpacerView.getLocationInWindow(spacerLocation);
controlContainer.getLocationInWindow(toolbarLocation);
// Calculate exactly where the spacer ends relative to the window.
return spacerLocation[0] + width - toolbarLocation[0];I'm wondering if instead of having the supplier provide a "desktop spacer width", we can simply have this supplier provide the vertical tabs container width (ie. VIEW_WIDTH_DP IIUC). And then `ToolbarControlContainer` can use this as the left bound of the system gesture exclusion rect as is - this is assuming that the entire width of the VT container (including the app chip icon region on the left side of the app header) is to be delegated to the system for gesture handling. Would this work?
int spacerWidth = mDesktopWindowSpacerWidthSupplier.get();`spacerWidth` is very ambiguous within the current class' context.
Like I suggested in my other comment, I think we can have this supplier provide the vertical tabs container width, which will be 0 when VT is disabled. Here we can do:
```
int left = mVerticalTabsContainerWidthSupplier.get();
...
Rect exclusionRect = new Rect(left, top, right, bottom);
```
Will this work as needed?
Related details
- Eleanor Lee
- Gazal Agarwal
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. |
Eleanor Lee (Gerrit)
Eleanor Lee added 3 comments![]( "Open in Gerrit")
View controlContainerView =
activity.findViewById(org.chromium.chrome.R.id.control_container);
if (controlContainerView instanceof ToolbarControlContainer controlContainer) {
controlContainer.setDesktopWindowSpacerWidthSupplier(Instead of doing this, could we pass in a settable Integer supplier (defined as a member in `TabbedRootUiCoordinator`) to the ctor of `VerticalTabListCoordinator` and convey this to the `ToolbarControlContainer` as we do for the boolean supplier here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tabbed_mode/TabbedRootUiCoordinator.java;l=1022;drc=3fa1318505ea6d8885ef471d9cc0281f9468ff8d
Done
int width = mSpacerView.getWidth();
if (width == 0) {
return activity.getResources()
.getDimensionPixelSize(
R.dimen.vertical_tabs_desktop_spacer_width);
}
// Get the position of both views relative to the window surface.
int[] spacerLocation = new int[2];
int[] toolbarLocation = new int[2];
mSpacerView.getLocationInWindow(spacerLocation);
controlContainer.getLocationInWindow(toolbarLocation);
// Calculate exactly where the spacer ends relative to the window.
return spacerLocation[0] + width - toolbarLocation[0];I'm wondering if instead of having the supplier provide a "desktop spacer width", we can simply have this supplier provide the vertical tabs container width (ie. VIEW_WIDTH_DP IIUC). And then `ToolbarControlContainer` can use this as the left bound of the system gesture exclusion rect as is - this is assuming that the entire width of the VT container (including the app chip icon region on the left side of the app header) is to be delegated to the system for gesture handling. Would this work?
Done
int spacerWidth = mDesktopWindowSpacerWidthSupplier.get();`spacerWidth` is very ambiguous within the current class' context.
Like I suggested in my other comment, I think we can have this supplier provide the vertical tabs container width, which will be 0 when VT is disabled. Here we can do:
```
int left = mVerticalTabsContainerWidthSupplier.get();
...
Rect exclusionRect = new Rect(left, top, right, bottom);
```Will this work as needed?
- Aishwarya Rajesh
- Gazal Agarwal
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 3 comments![]( "Open in Gerrit")
These changes generally LGTM, have a couple of comments. I think we can work on adding test coverage at this time.
ToolbarControlContainer controlContainer = mActivity.findViewById(R.id.control_container);Maybe it's okay to maintain a nullable internal state `mControlContainer` and use that here instead? We can set this in `initializeToolbar`. I see that a test is complaining of a null `mActivity`, maybe we could alternatively add a null-check on the activity instance before trying to resolve the control container.
private @Nullable NonNullObservableSupplier<Integer> mVerticalTabsContainerWidthSupplier;Why is this nullable, can't we just use a non-null supplier with a default value of 0?
Related details
- Eleanor Lee
- Gazal Agarwal
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. |
Eleanor Lee (Gerrit)
Eleanor Lee added 3 comments![]( "Open in Gerrit")
Will add unit tests after this round of reviews.
Done
ToolbarControlContainer controlContainer = mActivity.findViewById(R.id.control_container);Maybe it's okay to maintain a nullable internal state `mControlContainer` and use that here instead? We can set this in `initializeToolbar`. I see that a test is complaining of a null `mActivity`, maybe we could alternatively add a null-check on the activity instance before trying to resolve the control container.
Done
private @Nullable NonNullObservableSupplier<Integer> mVerticalTabsContainerWidthSupplier;Why is this nullable, can't we just use a non-null supplier with a default value of 0?
- Aishwarya Rajesh
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted and added 5 comments![]( "Open in Gerrit")
Votes added by Aishwarya Rajesh
| Code-Review | +1 |
5 comments
mContainerView.addOnLayoutChangeListener(Recommended by AI:
The `OnLayoutChangeListener` added here is never removed. To avoid memory leaks or unexpected updates to the supplier after the coordinator is destroyed, please store this listener as a member variable and remove it in the `destroy()` method using `mContainerView.removeOnLayoutChangeListener()`.
// Notifies mVerticalTabsWidthObserver in ToolbarControlContainer, triggering
// #updateSystemGestureExclusions and #setSystemGestureExclusionRects.nit: we should probably remove this specific detail, it's easy to miss removing such comments if the usage of this supplier is ever updated
if (mVerticalTabsContainerWidthSupplier != null) {Recommended by AI:
This null check is redundant because `mVerticalTabsContainerWidthSupplier` is initialized to a non-null value in its declaration and is kept non-null via the setter. However, it's harmless.
// The left edge of the exclusion rectangle dictates where the draggable desktop windowJust curious: would we show the VT rail on the right side of the window in an RTL layout? If yes, then we should consider accordingly updating the gesture exclusion rects below. If this is not applicable, probably okay to keep the current implementation as is.
Related details
- Eleanor Lee
- Gazal Agarwal
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. |