Allow tab and group go back to its origin after drag cancelled [chromium/src : main]
Fuhsin Liao (Gerrit)
Fuhsin Liao voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 4 comments![]( "Open in Gerrit")
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {With the current design, I find it difficult to track who calls setIsDragCancelled() with what value.
Instead of adding `getIsDragCancelled` and `setIsDragCancelled` to the delegate, how about either of the following?
- Add a new parameter to stopReorderMode()
- Add a new method to handle cancelling (e.g. cancelReorderMode())
protected int mOriginIndex = TabModel.INVALID_TAB_INDEX;This variable doesn't need to exist in this class. Please add it to where it's actually needed (e.g. TabReorderStrategy and GroupReorderStrategy).
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Is this value any different from `!dropHandled`?
public boolean isDragInProcess() {Not needed?
Related details
- Fuhsin Liao
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
| Commit-Queue | +1 |
3 comments
protected int mOriginIndex = TabModel.INVALID_TAB_INDEX;This variable doesn't need to exist in this class. Please add it to where it's actually needed (e.g. TabReorderStrategy and GroupReorderStrategy).
Done
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Is this value any different from `!dropHandled`?
Yes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
public boolean isDragInProcess() {Fuhsin LiaoNot needed?
Yup I accidentally include this during rebase
Related details
- Fuhsin Liao
- Ryo Hashimoto
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
1 comment
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {With the current design, I find it difficult to track who calls setIsDragCancelled() with what value.
Instead of adding `getIsDragCancelled` and `setIsDragCancelled` to the delegate, how about either of the following?
- Add a new parameter to stopReorderMode()
- Add a new method to handle cancelling (e.g. cancelReorderMode())
Passed the isDragCancelled to stopReorderMode(). However in 'SourceViewDragDropReorderStrategy', I need to pass isDragCancelled to a bunch of 'onStopViewDragAction' functions and I'm afraid that my change is too wide that it will break some things
Related details
- Ryo Hashimoto
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. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 5 comments![]( "Open in Gerrit")
Token interactingTabGroupId = assumeNonNull(mInteractingGroupTitle).getTabGroupId();No need to assumeNonNull?
`tabGroupId` parameter is marked as @Nullable.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabGroupModelFilter.java;l=120;drc=d9a7d1399cb299664db77d32ba175ecc1137aade
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {To make it easier to understand this code, how bout passing the boolean value as a param, instead of a member variable?
```suggestion
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles, boolean isDragCancelled) {
```
void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles);How about adding a default implementation for this method so that you don't need to repeat it in subclasses?
```suggestion
default void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {
stopReorderMode(stripViews, gruopTitles, false);
}
```
boolean isDragCancelled) {}Why you need to have this empty default implementation?
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Fuhsin LiaoIs this value any different from `!dropHandled`?
Yes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
If dropped at an invalid place, shouldn't we also restore the original position?
That'll also allows us to remove mIsManuallyCancelled from TabDragHandlerBase.
Related details
- Fuhsin Liao
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. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 1 comment![]( "Open in Gerrit")
Also please fix the failing tests
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 5 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
Also please fix the failing tests
Done
Token interactingTabGroupId = assumeNonNull(mInteractingGroupTitle).getTabGroupId();No need to assumeNonNull?
`tabGroupId` parameter is marked as @Nullable.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabGroupModelFilter.java;l=120;drc=d9a7d1399cb299664db77d32ba175ecc1137aade
Oh I think you might misunderstand? I assumeNonNull(mInteractingGroupTitle) only, not the whole line. mInteractingGroupTitle is Nullable so remove assumeNonNull cause a warning
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {To make it easier to understand this code, how bout passing the boolean value as a param, instead of a member variable?
```suggestion
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles, boolean isDragCancelled) {
```
good idea. Done
void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles);How about adding a default implementation for this method so that you don't need to repeat it in subclasses?
```suggestion
default void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {
stopReorderMode(stripViews, gruopTitles, false);
}
```
good idea
boolean isDragCancelled) {}Why you need to have this empty default implementation?
So that I don't have to implement stopReorderMode w/ 3 param in classes that's not using it, for example, `ExternalViewDragDropReorderStrategy.java` and `MultiTabReorderStrategy.java`
Related details
- Ryo Hashimoto
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
1 comment
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Fuhsin LiaoIs this value any different from `!dropHandled`?
Ryo HashimotoYes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
If dropped at an invalid place, shouldn't we also restore the original position?
That'll also allows us to remove mIsManuallyCancelled from TabDragHandlerBase.
Hmm logically speaking yes, but one of the 'invalid place' is the location bar(address bar). If we replace `getIsManuallyCancelled` with `!drophandled`, then when a user releases the tab at the location bar, it will bounce back. However in chrome, the same behavior results in open a new window. I thought clank should stick to chrome's behavior as much as possible, right?
Related details
- Ryo Hashimoto
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. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 3 comments![]( "Open in Gerrit")
mIsManuallyCancelled = true;Why is this variable called `mIsManuallyCancelled` instead of `mWasCancelled`? What does `manually` in this context mean? When can a drag evnet be cancelled not manually?
boolean isDragCancelled) {}Fuhsin LiaoWhy you need to have this empty default implementation?
So that I don't have to implement stopReorderMode w/ 3 param in classes that's not using it, for example, `ExternalViewDragDropReorderStrategy.java` and `MultiTabReorderStrategy.java`
That introduces inconsistency between the two `stopReorderMode` methods. In other words, calling `stopReorderMode(stripViews, groupTitles, false)` should have the same effect as `stopReorderMode(stripViews, groupTitles)`, but with this empty impl it's inconsistent in classes like ExternalViewDragDropReorderStrategy.
How about doing the following?
- No default implementation for the 3-param stopReorderMode() method.
- Add the new boolean param to ExternalViewDragDropReorderStrategy's stopReorderMode() impl.
This way, the two stopReorderMode() impls are consistent thanks to the default impl of the 2-param stopReorderMode.
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Fuhsin LiaoIs this value any different from `!dropHandled`?
Ryo HashimotoYes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
Fuhsin LiaoIf dropped at an invalid place, shouldn't we also restore the original position?
That'll also allows us to remove mIsManuallyCancelled from TabDragHandlerBase.
Hmm logically speaking yes, but one of the 'invalid place' is the location bar(address bar). If we replace `getIsManuallyCancelled` with `!drophandled`, then when a user releases the tab at the location bar, it will bounce back. However in chrome, the same behavior results in open a new window. I thought clank should stick to chrome's behavior as much as possible, right?
The API documentation for ACTION_DRAG_ENDED says getResult() returns the boolean value returned for ACTION_DROP.
https://developer.android.com/reference/android/view/DragEvent#ACTION_DRAG_ENDED
Isn't it possible to return true when handling ACTION_DROP if we want to differentiate it from cancelled cases?
Another option is changing the code to invoke the tab position restoration logic when handling ESC key press. This way you don't need to care if `dropEvent==false` means ESC key press or an invalid drop destination.
The current code structure sets a boolean value `mIsManuallyCancelled` in TabDragHandlerBase and uses it in TabStripDragHandler, and it's difficult to maintain. It'd be appreciated if you could avoid intorducing this new data member.
Related details
- Fuhsin Liao
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
3 comments
Why is this variable called `mIsManuallyCancelled` instead of `mWasCancelled`? What does `manually` in this context mean? When can a drag evnet be cancelled not manually?
Make sense
boolean isDragCancelled) {}Fuhsin LiaoWhy you need to have this empty default implementation?
Ryo HashimotoSo that I don't have to implement stopReorderMode w/ 3 param in classes that's not using it, for example, `ExternalViewDragDropReorderStrategy.java` and `MultiTabReorderStrategy.java`
That introduces inconsistency between the two `stopReorderMode` methods. In other words, calling `stopReorderMode(stripViews, groupTitles, false)` should have the same effect as `stopReorderMode(stripViews, groupTitles)`, but with this empty impl it's inconsistent in classes like ExternalViewDragDropReorderStrategy.
How about doing the following?
- No default implementation for the 3-param stopReorderMode() method.
- Add the new boolean param to ExternalViewDragDropReorderStrategy's stopReorderMode() impl.
This way, the two stopReorderMode() impls are consistent thanks to the default impl of the 2-param stopReorderMode.
Marked as resolved.
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Fuhsin LiaoIs this value any different from `!dropHandled`?
Ryo HashimotoYes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
Fuhsin LiaoIf dropped at an invalid place, shouldn't we also restore the original position?
That'll also allows us to remove mIsManuallyCancelled from TabDragHandlerBase.
Ryo HashimotoHmm logically speaking yes, but one of the 'invalid place' is the location bar(address bar). If we replace `getIsManuallyCancelled` with `!drophandled`, then when a user releases the tab at the location bar, it will bounce back. However in chrome, the same behavior results in open a new window. I thought clank should stick to chrome's behavior as much as possible, right?
The API documentation for ACTION_DRAG_ENDED says getResult() returns the boolean value returned for ACTION_DROP.
https://developer.android.com/reference/android/view/DragEvent#ACTION_DRAG_ENDEDIsn't it possible to return true when handling ACTION_DROP if we want to differentiate it from cancelled cases?
Another option is changing the code to invoke the tab position restoration logic when handling ESC key press. This way you don't need to care if `dropEvent==false` means ESC key press or an invalid drop destination.
The current code structure sets a boolean value `mIsManuallyCancelled` in TabDragHandlerBase and uses it in TabStripDragHandler, and it's difficult to maintain. It'd be appreciated if you could avoid intorducing this new data member.
Hmmm after thorough thoughts, I don't think either suggestion 1. or 2. would work, or maybe it's because my knowledge is limited, please let me know if you hold a different opinion. For suggestion 2, I think a variable to be passed into stopReorderMode() is still necessary, and I also understand that it might be difficult to maintain this variable if it is set and used in different places, so I moved the mWasCancelled to `TabStripDragHandler.java`. For suggestion 1, in order to modify the result of invalid drop, i need to modify the logic in onDrop, but it's a complicated function an I'd rather not touch it. (I feel like it's not worth it)
Related details
- Ryo Hashimoto
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. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ryo Hashimoto
| Code-Review | +1 |
3 comments
if (getIsManuallyCancelled()) mStripLayoutHelperSupplier.get().stopReorderModeDueToCancel();Fuhsin LiaoIs this value any different from `!dropHandled`?
Ryo HashimotoYes since `!dropHandled` includes drop in the invalid place & cancel drag, but `getIsManuallyCancelled` includes only cancel drag.
The prior one cause a test to fail -- The test expects stopReorderMode to be called yet stopReorderModeDueToCancel is called. I'm pretty sure that test isn't about canceling drag, so it shouldn't call stopReorderModeDueToCancel() even if it does no harm.
Fuhsin LiaoIf dropped at an invalid place, shouldn't we also restore the original position?
That'll also allows us to remove mIsManuallyCancelled from TabDragHandlerBase.
Ryo HashimotoHmm logically speaking yes, but one of the 'invalid place' is the location bar(address bar). If we replace `getIsManuallyCancelled` with `!drophandled`, then when a user releases the tab at the location bar, it will bounce back. However in chrome, the same behavior results in open a new window. I thought clank should stick to chrome's behavior as much as possible, right?
Fuhsin LiaoThe API documentation for ACTION_DRAG_ENDED says getResult() returns the boolean value returned for ACTION_DROP.
https://developer.android.com/reference/android/view/DragEvent#ACTION_DRAG_ENDEDIsn't it possible to return true when handling ACTION_DROP if we want to differentiate it from cancelled cases?
Another option is changing the code to invoke the tab position restoration logic when handling ESC key press. This way you don't need to care if `dropEvent==false` means ESC key press or an invalid drop destination.
The current code structure sets a boolean value `mIsManuallyCancelled` in TabDragHandlerBase and uses it in TabStripDragHandler, and it's difficult to maintain. It'd be appreciated if you could avoid intorducing this new data member.
Hmmm after thorough thoughts, I don't think either suggestion 1. or 2. would work, or maybe it's because my knowledge is limited, please let me know if you hold a different opinion. For suggestion 2, I think a variable to be passed into stopReorderMode() is still necessary, and I also understand that it might be difficult to maintain this variable if it is set and used in different places, so I moved the mWasCancelled to `TabStripDragHandler.java`. For suggestion 1, in order to modify the result of invalid drop, i need to modify the logic in onDrop, but it's a complicated function an I'd rather not touch it. (I feel like it's not worth it)
I see. The latest patch set looks much easier to understand. Thank you!
mWasCancelled = false;Probably it's better to reset this variable to false when drag starts instead of when drag ends.
Related details
- Fuhsin Liao
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
1 comment
Probably it's better to reset this variable to false when drag starts instead of when drag ends.
- Ryo Hashimoto
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. |
Neil Coronado (Gerrit)
Neil Coronado added 2 comments![]( "Open in Gerrit")
() -> mReorderDelegate.stopReorderMode(stripViews, groupTitles, false);nit: here and elsewhere, `..., /* isDragCancelled= */ false);`
mModel.moveTab(mInteractingTab.getTabId(), mOriginIndex);I suspect that if the dragged tab was originally in a group, we also need to manually re-add it to that group here. Is that correct? If so, no pref about updating here or in a followup.
Related details
- Fuhsin Liao
- Ryo Hashimoto
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
2 comments
() -> mReorderDelegate.stopReorderMode(stripViews, groupTitles, false);nit: here and elsewhere, `..., /* isDragCancelled= */ false);`
Done
mModel.moveTab(mInteractingTab.getTabId(), mOriginIndex);I suspect that if the dragged tab was originally in a group, we also need to manually re-add it to that group here. Is that correct? If so, no pref about updating here or in a followup.
Yup you're right. Dragging a tab from a group and then cancel it with ESC dosen't make it bounce back to the group. Will fix it in the following cls
Related details
- Neil Coronado
- Ryo Hashimoto
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. |
Fuhsin Liao (Gerrit)
Fuhsin Liao added 1 comment![]( "Open in Gerrit")
mModel.moveTab(mInteractingTab.getTabId(), mOriginIndex);Fuhsin LiaoI suspect that if the dragged tab was originally in a group, we also need to manually re-add it to that group here. Is that correct? If so, no pref about updating here or in a followup.
Yup you're right. Dragging a tab from a group and then cancel it with ESC dosen't make it bounce back to the group. Will fix it in the following cls
Done
Fuhsin Liao (Gerrit)
Fuhsin Liao added 1 comment![]( "Open in Gerrit")
public void stopReorderMode(StripLayoutView[] stripViews, StripLayoutGroupTitle[] groupTitles) {With the current design, I find it difficult to track who calls setIsDragCancelled() with what value.
Instead of adding `getIsDragCancelled` and `setIsDragCancelled` to the delegate, how about either of the following?
- Add a new parameter to stopReorderMode()
- Add a new method to handle cancelling (e.g. cancelReorderMode())
Passed the isDragCancelled to stopReorderMode(). However in 'SourceViewDragDropReorderStrategy', I need to pass isDragCancelled to a bunch of 'onStopViewDragAction' functions and I'm afraid that my change is too wide that it will break some things
Done
Related details
- Neil Coronado
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado voted Code-Review+1![]( "Open in Gerrit")
- Fuhsin Liao
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Fuhsin Liao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |