[Drag and Drop] Add onTouchListener for drag handle [chromium/src : main]
Eleanor Lee (Gerrit)
Eleanor Lee added 1 comment![]( "Open in Gerrit")
Unit tests will be added as a chained CL since this CL is already large.
Related details
- Brandon Wylie
- Mark Schillaci
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. |
Mark Schillaci (Gerrit)
Mark Schillaci voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Schillaci
| Code-Review | +1 |
1 comment
lgtm! thanks eleanor!
Related details
- Brandon Wylie
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. |
Brandon Wylie (Gerrit)
Brandon Wylie voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brandon Wylie
| Code-Review | +1 |
1 comment
- 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 removed Matthew Jones from this change![]( "Open in Gerrit")
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 4 comments![]( "Open in Gerrit")
Naive question on the approach/ I don't have access to the design doc, so maybe some of the question already have answers
for (int i = childrenCount - 1; i >= 0; --i) {Not a great fan of this traversal... it seems this section is meant to update the mouse icon during drag and drop.
Since Android event dispatcher usually starts from the child, back to the parent, I wonder if you can just override the child view's `onResolvePointerIcon` that you care about?
event.setLocation(localX, localY);nit: Can you do [MotionEvent.obtain](https://developer.android.com/reference/android/view/MotionEvent#obtain(android.view.MotionEvent)) do copy the event?
for (int i = 0; i < mRecyclerView.getChildCount(); i++) {
View child = mRecyclerView.getChildAt(i);
child.setPointerIcon(icon);
}This loops through the entire RecyclerView when a selection state changes, which feels quite expensive...
I wonder if we could just make the recycler view children a custom view that can handle reading the icon from a supplier (which owned by either the adapter or the coordinator)?
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 5 comments![]( "Open in Gerrit")
Naive question on the approach/ I don't have access to the design doc, so maybe some of the question already have answers
Added a direct link to the design doc in the bug comment #2 and now I think anyone with the link can view the doc.
Unit tests will be added as a chained CL since this CL is already large.
Done
for (int i = childrenCount - 1; i >= 0; --i) {Not a great fan of this traversal... it seems this section is meant to update the mouse icon during drag and drop.
Since Android event dispatcher usually starts from the child, back to the parent, I wonder if you can just override the child view's `onResolvePointerIcon` that you care about?
That is what I tried at first, but I think we directly need to modify CompositorViewHolder#onResolvePointerIcon. The reasoning is in this portion of the design doc (Section 8. in Proposed Design Changes):
There is also an on-going discussion with Theresa about why I added this specific traversal logic:
nit: Can you do [MotionEvent.obtain](https://developer.android.com/reference/android/view/MotionEvent#obtain(android.view.MotionEvent)) do copy the event?
Done
for (int i = 0; i < mRecyclerView.getChildCount(); i++) {
View child = mRecyclerView.getChildAt(i);
child.setPointerIcon(icon);
}This loops through the entire RecyclerView when a selection state changes, which feels quite expensive...
I wonder if we could just make the recycler view children a custom view that can handle reading the icon from a supplier (which owned by either the adapter or the coordinator)?
I think mRecyclerView.getChildCount() only returns the visible view count instead of the adapter count:
https://paste.googleplex.com/6260333788397568
And I think the loop is called only when the drag state changes (start drag, end drag, etc) so it does not run continuously during scroll or hover.
Related details
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
for (int i = childrenCount - 1; i >= 0; --i) {Eleanor LeeNot a great fan of this traversal... it seems this section is meant to update the mouse icon during drag and drop.
Since Android event dispatcher usually starts from the child, back to the parent, I wonder if you can just override the child view's `onResolvePointerIcon` that you care about?
That is what I tried at first, but I think we directly need to modify CompositorViewHolder#onResolvePointerIcon. The reasoning is in this portion of the design doc (Section 8. in Proposed Design Changes):
There is also an on-going discussion with Theresa about why I added this specific traversal logic:
Thanks for the pointer. I see now why you want to replicate this logic (See my comment in https://docs.google.com/document/d/1NKsngqlhivcnXKt3sAoTmb43gimqb0VmySNDXa6ET9g/edit?resourcekey=0-lrr3M6yb-Rc3xcA_KGEFlg&disco=AAAByDAW4vs)
---
I do wonder though, if we can just check if the current mView is a native view, then just call the super method, to bypass these web content's pointer resolution logic?
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 1 comment![]( "Open in Gerrit")
for (int i = childrenCount - 1; i >= 0; --i) {Eleanor LeeNot a great fan of this traversal... it seems this section is meant to update the mouse icon during drag and drop.
Since Android event dispatcher usually starts from the child, back to the parent, I wonder if you can just override the child view's `onResolvePointerIcon` that you care about?
That is what I tried at first, but I think we directly need to modify CompositorViewHolder#onResolvePointerIcon. The reasoning is in this portion of the design doc (Section 8. in Proposed Design Changes):
There is also an on-going discussion with Theresa about why I added this specific traversal logic:
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
2 comments
Glad this solution in patch#5 (forwarding event from CVH to mView) works :)
// Android has found the grabbing/grab cursor.nit: Remove this comment line - it's not necessary a grab cursor, it could be other views that has the cursor icon set ;)
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 1 comment![]( "Open in Gerrit")
nit: Remove this comment line - it's not necessary a grab cursor, it could be other views that has the cursor icon set ;)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eleanor Lee (Gerrit)
Eleanor Lee voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
Insertions: 0, Deletions: 2.
@@ -386,8 +386,6 @@
// Delegate to standard Android behavior (View Group). This internally loops through the
// children of the CompositorViewHolder and calculates the correct offsets.
PointerIcon icon = super.onResolvePointerIcon(event, pointerIndex);
-
- // Android has found the grabbing/grab cursor.
if (icon != null) return icon;
}
```
Change information
[Drag and Drop] Add onTouchListener for drag handle
The link for the design doc is in the bug in comment #2.
This CL covers sections 6. and 8. under "Proposed Design Changes".
After: https://screencast.googleplex.com/cast/NDc2NDcwMjEyMTkxODQ2NHwzNTFhZWRiOS1iNw
Notice that the mouse behavior is currently imperfect since we have not implemented hover listeners for the row body and the grab handle (will be chained to this CL).
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
- M chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManagerCoordinator.java
- M chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManagerDragHelper.java
- M chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/ImprovedBookmarkRow.java
- M chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/ImprovedBookmarkRowProperties.java
- M components/browser_ui/widget/android/java/src/org/chromium/components/browser_ui/widget/dragreorder/DragReorderableRecyclerViewAdapter.java
Code-Review: +1 by Brandon Wylie, +1 by Wenyu Fu, +1 by Mark Schillaci
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |